Re: [PATCH v8 30/30] packet: Add support for multi-vector packets

15 Aug 2025

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.
This patch extends the packet pool to support iovec array,
allowing a single logical packet to be composed of multiple iovec.
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 = NULL and iov_len holding the number of subsequent
vectors. The actual data vectors are then stored in the following
pool slots.
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.
Signed-off-by: Laurent Vivier 
---
  packet.c | 50 +++++++++++++++++++++++++++++++++-----------------
  packet.h |  2 +-
  tap.c    |  4 ++--
  3 files changed, 36 insertions(+), 20 deletions(-)

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 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.
okay
...
...
{
-	return p->count >= p->size;
+	return p->count + data->cnt  + (data->cnt > 1) >= 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.
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==99,
p->size==100.  We attempt to add a single buffer packet, data->cnt==1.

	99 + 1 + 0 >= 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 = p->count;
-	const char *start;
-	size_t len;
+	size_t idx = 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_tail *data,
  if (!iov_tail_prune(data))
      return;
-	ASSERT(data->cnt == 1); /* we don't support iovec */
+	if (data->cnt > 1) {
+		p->pkt[idx].iov_base = NULL;
+		p->pkt[idx].iov_len = data->cnt;
+		idx++;
+	}
-	len = data->iov[0].iov_len - data->off;
-	start = (char *)data->iov[0].iov_base + data->off;
+	offset = data->off;
+	for (i = 0; i < data->cnt; i++) {
+		const char *start;
+		size_t len;
-	if (packet_check_range(p, start, len, func, line))
-		return;
+		len = data->iov[i].iov_len - offset;
+		start = (char *)data->iov[i].iov_base + offset;
+		offset = 0;
-	p->pkt[idx].iov_base = (void *)start;
-	p->pkt[idx].iov_len = len;
+		if (packet_check_range(p, start, len, func, line))
+			return;
-	p->count++;
+		p->pkt[idx].iov_base = (void *)start;
+		p->pkt[idx].iov_len = 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?
Yes, I think the code is clearer like this. And it avoids to scan the iovec
array twice (with the offset management).
Ok.

-- 
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

    