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=gOiM2ung; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 212655A027A for ; Fri, 15 Aug 2025 03:11:44 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1755220300; bh=ehOkix40KcAJUM+oXN7W99E/hTE3aip3qICK2Si9VUA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gOiM2ungBABpIIYt7O15Q7SenURmx85EO9SLg6fMHcy6fKufxTNQfMfBoq/vRrQ4K /1w835WFmVSDVuFwQlpAFyzhUEDy+K3IVOE8NRP8NHhACOmSLkMELcpUIpCuC6FGeF UKWOhFdg2FUfmkQu+/vmYsM/psXUecErnl9EXopdkM4HxWeuX2jk330F7FhicWH51v jnTDyAud5Q4gtKHGbRWYnYZPdc/ynRMe5kqikcoM8xH3QgN/KuyGglw3YFGtMr55Cs dSfBPk6MP3VKSl55bDi5MVQ2CokHufxv7iVhleQmxFbtzeXwhz4zrKnLZZku68X1QS 6nMQBlAVKcHKg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4c33wX5HSVz4xdg; Fri, 15 Aug 2025 11:11:40 +1000 (AEST) Date: Fri, 15 Aug 2025 11:08:36 +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> <84f40250-6c5c-48ae-a02b-52720d9cb455@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="jtYT7BoqwN6RdCzF" Content-Disposition: inline In-Reply-To: <84f40250-6c5c-48ae-a02b-52720d9cb455@redhat.com> Message-ID-Hash: U4XNSJQSKNOVA3QHSW5U5BBF6RERSILP X-Message-ID-Hash: U4XNSJQSKNOVA3QHSW5U5BBF6RERSILP 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: --jtYT7BoqwN6RdCzF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 14, 2025 at 11:02:57AM +0200, Laurent Vivier wrote: > On 07/08/2025 08:17, David Gibson wrote: > > 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, const 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 ad= ded > > > + * 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) > >=20 > > Given the slightly changed semantics, I wonder if 'pool_can_fit()' > > might be a better name now. >=20 > okay >=20 > >=20 > > > { > > > - return p->count >=3D p->size; > > > + return p->count + data->cnt + (data->cnt > 1) >=3D p->size; > >=20 > > 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. > >=20 > > 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 > Could you explain why you think it's off by 1? Loosely, because the old version needed to check if the pool was _already_ full, but the new version needs to check if it _will_ be full with the addition. Suppose there's exactly one free slot in the pool, say p->count=3D=3D99, p->size=3D=3D100. We attempt to add a single buffer packet, data->cnt=3D= =3D1. 99 + 1 + 0 >=3D 100 But it should (just) fit. > > > } > > > /** > > > @@ -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; > > > - 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_t= ail *data, > > > if (!iov_tail_prune(data)) > > > return; > > > - 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++; > > > + } > > > - 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; > > > - 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; > > > - 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; > > > - p->count++; > > > + p->pkt[idx].iov_base =3D (void *)start; > > > + p->pkt[idx].iov_len =3D len; > > > + idx++; > >=20 > > 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? >=20 > Yes, I think the code is clearer like this. And it avoids to scan the iov= ec > array twice (with the offset management). Ok. --=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 --jtYT7BoqwN6RdCzF Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmieiJQACgkQzQJF27ox 2GetiQ//RRrAo4Stn8rKlaXWK/7qZW3irsv3Daw/qAPu4L9n1YOH1ffDtIMsSUru B5pdaS7j+Dnsh8edRCJHEzOcEvF2mAZU7cCR93PP/UnLPpiysAG3VhKCcA+vqDR+ NyBkPT3khCmAF/ovck4XoisNQVqCWDkeiV8p9IsZzmUmTxfjwGzKkVNbDYqVPf0E 9pUWlS5zgD0XGvNVB0KCHKxz6uSFq160iLZlVX1rbgairwBnABKupe/krP9XS4QV CfmZLcMaINFMxzOKH3gHnDjMigNRCqtEUovLWyMxivMET5EyS8H3ewN3sTKpZyXf DySgm+UdAv9rC5R9HYAgI7ZovM61Ofzxfti3KEd/F/gCgCRur6nia4W+CRhUgOuc CeiTXs68xefulJQ3blc+6cwurX8YJiN1tr2UIxMhmsr3QtCbCECfDuARrQPnNq4B AZ2MoiwF2tD8KTWxjMpvXeVw4jxZ3NfUKgCV9tPPzbgkcn9RuSG8YOechDP4U76Q izU3u1WhfUruNb0m/N5jwxUGqVKcjck8rqwQvThqBMBGhw9YD5EtnehPcoXW3oA0 vmr6ZAd2pcvV/gKr5FFzO+Fvvs+cuYaa+8yieiejKaxewsyL/5gRiZAUx/tFkiHk 3tyTohYntHC34UDtdWiAyXNSiv7l3ZYRwoboW5B96PY2luA0Ml4= =XjHS -----END PGP SIGNATURE----- --jtYT7BoqwN6RdCzF--