On Wed, Aug 09, 2023 at 11:23:42AM +0200, Laurent Vivier wrote:
...
  The number of items in pool_l4_t is defined to
UIO_MAXIOV,
 not TAP_SEQS. TAP_SEQS is the number of the messages.
 
 Fix the value used to compare seq->p.count with.
 
 Fix: bb708111833e ("treewide: Packet abstraction with mandatory boundary
checks")
 Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> 
Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>

...
  ---
  tap.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/tap.c b/tap.c
 index e034f9468267..69bd19a2a91a 100644
 --- a/tap.c
 +++ b/tap.c
 @@ -678,7 +678,7 @@ resume:
  		seq->daddr.s_addr	= iph->daddr;			\
  	} while (0)
  
 -		if (seq && L4_MATCH(iph, uh, seq) && seq->p.count < TAP_SEQS)
 +		if (seq && L4_MATCH(iph, uh, seq) && seq->p.count < UIO_MAXIOV)
  			goto append;
  
  		if (seq_count == TAP_SEQS)
 @@ -686,7 +686,7 @@ resume:
  
  		for (seq = tap4_l4 + seq_count - 1; seq >= tap4_l4; seq--) {
  			if (L4_MATCH(iph, uh, seq)) {
 -				if (seq->p.count >= TAP_SEQS)
 +				if (seq->p.count >= UIO_MAXIOV)
  					seq = NULL;
  				break;
  			}
 @@ -840,7 +840,7 @@ resume:
  	} while (0)
  
  		if (seq && L4_MATCH(ip6h, proto, uh, seq) &&
 -		    seq->p.count < TAP_SEQS)
 +		    seq->p.count < UIO_MAXIOV)
  			goto append;
  
  		if (seq_count == TAP_SEQS)
 @@ -848,7 +848,7 @@ resume:
  
  		for (seq = tap6_l4 + seq_count - 1; seq >= tap6_l4; seq--) {
  			if (L4_MATCH(ip6h, proto, uh, seq)) {
 -				if (seq->p.count >= TAP_SEQS)
 +				if (seq->p.count >= UIO_MAXIOV)
  					seq = NULL;
  				break;
  			} 
-- 
David Gibson			| 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

On Wed,  9 Aug 2023 11:23:42 +0200
Laurent Vivier <lvivier(a)redhat.com> wrote:

...
  The number of items in pool_l4_t is defined to
UIO_MAXIOV,
 not TAP_SEQS. TAP_SEQS is the number of the messages. 
...sequences of packets (within the same connection), rather than
"messages" (which might sound like packets).

...
  Fix the value used to compare seq->p.count with.
 
 Fix: bb708111833e ("treewide: Packet abstraction with mandatory boundary
checks")
 Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> 
I was wondering why throughput tests on the tap path, namespace to
host, started failing miserably with this (0.4 Gbps instead of 7 Gbps
with small packets).

It turned out that yes, the pool has UIO_MAXIOV items, but (also by
mistake in some sense) we initialised only the first TAP_SEQS ones. This
fixes it:

diff --git a/tap.c b/tap.c
index 7d5dd6a..a6f8692 100644
--- a/tap.c
+++ b/tap.c
@@ -1258,8 +1258,8 @@ void tap_sock_init(struct ctx *c)
        pool_tap6_storage = PACKET_INIT(pool_tap6, TAP_MSGS, pkt_buf, sz);
 
        for (i = 0; i < TAP_SEQS; i++) {
-               tap4_l4[i].p = PACKET_INIT(pool_l4, TAP_SEQS, pkt_buf, sz);
-               tap6_l4[i].p = PACKET_INIT(pool_l4, TAP_SEQS, pkt_buf, sz);
+               tap4_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, pkt_buf, sz);
+               tap6_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, pkt_buf, sz);
        }
 
        if (c->fd_tap != -1) { /* Passed as --fd */

...I would simply apply it on top.

-- 
Stefano