From: Eric Dumazet <hidden> Date: 2021-12-22 04:32:14
On Tue, Dec 21, 2021 at 1:34 PM Paolo Abeni [off-list ref] wrote:
After commit d3256efd8e8b ("veth: allow enabling NAPI even without XDP"),
if GRO is enabled on a veth device and TSO is disabled on the peer
device, TCP skbs will go through the NAPI callback. If there is no XDP
program attached, the veth code does not perform any share check, and
shared/cloned skbs could enter the GRO engine.
...
quoted hunk
Address the issue checking for cloned skbs even in the GRO-without-XDP
input path.
Reported-and-tested-by: Ignat Korchagin <redacted>
Fixes: d3256efd8e8b ("veth: allow enabling NAPI even without XDP")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
drivers/net/veth.c | 8 ++++++++
1 file changed, 8 insertions(+)
- It seems adding yet memory alloc/free and copies is defeating GRO purpose.
- After skb_copy(), GRO is forced to use the expensive frag_list way
for aggregation anyway.
- veth mtu could be set to 64KB, so we could have order-4 allocation
attempts here.
Would the following fix [1] be better maybe, in terms of efficiency,
and keeping around skb EDT/tstamp
information (see recent thread with Martin and Daniel )
I think it also focuses more on the problem (GRO is not capable of
dealing with cloned skb yet).
Who knows, maybe in the future we will _have_ to add more checks in
GRO fast path for some other reason,
since it is becoming the Swiss army knife of networking :)
Although I guess this whole case (disabling TSO) is moot, I have no
idea why anyone would do that :)
[1]
From: Paolo Abeni <pabeni@redhat.com> Date: 2021-12-22 11:06:39
Hello,
On Tue, 2021-12-21 at 20:31 -0800, Eric Dumazet wrote:
On Tue, Dec 21, 2021 at 1:34 PM Paolo Abeni [off-list ref] wrote:
quoted
After commit d3256efd8e8b ("veth: allow enabling NAPI even without XDP"),
if GRO is enabled on a veth device and TSO is disabled on the peer
device, TCP skbs will go through the NAPI callback. If there is no XDP
program attached, the veth code does not perform any share check, and
shared/cloned skbs could enter the GRO engine.
...
quoted
Address the issue checking for cloned skbs even in the GRO-without-XDP
input path.
Reported-and-tested-by: Ignat Korchagin <redacted>
Fixes: d3256efd8e8b ("veth: allow enabling NAPI even without XDP")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
drivers/net/veth.c | 8 ++++++++
1 file changed, 8 insertions(+)
- It seems adding yet memory alloc/free and copies is defeating GRO purpose.
- After skb_copy(), GRO is forced to use the expensive frag_list way
for aggregation anyway.
- veth mtu could be set to 64KB, so we could have order-4 allocation
attempts here.
Would the following fix [1] be better maybe, in terms of efficiency,
and keeping around skb EDT/tstamp
information (see recent thread with Martin and Daniel )
I think it also focuses more on the problem (GRO is not capable of
dealing with cloned skb yet).
Who knows, maybe in the future we will _have_ to add more checks in
GRO fast path for some other reason,
since it is becoming the Swiss army knife of networking :)
Only vaguely related: I have a bunch of micro optimizations for the GRO
engine. I did not submit the patches because I can observe the gain
only in micro-benchmarks, but I'm wondering if that could be visible
with very high speed TCP stream? I can share the code if that could be
of general interest (after some rebasing, the patches predates gro.c)
quoted hunk
Although I guess this whole case (disabling TSO) is moot, I have no
idea why anyone would do that :)
[1]
@@ -879,8 +879,12 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,stats->xdp_bytes+=skb->len;skb=veth_xdp_rcv_skb(rq,skb,bq,stats);-if(skb)-napi_gro_receive(&rq->xdp_napi,skb);+if(skb){+if(skb_shared(skb)||skb_cloned(skb))+netif_receive_skb(skb);+else+napi_gro_receive(&rq->xdp_napi,skb);+}}done++;}
I tested the above, and it works, too.
I thought about something similar, but I overlooked possible OoO or
behaviour changes when a packet socket is attached to the paired device
(as it would disable GRO).
It looks like tcpdump should have not ill-effects (the mmap rx-path
releases the skb clone before the orig packet reaches the other end),
so I guess the above is fine (and sure is better to avoid more
timestamp related problem).
Do you prefer to submit it formally, or do you prefer I'll send a v2
with the latter code?
Thanks!
Paolo
From: Eric Dumazet <hidden> Date: 2021-12-22 11:58:27
On Wed, Dec 22, 2021 at 3:06 AM Paolo Abeni [off-list ref] wrote:
Hello,
On Tue, 2021-12-21 at 20:31 -0800, Eric Dumazet wrote:
quoted
On Tue, Dec 21, 2021 at 1:34 PM Paolo Abeni [off-list ref] wrote:
quoted
After commit d3256efd8e8b ("veth: allow enabling NAPI even without XDP"),
if GRO is enabled on a veth device and TSO is disabled on the peer
device, TCP skbs will go through the NAPI callback. If there is no XDP
program attached, the veth code does not perform any share check, and
shared/cloned skbs could enter the GRO engine.
...
quoted
Address the issue checking for cloned skbs even in the GRO-without-XDP
input path.
Reported-and-tested-by: Ignat Korchagin <redacted>
Fixes: d3256efd8e8b ("veth: allow enabling NAPI even without XDP")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
drivers/net/veth.c | 8 ++++++++
1 file changed, 8 insertions(+)
- It seems adding yet memory alloc/free and copies is defeating GRO purpose.
- After skb_copy(), GRO is forced to use the expensive frag_list way
for aggregation anyway.
- veth mtu could be set to 64KB, so we could have order-4 allocation
attempts here.
Would the following fix [1] be better maybe, in terms of efficiency,
and keeping around skb EDT/tstamp
information (see recent thread with Martin and Daniel )
I think it also focuses more on the problem (GRO is not capable of
dealing with cloned skb yet).
Who knows, maybe in the future we will _have_ to add more checks in
GRO fast path for some other reason,
since it is becoming the Swiss army knife of networking :)
Only vaguely related: I have a bunch of micro optimizations for the GRO
engine. I did not submit the patches because I can observe the gain
only in micro-benchmarks, but I'm wondering if that could be visible
with very high speed TCP stream? I can share the code if that could be
of general interest (after some rebasing, the patches predates gro.c)
quoted
Although I guess this whole case (disabling TSO) is moot, I have no
idea why anyone would do that :)
[1]
@@ -879,8 +879,12 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,stats->xdp_bytes+=skb->len;skb=veth_xdp_rcv_skb(rq,skb,bq,stats);-if(skb)-napi_gro_receive(&rq->xdp_napi,skb);+if(skb){+if(skb_shared(skb)||skb_cloned(skb))+netif_receive_skb(skb);+else+napi_gro_receive(&rq->xdp_napi,skb);+}}done++;}
I tested the above, and it works, too.
I thought about something similar, but I overlooked possible OoO or
behaviour changes when a packet socket is attached to the paired device
(as it would disable GRO).
Have you tried a pskb_expand_head() instead of a full copy ?
Perhaps that would be enough, and keep all packets going through GRO to
make sure OOO is covered.
It looks like tcpdump should have not ill-effects (the mmap rx-path
releases the skb clone before the orig packet reaches the other end),
so I guess the above is fine (and sure is better to avoid more
timestamp related problem).
Do you prefer to submit it formally, or do you prefer I'll send a v2
with the latter code?
From: Paolo Abeni <pabeni@redhat.com> Date: 2021-12-22 16:10:37
On Wed, 2021-12-22 at 03:58 -0800, Eric Dumazet wrote:
On Wed, Dec 22, 2021 at 3:06 AM Paolo Abeni [off-list ref] wrote:
quoted
I thought about something similar, but I overlooked possible OoO or
behaviour changes when a packet socket is attached to the paired device
(as it would disable GRO).
Have you tried a pskb_expand_head() instead of a full copy ?
Perhaps that would be enough, and keep all packets going through GRO to
make sure OOO is covered.
Indeed it looks like it's enough. I'll do some more testing and I'll
send a v2 using pskb_expand_head().
Many thanks!
Paolo
From: Dave Taht <hidden> Date: 2021-12-23 18:08:21
On Wed, Dec 22, 2021 at 5:17 PM Eric Dumazet [off-list ref] wrote:
On Tue, Dec 21, 2021 at 1:34 PM Paolo Abeni [off-list ref] wrote:
quoted
After commit d3256efd8e8b ("veth: allow enabling NAPI even without XDP"),
if GRO is enabled on a veth device and TSO is disabled on the peer
device, TCP skbs will go through the NAPI callback. If there is no XDP
program attached, the veth code does not perform any share check, and
shared/cloned skbs could enter the GRO engine.
...
quoted
Address the issue checking for cloned skbs even in the GRO-without-XDP
input path.
Reported-and-tested-by: Ignat Korchagin <redacted>
Fixes: d3256efd8e8b ("veth: allow enabling NAPI even without XDP")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
drivers/net/veth.c | 8 ++++++++
1 file changed, 8 insertions(+)
- It seems adding yet memory alloc/free and copies is defeating GRO purpose.
- After skb_copy(), GRO is forced to use the expensive frag_list way
for aggregation anyway.
- veth mtu could be set to 64KB, so we could have order-4 allocation
attempts here.
Would the following fix [1] be better maybe, in terms of efficiency,
and keeping around skb EDT/tstamp
information (see recent thread with Martin and Daniel )
I've always liked the idea of being able to coherently timestamp from
packet ingress to egress.
I think it also focuses more on the problem (GRO is not capable of
dealing with cloned skb yet).
Who knows, maybe in the future we will _have_ to add more checks in
GRO fast path for some other reason,
since it is becoming the Swiss army knife of networking :)
GRO is the bane of my sub-gbit existence. I've been wishing we had a
compile time option to always split it up or a righter path forward
using veth interfaces here: https://github.com/rchac/LibreQoS
quoted hunk
Although I guess this whole case (disabling TSO) is moot, I have no
idea why anyone would do that :)
[1]