* [PATCH net-next] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets
@ 2021-01-12 21:16 Alexander Lobakin
2021-01-13 3:10 ` Alexander Duyck
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Lobakin @ 2021-01-12 21:16 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski
Cc: Eric Dumazet, Edward Cree, Willem de Bruijn, Steffen Klassert,
Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev, linux-kernel,
Alexander Lobakin
Commit 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") actually
not only added a support for fraglisted UDP GRO, but also tweaked
some logics the way that non-fraglisted UDP GRO started to work for
forwarding too.
Tests showed that currently forwarding and NATing of plain UDP GRO
packets are performed fully correctly, regardless if the target
netdevice has a support for hardware/driver GSO UDP L4 or not.
Add the last element and allow to form plain UDP GRO packets if
there is no socket -> we are on forwarding path.
Plain UDP GRO forwarding even shows better performance than fraglisted
UDP GRO in some cases due to not wasting one skbuff_head per every
segment.
Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
net/ipv4/udp_offload.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index ff39e94781bf..9d71df3d52ce 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -460,12 +460,13 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
- if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {
+ if (!sk || (sk && udp_sk(sk)->gro_enabled) ||
+ NAPI_GRO_CB(skb)->is_flist) {
pp = call_gro_receive(udp_gro_receive_segment, head, skb);
return pp;
}
- if (!sk || NAPI_GRO_CB(skb)->encap_mark ||
+ if (NAPI_GRO_CB(skb)->encap_mark ||
(skb->ip_summed != CHECKSUM_PARTIAL &&
NAPI_GRO_CB(skb)->csum_cnt == 0 &&
!NAPI_GRO_CB(skb)->csum_valid) ||
--
2.30.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets
2021-01-12 21:16 [PATCH net-next] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets Alexander Lobakin
@ 2021-01-13 3:10 ` Alexander Duyck
2021-01-13 3:59 ` Dongseok Yi
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Duyck @ 2021-01-13 3:10 UTC (permalink / raw)
To: Alexander Lobakin, David S. Miller, Jakub Kicinski
Cc: Eric Dumazet, Edward Cree, Willem de Bruijn, Steffen Klassert,
Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev, linux-kernel
On 1/12/21 1:16 PM, Alexander Lobakin wrote:
> Commit 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") actually
> not only added a support for fraglisted UDP GRO, but also tweaked
> some logics the way that non-fraglisted UDP GRO started to work for
> forwarding too.
> Tests showed that currently forwarding and NATing of plain UDP GRO
> packets are performed fully correctly, regardless if the target
> netdevice has a support for hardware/driver GSO UDP L4 or not.
> Add the last element and allow to form plain UDP GRO packets if
> there is no socket -> we are on forwarding path.
>
> Plain UDP GRO forwarding even shows better performance than fraglisted
> UDP GRO in some cases due to not wasting one skbuff_head per every
> segment.
>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> ---
> net/ipv4/udp_offload.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index ff39e94781bf..9d71df3d52ce 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -460,12 +460,13 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
> if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
>
> - if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {
> + if (!sk || (sk && udp_sk(sk)->gro_enabled) ||
> + NAPI_GRO_CB(skb)->is_flist) {
> pp = call_gro_receive(udp_gro_receive_segment, head, skb);
> return pp;
> }
>
The second check for sk in "(sk && udp_sk(sk)->gro_enabled)" is
redundant and can be dropped. You already verified it is present when
you checked for !sk before the logical OR.
> - if (!sk || NAPI_GRO_CB(skb)->encap_mark ||
> + if (NAPI_GRO_CB(skb)->encap_mark ||
> (skb->ip_summed != CHECKSUM_PARTIAL &&
> NAPI_GRO_CB(skb)->csum_cnt == 0 &&
> !NAPI_GRO_CB(skb)->csum_valid) ||
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH net-next] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets
2021-01-13 3:10 ` Alexander Duyck
@ 2021-01-13 3:59 ` Dongseok Yi
2021-01-13 10:05 ` Alexander Lobakin
0 siblings, 1 reply; 5+ messages in thread
From: Dongseok Yi @ 2021-01-13 3:59 UTC (permalink / raw)
To: 'Alexander Duyck', 'Alexander Lobakin',
'David S. Miller', 'Jakub Kicinski'
Cc: 'Eric Dumazet', 'Edward Cree',
'Willem de Bruijn', 'Steffen Klassert',
'Alexey Kuznetsov', 'Hideaki YOSHIFUJI',
netdev, linux-kernel
On 2021-01-13 12:10, Alexander Duyck wrote:
> On 1/12/21 1:16 PM, Alexander Lobakin wrote:
> > Commit 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") actually
> > not only added a support for fraglisted UDP GRO, but also tweaked
> > some logics the way that non-fraglisted UDP GRO started to work for
> > forwarding too.
> > Tests showed that currently forwarding and NATing of plain UDP GRO
> > packets are performed fully correctly, regardless if the target
> > netdevice has a support for hardware/driver GSO UDP L4 or not.
> > Add the last element and allow to form plain UDP GRO packets if
> > there is no socket -> we are on forwarding path.
> >
Your patch is very similar with the RFC what I submitted but has
different approach. My concern was NAT forwarding.
https://lore.kernel.org/patchwork/patch/1362257/
Nevertheless, I agreed with your idea that allow fraglisted UDP GRO
if there is socket.
> > Plain UDP GRO forwarding even shows better performance than fraglisted
> > UDP GRO in some cases due to not wasting one skbuff_head per every
> > segment.
> >
> > Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> > ---
> > net/ipv4/udp_offload.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index ff39e94781bf..9d71df3d52ce 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -460,12 +460,13 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
> > if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> > NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
is_flist can be true even if !sk.
> >
> > - if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {
> > + if (!sk || (sk && udp_sk(sk)->gro_enabled) ||
Actually sk would be NULL by udp_encap_needed_key in udp4_gro_receive
or udp6_gro_receive.
> > + NAPI_GRO_CB(skb)->is_flist) {
> > pp = call_gro_receive(udp_gro_receive_segment, head, skb);
udp_gro_receive_segment will check is_flist first and try to do
fraglisted UDP GRO. Can you check what I'm missing?
> > return pp;
> > }
> >
>
> The second check for sk in "(sk && udp_sk(sk)->gro_enabled)" is
> redundant and can be dropped. You already verified it is present when
> you checked for !sk before the logical OR.
>
Sorry, Alexander Duyck. I believe Alexander Lobakin will answer this.
> > - if (!sk || NAPI_GRO_CB(skb)->encap_mark ||
> > + if (NAPI_GRO_CB(skb)->encap_mark ||
> > (skb->ip_summed != CHECKSUM_PARTIAL &&
> > NAPI_GRO_CB(skb)->csum_cnt == 0 &&
> > !NAPI_GRO_CB(skb)->csum_valid) ||
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH net-next] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets
2021-01-13 3:59 ` Dongseok Yi
@ 2021-01-13 10:05 ` Alexander Lobakin
0 siblings, 0 replies; 5+ messages in thread
From: Alexander Lobakin @ 2021-01-13 10:05 UTC (permalink / raw)
To: Dongseok Yi
Cc: 'Alexander Lobakin', 'Alexander Duyck',
'David S. Miller', 'Jakub Kicinski',
'Eric Dumazet', 'Edward Cree',
'Willem de Bruijn', 'Steffen Klassert',
'Alexey Kuznetsov', 'Hideaki YOSHIFUJI',
netdev, linux-kernel
From: "'Alexander Lobakin'" <alobakin@pm.me>
From: Dongseok Yi" <dseok.yi@samsung.com>
Date: Wed, 13 Jan 2021 12:59:45 +0900
> On 2021-01-13 12:10, Alexander Duyck wrote:
>> On 1/12/21 1:16 PM, Alexander Lobakin wrote:
>>> Commit 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") actually
>>> not only added a support for fraglisted UDP GRO, but also tweaked
>>> some logics the way that non-fraglisted UDP GRO started to work for
>>> forwarding too.
>>> Tests showed that currently forwarding and NATing of plain UDP GRO
>>> packets are performed fully correctly, regardless if the target
>>> netdevice has a support for hardware/driver GSO UDP L4 or not.
>>> Add the last element and allow to form plain UDP GRO packets if
>>> there is no socket -> we are on forwarding path.
>>>
>
> Your patch is very similar with the RFC what I submitted but has
> different approach. My concern was NAT forwarding.
> https://lore.kernel.org/patchwork/patch/1362257/
Not really. You tried to forbid forwarding of fraglisted UDP GRO
packets, I allow forwarding of plain UDP GRO packets.
With my patch on, you can switch between plain and fraglisted UDP GRO
with the command:
ethtool -K eth0 rx-gro-list on/off
> Nevertheless, I agreed with your idea that allow fraglisted UDP GRO
> if there is socket.
Recheck my change. There's ||, not &&.
As I said in the commit message, forwarding and NATing of plain
UDP GRO packets are performed correctly, both with and without
driver-side support, so it's safe to enable.
Also, as I said in reply to your RFC, NATing of UDP GRO fraglists
is performed correctly if driver is aware of it and advertises a
support for fraglist GSO.
If not, then there may be problems you described. But the idea to
forbid forwarding and NATing of any UDP GRO skbs is an absolute
overkill.
>>> Plain UDP GRO forwarding even shows better performance than fraglisted
>>> UDP GRO in some cases due to not wasting one skbuff_head per every
>>> segment.
>>>
>>> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
>>> ---
>>> net/ipv4/udp_offload.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>> index ff39e94781bf..9d71df3d52ce 100644
>>> --- a/net/ipv4/udp_offload.c
>>> +++ b/net/ipv4/udp_offload.c
>>> @@ -460,12 +460,13 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
>>> if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
>>> NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
>
> is_flist can be true even if !sk.
>
>>>
>>> - if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {
>>> + if (!sk || (sk && udp_sk(sk)->gro_enabled) ||
>
> Actually sk would be NULL by udp_encap_needed_key in udp4_gro_receive
> or udp6_gro_receive.
>
>>> + NAPI_GRO_CB(skb)->is_flist) {
>>> pp = call_gro_receive(udp_gro_receive_segment, head, skb);
>
> udp_gro_receive_segment will check is_flist first and try to do
> fraglisted UDP GRO. Can you check what I'm missing?
I think you miss that is_flist is set to true *only* if the receiving
netdevice has NETIF_F_GRO_FRAGLIST feature on.
If it's not, stack will form a non-fraglisted UDP GRO skb. That was
tested multiple times.
>>> return pp;
>>> }
>>>
>>
>> The second check for sk in "(sk && udp_sk(sk)->gro_enabled)" is
>> redundant and can be dropped. You already verified it is present when
>> you checked for !sk before the logical OR.
Alex are right, I'll simplify the condition in v2. Thanks!
> Sorry, Alexander Duyck. I believe Alexander Lobakin will answer this.
>
>>> - if (!sk || NAPI_GRO_CB(skb)->encap_mark ||
>>> + if (NAPI_GRO_CB(skb)->encap_mark ||
>>> (skb->ip_summed != CHECKSUM_PARTIAL &&
>>> NAPI_GRO_CB(skb)->csum_cnt == 0 &&
>>> !NAPI_GRO_CB(skb)->csum_valid) ||
>>>
Al
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets
@ 2021-01-13 0:42 kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-01-13 0:42 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210112211536.261172-1-alobakin@pm.me>
References: <20210112211536.261172-1-alobakin@pm.me>
TO: Alexander Lobakin <alobakin@pm.me>
TO: "David S. Miller" <davem@davemloft.net>
CC: netdev(a)vger.kernel.org
TO: Jakub Kicinski <kuba@kernel.org>
CC: Eric Dumazet <edumazet@google.com>
CC: Edward Cree <ecree@solarflare.com>
CC: Willem de Bruijn <willemb@google.com>
CC: Steffen Klassert <steffen.klassert@secunet.com>
CC: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
CC: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
CC: linux-kernel(a)vger.kernel.org
Hi Alexander,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Alexander-Lobakin/udp-allow-forwarding-of-plain-non-fraglisted-UDP-GRO-packets/20210113-054212
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git c73a45965dd54a10c368191804b9de661eee1007
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
config: i386-randconfig-c001-20210112 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>
"coccinelle warnings: (new ones prefixed by >>)"
>> net/ipv4/udp_offload.c:463:16-18: WARNING !A || A && B is equivalent to !A || B
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30195 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-13 10:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-12 21:16 [PATCH net-next] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets Alexander Lobakin
2021-01-13 3:10 ` Alexander Duyck
2021-01-13 3:59 ` Dongseok Yi
2021-01-13 10:05 ` Alexander Lobakin
2021-01-13 0:42 kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.