Re: [PATCH] tap: fix seq->p.count limit

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  9 Aug 2023 11:23:42 +0200
Laurent Vivier <lvivier@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@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.

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