* [PATCH] tap: fix seq->p.count limit
@ 2023-08-09 9:23 Laurent Vivier
2023-08-09 9:54 ` David Gibson
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Laurent Vivier @ 2023-08-09 9:23 UTC (permalink / raw)
To: passt-dev; +Cc: Laurent Vivier
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@redhat.com>
---
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;
}
--
@@ -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;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] tap: fix seq->p.count limit
2023-08-09 9:23 [PATCH] tap: fix seq->p.count limit Laurent Vivier
@ 2023-08-09 9:54 ` David Gibson
2023-08-09 15:44 ` Laurent Vivier
2023-08-13 13:17 ` Stefano Brivio
2 siblings, 0 replies; 5+ messages in thread
From: David Gibson @ 2023-08-09 9:54 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]
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@redhat.com>
Reviewed-by: David Gibson <david@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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] tap: fix seq->p.count limit
2023-08-09 9:23 [PATCH] tap: fix seq->p.count limit Laurent Vivier
2023-08-09 9:54 ` David Gibson
@ 2023-08-09 15:44 ` Laurent Vivier
2023-08-13 13:17 ` Stefano Brivio
2 siblings, 0 replies; 5+ messages in thread
From: Laurent Vivier @ 2023-08-09 15:44 UTC (permalink / raw)
To: passt-dev
On 8/9/23 11:23, 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")
In fact, it actually fixes:
Fix: 37c228ada88b ("tap, tcp, udp, icmp: Cut down on some oversized buffers")
that replaces UIO_MAXIOV by TAP_SEQS in the array declaration and didn't correclty update
the code.
Thanks,
Laurent
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> 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;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] tap: fix seq->p.count limit
2023-08-09 9:23 [PATCH] tap: fix seq->p.count limit Laurent Vivier
2023-08-09 9:54 ` David Gibson
2023-08-09 15:44 ` Laurent Vivier
@ 2023-08-13 13:17 ` Stefano Brivio
2023-08-13 16:31 ` Stefano Brivio
2 siblings, 1 reply; 5+ messages in thread
From: Stefano Brivio @ 2023-08-13 13:17 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev, David Gibson
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] tap: fix seq->p.count limit
2023-08-13 13:17 ` Stefano Brivio
@ 2023-08-13 16:31 ` Stefano Brivio
0 siblings, 0 replies; 5+ messages in thread
From: Stefano Brivio @ 2023-08-13 16:31 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev, David Gibson
On Sun, 13 Aug 2023 15:17:37 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:
> 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.
Applied (with this change), thanks!
--
Stefano
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-13 16:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-09 9:23 [PATCH] tap: fix seq->p.count limit Laurent Vivier
2023-08-09 9:54 ` David Gibson
2023-08-09 15:44 ` Laurent Vivier
2023-08-13 13:17 ` Stefano Brivio
2023-08-13 16:31 ` Stefano Brivio
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).