* [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs
@ 2025-01-09 14:27 Thomas Bogendoerfer
2025-01-09 14:56 ` Eric Dumazet
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Bogendoerfer @ 2025-01-09 14:27 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, netdev, linux-kernel
gro_cells_receive() passes a cloned skb directly up the stack and
could cause re-ordering against segments still in GRO. To avoid
this copy the skb and let GRO do it's work.
Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure")
Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
net/core/gro_cells.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
index ff8e5b64bf6b..2f8d688f9d82 100644
--- a/net/core/gro_cells.c
+++ b/net/core/gro_cells.c
@@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb)
if (unlikely(!(dev->flags & IFF_UP)))
goto drop;
- if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
+ if (!gcells->cells || netif_elide_gro(dev)) {
+netif_rx:
res = netif_rx(skb);
goto unlock;
}
+ if (skb_cloned(skb)) {
+ struct sk_buff *n;
+ n = skb_copy(skb, GFP_KERNEL);
+ if (!n)
+ goto netif_rx;
+ kfree_skb(skb);
+ skb = n;
+ }
cell = this_cpu_ptr(gcells->cells);
if (skb_queue_len(&cell->napi_skbs) > READ_ONCE(net_hotdata.max_backlog)) {
--
2.35.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs 2025-01-09 14:27 [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs Thomas Bogendoerfer @ 2025-01-09 14:56 ` Eric Dumazet 2025-01-13 11:28 ` Thomas Bogendoerfer 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2025-01-09 14:56 UTC (permalink / raw) To: Thomas Bogendoerfer Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel On Thu, Jan 9, 2025 at 3:27 PM Thomas Bogendoerfer <tbogendoerfer@suse.de> wrote: > > gro_cells_receive() passes a cloned skb directly up the stack and > could cause re-ordering against segments still in GRO. To avoid > this copy the skb and let GRO do it's work. > > Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure") > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > --- > net/core/gro_cells.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > index ff8e5b64bf6b..2f8d688f9d82 100644 > --- a/net/core/gro_cells.c > +++ b/net/core/gro_cells.c > @@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) > if (unlikely(!(dev->flags & IFF_UP))) > goto drop; > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { > + if (!gcells->cells || netif_elide_gro(dev)) { > +netif_rx: > res = netif_rx(skb); > goto unlock; > } > + if (skb_cloned(skb)) { > + struct sk_buff *n; > > + n = skb_copy(skb, GFP_KERNEL); I do not think we want this skb_copy(). This is going to fail too often. Can you remind us why we have this skb_cloned() check here ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs 2025-01-09 14:56 ` Eric Dumazet @ 2025-01-13 11:28 ` Thomas Bogendoerfer 2025-01-13 12:55 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Thomas Bogendoerfer @ 2025-01-13 11:28 UTC (permalink / raw) To: Eric Dumazet Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel On Thu, 9 Jan 2025 15:56:24 +0100 Eric Dumazet <edumazet@google.com> wrote: > On Thu, Jan 9, 2025 at 3:27 PM Thomas Bogendoerfer > <tbogendoerfer@suse.de> wrote: > > > > gro_cells_receive() passes a cloned skb directly up the stack and > > could cause re-ordering against segments still in GRO. To avoid > > this copy the skb and let GRO do it's work. > > > > Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure") > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > > --- > > net/core/gro_cells.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > > index ff8e5b64bf6b..2f8d688f9d82 100644 > > --- a/net/core/gro_cells.c > > +++ b/net/core/gro_cells.c > > @@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) > > if (unlikely(!(dev->flags & IFF_UP))) > > goto drop; > > > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { > > + if (!gcells->cells || netif_elide_gro(dev)) { > > +netif_rx: > > res = netif_rx(skb); > > goto unlock; > > } > > + if (skb_cloned(skb)) { > > + struct sk_buff *n; > > > > + n = skb_copy(skb, GFP_KERNEL); > > I do not think we want this skb_copy(). This is going to fail too often. ok > Can you remind us why we have this skb_cloned() check here ? some fields of the ip/tcp header are going to be changed in the first gro segment Thomas. -- SUSE Software Solutions Germany GmbH HRB 36809 (AG Nürnberg) Geschäftsführer: Ivo Totev, Andrew McDonald, Werner Knoblich ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs 2025-01-13 11:28 ` Thomas Bogendoerfer @ 2025-01-13 12:55 ` Eric Dumazet 2025-01-13 13:33 ` Alexander Lobakin 2025-01-20 14:31 ` Thomas Bogendoerfer 0 siblings, 2 replies; 8+ messages in thread From: Eric Dumazet @ 2025-01-13 12:55 UTC (permalink / raw) To: Thomas Bogendoerfer Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel On Mon, Jan 13, 2025 at 12:28 PM Thomas Bogendoerfer <tbogendoerfer@suse.de> wrote: > > On Thu, 9 Jan 2025 15:56:24 +0100 > Eric Dumazet <edumazet@google.com> wrote: > > > On Thu, Jan 9, 2025 at 3:27 PM Thomas Bogendoerfer > > <tbogendoerfer@suse.de> wrote: > > > > > > gro_cells_receive() passes a cloned skb directly up the stack and > > > could cause re-ordering against segments still in GRO. To avoid > > > this copy the skb and let GRO do it's work. > > > > > > Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure") > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > > > --- > > > net/core/gro_cells.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > > > index ff8e5b64bf6b..2f8d688f9d82 100644 > > > --- a/net/core/gro_cells.c > > > +++ b/net/core/gro_cells.c > > > @@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) > > > if (unlikely(!(dev->flags & IFF_UP))) > > > goto drop; > > > > > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { > > > + if (!gcells->cells || netif_elide_gro(dev)) { > > > +netif_rx: > > > res = netif_rx(skb); > > > goto unlock; > > > } > > > + if (skb_cloned(skb)) { > > > + struct sk_buff *n; > > > > > > + n = skb_copy(skb, GFP_KERNEL); > > > > I do not think we want this skb_copy(). This is going to fail too often. > > ok > > > Can you remind us why we have this skb_cloned() check here ? > > some fields of the ip/tcp header are going to be changed in the first gro > segment Presumably we should test skb_header_cloned() This means something like skb_cow_head(skb, 0) could be much more reasonable than skb_copy(). diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c index ff8e5b64bf6b76451a69e3eae132b593c60ee204..bd8966484da3fe85d1d87bf847d3730d7ad094e5 100644 --- a/net/core/gro_cells.c +++ b/net/core/gro_cells.c @@ -20,7 +20,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) if (unlikely(!(dev->flags & IFF_UP))) goto drop; - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { + if (!gcells->cells || netif_elide_gro(dev) || skb_cow_head(skb, 0)) { res = netif_rx(skb); goto unlock; } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs 2025-01-13 12:55 ` Eric Dumazet @ 2025-01-13 13:33 ` Alexander Lobakin 2025-01-20 14:31 ` Thomas Bogendoerfer 1 sibling, 0 replies; 8+ messages in thread From: Alexander Lobakin @ 2025-01-13 13:33 UTC (permalink / raw) To: Eric Dumazet, Thomas Bogendoerfer Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel From: Eric Dumazet <edumazet@google.com> Date: Mon, 13 Jan 2025 13:55:18 +0100 > On Mon, Jan 13, 2025 at 12:28 PM Thomas Bogendoerfer > <tbogendoerfer@suse.de> wrote: >> >> On Thu, 9 Jan 2025 15:56:24 +0100 >> Eric Dumazet <edumazet@google.com> wrote: >> >>> On Thu, Jan 9, 2025 at 3:27 PM Thomas Bogendoerfer >>> <tbogendoerfer@suse.de> wrote: >>>> >>>> gro_cells_receive() passes a cloned skb directly up the stack and >>>> could cause re-ordering against segments still in GRO. To avoid >>>> this copy the skb and let GRO do it's work. >>>> >>>> Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure") >>>> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> >>>> --- >>>> net/core/gro_cells.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c >>>> index ff8e5b64bf6b..2f8d688f9d82 100644 >>>> --- a/net/core/gro_cells.c >>>> +++ b/net/core/gro_cells.c >>>> @@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) >>>> if (unlikely(!(dev->flags & IFF_UP))) >>>> goto drop; >>>> >>>> - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { >>>> + if (!gcells->cells || netif_elide_gro(dev)) { >>>> +netif_rx: >>>> res = netif_rx(skb); >>>> goto unlock; >>>> } >>>> + if (skb_cloned(skb)) { >>>> + struct sk_buff *n; >>>> >>>> + n = skb_copy(skb, GFP_KERNEL); >>> >>> I do not think we want this skb_copy(). This is going to fail too often. >> >> ok >> >>> Can you remind us why we have this skb_cloned() check here ? >> >> some fields of the ip/tcp header are going to be changed in the first gro >> segment > > Presumably we should test skb_header_cloned() > > This means something like skb_cow_head(skb, 0) could be much more > reasonable than skb_copy(). Maybe skb_try_make_writable() would fit? > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > index ff8e5b64bf6b76451a69e3eae132b593c60ee204..bd8966484da3fe85d1d87bf847d3730d7ad094e5 > 100644 > --- a/net/core/gro_cells.c > +++ b/net/core/gro_cells.c > @@ -20,7 +20,7 @@ int gro_cells_receive(struct gro_cells *gcells, > struct sk_buff *skb) > if (unlikely(!(dev->flags & IFF_UP))) > goto drop; > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { > + if (!gcells->cells || netif_elide_gro(dev) || skb_cow_head(skb, 0)) { > res = netif_rx(skb); > goto unlock; > } Thanks, Olek ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs 2025-01-13 12:55 ` Eric Dumazet 2025-01-13 13:33 ` Alexander Lobakin @ 2025-01-20 14:31 ` Thomas Bogendoerfer 2025-01-20 14:55 ` Eric Dumazet 1 sibling, 1 reply; 8+ messages in thread From: Thomas Bogendoerfer @ 2025-01-20 14:31 UTC (permalink / raw) To: Eric Dumazet Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel On Mon, 13 Jan 2025 13:55:18 +0100 Eric Dumazet <edumazet@google.com> wrote: > On Mon, Jan 13, 2025 at 12:28 PM Thomas Bogendoerfer > <tbogendoerfer@suse.de> wrote: > > > > On Thu, 9 Jan 2025 15:56:24 +0100 > > Eric Dumazet <edumazet@google.com> wrote: > > > > > On Thu, Jan 9, 2025 at 3:27 PM Thomas Bogendoerfer > > > <tbogendoerfer@suse.de> wrote: > > > > > > > > gro_cells_receive() passes a cloned skb directly up the stack and > > > > could cause re-ordering against segments still in GRO. To avoid > > > > this copy the skb and let GRO do it's work. > > > > > > > > Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure") > > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > > > > --- > > > > net/core/gro_cells.c | 11 ++++++++++- > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > > > > index ff8e5b64bf6b..2f8d688f9d82 100644 > > > > --- a/net/core/gro_cells.c > > > > +++ b/net/core/gro_cells.c > > > > @@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) > > > > if (unlikely(!(dev->flags & IFF_UP))) > > > > goto drop; > > > > > > > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { > > > > + if (!gcells->cells || netif_elide_gro(dev)) { > > > > +netif_rx: > > > > res = netif_rx(skb); > > > > goto unlock; > > > > } > > > > + if (skb_cloned(skb)) { > > > > + struct sk_buff *n; > > > > > > > > + n = skb_copy(skb, GFP_KERNEL); > > > > > > I do not think we want this skb_copy(). This is going to fail too often. > > > > ok > > > > > Can you remind us why we have this skb_cloned() check here ? > > > > some fields of the ip/tcp header are going to be changed in the first gro > > segment > > Presumably we should test skb_header_cloned() > > This means something like skb_cow_head(skb, 0) could be much more > reasonable than skb_copy(). I don't think this will work, because at that point it's skb->data points at the IPv6 header in my test case (traffic between two namespaces connected via ip6 tunnel over ipvlan). Correct header offsets are set after later, when gro_cells napi routine runs. Do you see another option ? Thomas. -- SUSE Software Solutions Germany GmbH HRB 36809 (AG Nürnberg) Geschäftsführer: Ivo Totev, Andrew McDonald, Werner Knoblich ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs 2025-01-20 14:31 ` Thomas Bogendoerfer @ 2025-01-20 14:55 ` Eric Dumazet 2025-01-21 11:51 ` Thomas Bogendoerfer 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2025-01-20 14:55 UTC (permalink / raw) To: Thomas Bogendoerfer Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel On Mon, Jan 20, 2025 at 3:32 PM Thomas Bogendoerfer <tbogendoerfer@suse.de> wrote: > > On Mon, 13 Jan 2025 13:55:18 +0100 > Eric Dumazet <edumazet@google.com> wrote: > > > On Mon, Jan 13, 2025 at 12:28 PM Thomas Bogendoerfer > > <tbogendoerfer@suse.de> wrote: > > > > > > On Thu, 9 Jan 2025 15:56:24 +0100 > > > Eric Dumazet <edumazet@google.com> wrote: > > > > > > > On Thu, Jan 9, 2025 at 3:27 PM Thomas Bogendoerfer > > > > <tbogendoerfer@suse.de> wrote: > > > > > > > > > > gro_cells_receive() passes a cloned skb directly up the stack and > > > > > could cause re-ordering against segments still in GRO. To avoid > > > > > this copy the skb and let GRO do it's work. > > > > > > > > > > Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure") > > > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > > > > > --- > > > > > net/core/gro_cells.c | 11 ++++++++++- > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > > > > > index ff8e5b64bf6b..2f8d688f9d82 100644 > > > > > --- a/net/core/gro_cells.c > > > > > +++ b/net/core/gro_cells.c > > > > > @@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) > > > > > if (unlikely(!(dev->flags & IFF_UP))) > > > > > goto drop; > > > > > > > > > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { > > > > > + if (!gcells->cells || netif_elide_gro(dev)) { > > > > > +netif_rx: > > > > > res = netif_rx(skb); > > > > > goto unlock; > > > > > } > > > > > + if (skb_cloned(skb)) { > > > > > + struct sk_buff *n; > > > > > > > > > > + n = skb_copy(skb, GFP_KERNEL); > > > > > > > > I do not think we want this skb_copy(). This is going to fail too often. > > > > > > ok > > > > > > > Can you remind us why we have this skb_cloned() check here ? > > > > > > some fields of the ip/tcp header are going to be changed in the first gro > > > segment > > > > Presumably we should test skb_header_cloned() > > > > This means something like skb_cow_head(skb, 0) could be much more > > reasonable than skb_copy(). > > I don't think this will work, because at that point it's skb->data points > at the IPv6 header in my test case (traffic between two namespaces connected > via ip6 tunnel over ipvlan). Correct header offsets are set after later, > when gro_cells napi routine runs. > > Do you see another option ? Anything not attempting order-5 allocations will work :) I would try something like that. diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c index ff8e5b64bf6b..74416194f148 100644 --- a/net/core/gro_cells.c +++ b/net/core/gro_cells.c @@ -4,6 +4,7 @@ #include <linux/netdevice.h> #include <net/gro_cells.h> #include <net/hotdata.h> +#include <net/gro.h> struct gro_cell { struct sk_buff_head napi_skbs; @@ -20,7 +21,7 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) if (unlikely(!(dev->flags & IFF_UP))) goto drop; - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { + if (!gcells->cells || netif_elide_gro(dev)) { res = netif_rx(skb); goto unlock; } @@ -58,7 +59,11 @@ static int gro_cell_poll(struct napi_struct *napi, int budget) skb = __skb_dequeue(&cell->napi_skbs); if (!skb) break; - napi_gro_receive(napi, skb); + /* Core GRO stack does not play well with clones. */ + if (skb_cloned(skb)) + gro_normal_one(napi, skb, 1); + else + napi_gro_receive(napi, skb); work_done++; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs 2025-01-20 14:55 ` Eric Dumazet @ 2025-01-21 11:51 ` Thomas Bogendoerfer 0 siblings, 0 replies; 8+ messages in thread From: Thomas Bogendoerfer @ 2025-01-21 11:51 UTC (permalink / raw) To: Eric Dumazet Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel On Mon, 20 Jan 2025 15:55:26 +0100 Eric Dumazet <edumazet@google.com> wrote: > On Mon, Jan 20, 2025 at 3:32 PM Thomas Bogendoerfer > <tbogendoerfer@suse.de> wrote: > > > > On Mon, 13 Jan 2025 13:55:18 +0100 > > Eric Dumazet <edumazet@google.com> wrote: > > > > > On Mon, Jan 13, 2025 at 12:28 PM Thomas Bogendoerfer > > > <tbogendoerfer@suse.de> wrote: > > > > > > > > On Thu, 9 Jan 2025 15:56:24 +0100 > > > > Eric Dumazet <edumazet@google.com> wrote: > > > > > > > > > On Thu, Jan 9, 2025 at 3:27 PM Thomas Bogendoerfer > > > > > <tbogendoerfer@suse.de> wrote: > > > > > > > > > > > > gro_cells_receive() passes a cloned skb directly up the stack and > > > > > > could cause re-ordering against segments still in GRO. To avoid > > > > > > this copy the skb and let GRO do it's work. > > > > > > > > > > > > Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure") > > > > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > > > > > > --- > > > > > > net/core/gro_cells.c | 11 ++++++++++- > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > > > > > > index ff8e5b64bf6b..2f8d688f9d82 100644 > > > > > > --- a/net/core/gro_cells.c > > > > > > +++ b/net/core/gro_cells.c > > > > > > @@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) > > > > > > if (unlikely(!(dev->flags & IFF_UP))) > > > > > > goto drop; > > > > > > > > > > > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { > > > > > > + if (!gcells->cells || netif_elide_gro(dev)) { > > > > > > +netif_rx: > > > > > > res = netif_rx(skb); > > > > > > goto unlock; > > > > > > } > > > > > > + if (skb_cloned(skb)) { > > > > > > + struct sk_buff *n; > > > > > > > > > > > > + n = skb_copy(skb, GFP_KERNEL); > > > > > > > > > > I do not think we want this skb_copy(). This is going to fail too often. > > > > > > > > ok > > > > > > > > > Can you remind us why we have this skb_cloned() check here ? > > > > > > > > some fields of the ip/tcp header are going to be changed in the first gro > > > > segment > > > > > > Presumably we should test skb_header_cloned() > > > > > > This means something like skb_cow_head(skb, 0) could be much more > > > reasonable than skb_copy(). > > > > I don't think this will work, because at that point it's skb->data points > > at the IPv6 header in my test case (traffic between two namespaces connected > > via ip6 tunnel over ipvlan). Correct header offsets are set after later, > > when gro_cells napi routine runs. > > > > Do you see another option ? > > Anything not attempting order-5 allocations will work :) > > I would try something like that. > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c > index ff8e5b64bf6b..74416194f148 100644 > --- a/net/core/gro_cells.c > +++ b/net/core/gro_cells.c > @@ -4,6 +4,7 @@ > #include <linux/netdevice.h> > #include <net/gro_cells.h> > #include <net/hotdata.h> > +#include <net/gro.h> > > struct gro_cell { > struct sk_buff_head napi_skbs; > @@ -20,7 +21,7 @@ int gro_cells_receive(struct gro_cells *gcells, > struct sk_buff *skb) > if (unlikely(!(dev->flags & IFF_UP))) > goto drop; > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) { > + if (!gcells->cells || netif_elide_gro(dev)) { > res = netif_rx(skb); > goto unlock; > } > @@ -58,7 +59,11 @@ static int gro_cell_poll(struct napi_struct *napi, > int budget) > skb = __skb_dequeue(&cell->napi_skbs); > if (!skb) > break; > - napi_gro_receive(napi, skb); > + /* Core GRO stack does not play well with clones. */ > + if (skb_cloned(skb)) > + gro_normal_one(napi, skb, 1); > + else > + napi_gro_receive(napi, skb); > work_done++; > } works perfectly, thank you. I've sent a v2 of the fix. Thomas. -- SUSE Software Solutions Germany GmbH HRB 36809 (AG Nürnberg) Geschäftsführer: Ivo Totev, Andrew McDonald, Werner Knoblich ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-01-21 11:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-09 14:27 [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs Thomas Bogendoerfer 2025-01-09 14:56 ` Eric Dumazet 2025-01-13 11:28 ` Thomas Bogendoerfer 2025-01-13 12:55 ` Eric Dumazet 2025-01-13 13:33 ` Alexander Lobakin 2025-01-20 14:31 ` Thomas Bogendoerfer 2025-01-20 14:55 ` Eric Dumazet 2025-01-21 11:51 ` Thomas Bogendoerfer
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 NNTP newsgroup(s).