From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202508 header.b=UpzzUNsX; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id EA5855A0279 for ; Thu, 07 Aug 2025 08:18:02 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1754547479; bh=7/5vabTwmFbLPjDPNVgAIxgp7rM9nlNMOsPNgysK/zI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UpzzUNsXoK9Czn1UF/oaL1zuMI/PbSrSvn+zTrDVnkpPk82PjsfuqvcoG4BXN0RS1 KYU4eXK/tROIaMhYiFjK/6yuFHFh6TZSGd6kNdwdOu+UnSL0SXzrgZUPq067ZDLrBc /bZFMl1hZ39wIdkXvF3VleAbFSV7Ck+CWc3KoTy7Nh5u3Hp2ikDWG4/mltcSJn/zst qC6umb1J4vfxXAw5aAAOkNDcUfR2kJ3IxxWtHKz8mtv0QocQyJNHy2KyWEBDJYmjIi QOFcKx95HBnQ5iqFE4HRRq0LMu2XEH5/DpCowciVLgWjbqoym4C88sOr4DtmswHEFH WF5Pot/9Ugb+Q== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4byH5g1tRdz4xQW; Thu, 7 Aug 2025 16:17:59 +1000 (AEST) Date: Thu, 7 Aug 2025 16:17:33 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v8 30/30] packet: Add support for multi-vector packets Message-ID: References: <20250805154628.301343-31-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="S7qzh2njDkeF59PL" Content-Disposition: inline In-Reply-To: <20250805154628.301343-31-lvivier@redhat.com> Message-ID-Hash: 5DR6TTQ6JVEQUCUDRGSISRPPXQO55NXA X-Message-ID-Hash: 5DR6TTQ6JVEQUCUDRGSISRPPXQO55NXA X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --S7qzh2njDkeF59PL Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Aug 05, 2025 at 05:46:28PM +0200, Laurent Vivier wrote: > The packet pool was previously limited to handling packets contained > within a single buffer. >=20 > This patch extends the packet pool to support iovec array, > allowing a single logical packet to be composed of multiple iovec. >=20 > To accommodate this, the storage format within the pool is modified. > For a multi-vector packet, a header entry is now stored first with > iov_base =3D NULL and iov_len holding the number of subsequent > vectors. The actual data vectors are then stored in the following > pool slots. >=20 > The packet_add_do() and packet_get_do() functions are updated to > manage this new format for storing and retrieving packets. The > pool_full() check is also adjusted to ensure there is enough > space for all vectors of a new packet before adding it. >=20 > Signed-off-by: Laurent Vivier > --- > packet.c | 50 +++++++++++++++++++++++++++++++++----------------- > packet.h | 2 +- > tap.c | 4 ++-- > 3 files changed, 36 insertions(+), 20 deletions(-) >=20 > diff --git a/packet.c b/packet.c > index 4b93688509a4..d697232d951a 100644 > --- a/packet.c > +++ b/packet.c > @@ -90,12 +90,13 @@ static int packet_check_range(const struct pool *p, c= onst char *ptr, size_t len, > /** > * pool_full() - Is a packet pool full? > * @p: Pointer to packet pool > + * @data: check data can fit in the pool > * > - * Return: true if the pool is full, false if more packets can be added > + * Return: true if the pool is full, false if data can be added > */ > -bool pool_full(const struct pool *p) > +bool pool_full(const struct pool *p, const struct iov_tail *data) Given the slightly changed semantics, I wonder if 'pool_can_fit()' might be a better name now. > { > - return p->count >=3D p->size; > + return p->count + data->cnt + (data->cnt > 1) >=3D p->size; This test is only correct if data is already pruned. As I've said elsewhere, it might be worth changing to the assumption that iov_tails are pruned everywhere outside the iov_tail internal handling. Oh.. also I think the new check is off by one (in the relatively safe direction). It will say there's no room when there is just exactly enough room. > } > =20 > /** > @@ -108,11 +109,9 @@ bool pool_full(const struct pool *p) > void packet_add_do(struct pool *p, struct iov_tail *data, > const char *func, int line) > { > - size_t idx =3D p->count; > - const char *start; > - size_t len; > + size_t idx =3D p->count, i, offset; > =20 > - if (pool_full(p)) { > + if (pool_full(p, data)) { > debug("add packet index %zu to pool with size %zu, %s:%i", > idx, p->size, func, line); > return; > @@ -121,18 +120,30 @@ void packet_add_do(struct pool *p, struct iov_tail = *data, > if (!iov_tail_prune(data)) > return; > =20 > - ASSERT(data->cnt =3D=3D 1); /* we don't support iovec */ > + if (data->cnt > 1) { > + p->pkt[idx].iov_base =3D NULL; > + p->pkt[idx].iov_len =3D data->cnt; > + idx++; > + } > =20 > - len =3D data->iov[0].iov_len - data->off; > - start =3D (char *)data->iov[0].iov_base + data->off; > + offset =3D data->off; > + for (i =3D 0; i < data->cnt; i++) { > + const char *start; > + size_t len; > =20 > - if (packet_check_range(p, start, len, func, line)) > - return; > + len =3D data->iov[i].iov_len - offset; > + start =3D (char *)data->iov[i].iov_base + offset; > + offset =3D 0; > =20 > - p->pkt[idx].iov_base =3D (void *)start; > - p->pkt[idx].iov_len =3D len; > + if (packet_check_range(p, start, len, func, line)) > + return; > =20 > - p->count++; > + p->pkt[idx].iov_base =3D (void *)start; > + p->pkt[idx].iov_len =3D len; > + idx++; Hm. Isn't the above equivalent to iov_tail_clone()? Is calling packet_check_range() on each chunk the only reason for open-coding it here? > + } > + > + p->count =3D idx; > } > =20 > /** > @@ -162,9 +173,14 @@ bool packet_get_do(const struct pool *p, size_t idx, > return false; > } > =20 > - data->cnt =3D 1; > + if (p->pkt[idx].iov_base) { > + data->cnt =3D 1; > + data->iov =3D &p->pkt[idx]; > + } else { > + data->cnt =3D p->pkt[idx].iov_len; > + data->iov =3D &p->pkt[idx + 1]; > + } > data->off =3D 0; > - data->iov =3D &p->pkt[idx]; > =20 > for (i =3D 0; i < data->cnt; i++) { > ASSERT_WITH_MSG(!packet_check_range(p, data->iov[i].iov_base, > diff --git a/packet.h b/packet.h > index e51cbd19fdc4..67dc7deb17db 100644 > --- a/packet.h > +++ b/packet.h > @@ -37,7 +37,7 @@ void packet_add_do(struct pool *p, struct iov_tail *dat= a, > const char *func, int line); > bool packet_get_do(const struct pool *p, const size_t idx, > struct iov_tail *data, const char *func, int line); > -bool pool_full(const struct pool *p); > +bool pool_full(const struct pool *p, const struct iov_tail *data); > void pool_flush(struct pool *p); > =20 > #define packet_add(p, data) \ > diff --git a/tap.c b/tap.c > index 9fd00915bb01..95688b22fcb7 100644 > --- a/tap.c > +++ b/tap.c > @@ -1103,14 +1103,14 @@ void tap_add_packet(struct ctx *c, struct iov_tai= l *data, > switch (ntohs(eh->h_proto)) { > case ETH_P_ARP: > case ETH_P_IP: > - if (pool_full(pool_tap4)) { > + if (pool_full(pool_tap4, data)) { > tap4_handler(c, pool_tap4, now); > pool_flush(pool_tap4); > } > packet_add(pool_tap4, data); > break; > case ETH_P_IPV6: > - if (pool_full(pool_tap6)) { > + if (pool_full(pool_tap6, data)) { > tap6_handler(c, pool_tap6, now); > pool_flush(pool_tap6); > } --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --S7qzh2njDkeF59PL Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmiURO8ACgkQzQJF27ox 2GdOnA//XrbXm0pfpIFagwhBlY5f7x9LxgjA3FCRuMMC1899uRX0rguJdHY1XhD+ FNOeqW+Yq3CFj/HwIlJIUeEtqW5Z+QioNc6gACB8Zlq+iEdS76sf3A5gmoNrUb6o W2I7ct/YFKQm/9PCoc08ujs5VNLtSDFqnr+ZoIVHIkxXafKi4ebXEmxkJOUzzxLI V2A82y0waoQknKnnwgq5wqTQWTmoXGNTYceK2WCUoFh356Jj0LjYJWPC6Dk0qq/1 PAM5ut+nDz5TAHmhv7f+JGczLP6yL0DG8djyrMvAbQsAhW1oD92JsIrSXGR/G6SF zax1d9huA8mx3zEKHcT/rMBsnZtSWe/uctQz0FuF+UPzR6FOjFTdztAfPuiAbrGq k5h94Xx/H/m2bn+vkvYKT6Jv8QJYw9xxx1zciVV0lg9QgRHrunR+ig0drOy/xXar 7319pv+0esQkK68KbZdf3/JT81efyb/yObwwK/q+u/lo7TuiSTUYUGSdGJpBmUzb Uk2GuRrIXV15Lmh/T9YY7cnExvOpWiKxIlVFqli3hfQecPXweNgsedBvKhct11mr keLsg5sDt53tc+J5R4dqE1/vYaYkhBt3ASPoBMYz0Fuupzwws/TqxbcpiSKIoBmK HADq0KYgW9XQ8P+oPH090UEnNyjYdIRSabBqp0uATPD/PyerDAQ= =rXJB -----END PGP SIGNATURE----- --S7qzh2njDkeF59PL--