[PATCH net-next 0/2] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets

[PATCH net-next 0/2] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets

[Alexander Lobakin]

This series allows to form UDP GRO packets in cases without sockets,
primarily for forwarding. To not change the current datapath, this
is performed only when the new netdev feature is enabled via Ethtool.
Prior to this point, only fraglisted UDP GRO was available.

Since v2 [1]:
 - convert to a series;
 - new: add new netdev_feature to explicitly enable/disable UDP GRO
   when there is no socket, defaults to off (Paolo Abeni).

Since v1 [0]:
 - drop redundant 'if (sk)' check (Alexander Duyck);
 - add a ref in the commit message to one more commit that was
   an important step for UDP GRO forwarding.

[0] https://lore.kernel.org/netdev/20210112211536.261172-1-alobakin@pm.me
[1] https://lore.kernel.org/netdev/20210113103232.4761-1-alobakin@pm.me

Alexander Lobakin (2):
  net: introduce UDP GRO netdev feature
  udp: allow forwarding of plain (non-fraglisted) UDP GRO packets

 include/linux/netdev_features.h |  4 +++-
 net/ethtool/common.c            |  1 +
 net/ipv4/udp_offload.c          | 16 +++++++++++-----
 3 files changed, 15 insertions(+), 6 deletions(-)

-- 
2.30.0

[PATCH net-next 1/2] net: introduce UDP GRO netdev feature

[Alexander Lobakin]

Introduce a new netdev_feature, NETIF_F_GRO_UDP, to allow user
to turn UDP GRO on and off when there is no socket, e.g. when
forwarding.
Defaults to off to not change current datapath.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
 include/linux/netdev_features.h | 4 +++-
 net/ethtool/common.c            | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 934de56644e7..c6526aa13684 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -84,6 +84,7 @@ enum {
 	NETIF_F_GRO_FRAGLIST_BIT,	/* Fraglist GRO */
 
 	NETIF_F_HW_MACSEC_BIT,		/* Offload MACsec operations */
+	NETIF_F_GRO_UDP_BIT,		/* Enable UDP GRO */
 
 	/*
 	 * Add your fresh new feature above and remember to update
@@ -157,6 +158,7 @@ enum {
 #define NETIF_F_GRO_FRAGLIST	__NETIF_F(GRO_FRAGLIST)
 #define NETIF_F_GSO_FRAGLIST	__NETIF_F(GSO_FRAGLIST)
 #define NETIF_F_HW_MACSEC	__NETIF_F(HW_MACSEC)
+#define NETIF_F_GRO_UDP		__NETIF_F(GRO_UDP)
 
 /* Finds the next feature with the highest number of the range of start till 0.
  */
@@ -234,7 +236,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
 #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
 
 /* Changeable features with no special hardware requirements that defaults to off. */
-#define NETIF_F_SOFT_FEATURES_OFF	NETIF_F_GRO_FRAGLIST
+#define NETIF_F_SOFT_FEATURES_OFF	(NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP)
 
 #define NETIF_F_VLAN_FEATURES	(NETIF_F_HW_VLAN_CTAG_FILTER | \
 				 NETIF_F_HW_VLAN_CTAG_RX | \
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 24036e3055a1..e45128882372 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -68,6 +68,7 @@ const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
 	[NETIF_F_HW_TLS_RX_BIT] =	 "tls-hw-rx-offload",
 	[NETIF_F_GRO_FRAGLIST_BIT] =	 "rx-gro-list",
 	[NETIF_F_HW_MACSEC_BIT] =	 "macsec-hw-offload",
+	[NETIF_F_GRO_UDP_BIT] =		 "rx-gro-udp",
 };
 
 const char
-- 
2.30.0

[PATCH net-next 2/2] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets

[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.
Commit 2e4ef10f5850 ("net: add GSO UDP L4 and GSO fraglists to the
list of software-backed types") added GSO UDP L4 to the list of
software GSO to allow virtual netdevs to forward them as is up to
the real drivers.

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

Add the last element and allow to form plain UDP GRO packets if
there is no socket -> we are on forwarding path, and the new
NETIF_F_GRO_UDP is enabled on a receiving netdevice.
Note that fraglisted UDP GRO now also depends on this feature, as
NETIF_F_GRO_FRAGLIST isn't tied to any particular L4 protocol.

Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
 net/ipv4/udp_offload.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index ff39e94781bf..781a035de5a9 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -454,13 +454,19 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
 	struct sk_buff *p;
 	struct udphdr *uh2;
 	unsigned int off = skb_gro_offset(skb);
-	int flush = 1;
+	int flist = 0, flush = 1;
+	bool gro_by_feat = false;
 
-	NAPI_GRO_CB(skb)->is_flist = 0;
-	if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
-		NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
+	if (skb->dev->features & NETIF_F_GRO_UDP) {
+		if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
+			flist = !sk || !udp_sk(sk)->gro_enabled;
 
-	if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {
+		gro_by_feat = !sk || flist;
+	}
+
+	NAPI_GRO_CB(skb)->is_flist = flist;
+
+	if (gro_by_feat || (sk && udp_sk(sk)->gro_enabled)) {
 		pp = call_gro_receive(udp_gro_receive_segment, head, skb);
 		return pp;
 	}
-- 
2.30.0

Re: [PATCH net-next 2/2] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets

[Willem de Bruijn]

On Mon, Jan 18, 2021 at 2:33 PM Alexander Lobakin <alobakin@pm.me> 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.
> Commit 2e4ef10f5850 ("net: add GSO UDP L4 and GSO fraglists to the
> list of software-backed types") added GSO UDP L4 to the list of
> software GSO to allow virtual netdevs to forward them as is up to
> the real drivers.
>
> 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.
> 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.

That is surprising. The choice for fraglist based forwarding was made
on the assumption that it is cheaper if software segmentation is needed.

Do you have a more specific definition of the relevant cases?

There currently is no option to enable GRO for forwarding, without
fraglist if to a device with h/w udp segmentation offload. This would
add that option too.

Though under admin control, which may make it a rarely exercised option.
Assuming most hosts to have single or homogeneous NICs, the OS should
be able to choose the preferred option in most cases (e.g.,: use fraglist
unless all devices support h/w gro).

> Add the last element and allow to form plain UDP GRO packets if
> there is no socket -> we are on forwarding path, and the new
> NETIF_F_GRO_UDP is enabled on a receiving netdevice.
> Note that fraglisted UDP GRO now also depends on this feature, as

That may cause a regression for applications that currently enable
that device feature.

> NETIF_F_GRO_FRAGLIST isn't tied to any particular L4 protocol.
>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> ---
>  net/ipv4/udp_offload.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index ff39e94781bf..781a035de5a9 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -454,13 +454,19 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
>         struct sk_buff *p;
>         struct udphdr *uh2;
>         unsigned int off = skb_gro_offset(skb);
> -       int flush = 1;
> +       int flist = 0, flush = 1;
> +       bool gro_by_feat = false;

What is this variable shorthand for? By feature? Perhaps
gro_forwarding is more descriptive.

>
> -       NAPI_GRO_CB(skb)->is_flist = 0;
> -       if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> -               NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
> +       if (skb->dev->features & NETIF_F_GRO_UDP) {
> +               if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> +                       flist = !sk || !udp_sk(sk)->gro_enabled;
>
> -       if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {

I would almost rename NETIF_F_GRO_FRAGLIST to NETIF_F_UDP_GRO_FWD.
Then this could be a !NETIF_F_UDP_GRO_FWD_FRAGLIST toggle on top of
that. If it wasn't for this fraglist option also enabling UDP GRO to
local sockets if set.

That is, if the performance difference is significant enough to
require supporting both types of forwarding, under admin control.

Perhaps the simplest alternative is to add the new feature without
making fraglist dependent on it:

  if ((sk && udp_sk(sk)->gro_enabled) ||
      (skb->dev->features & NETIF_F_GRO_FRAGLIST) ||
      (!sk && skb->dev->features & NETIF_F_GRO_UDP_FWD))






> +               gro_by_feat = !sk || flist;
> +       }
> +
> +       NAPI_GRO_CB(skb)->is_flist = flist;
> +
> +       if (gro_by_feat || (sk && udp_sk(sk)->gro_enabled)) {
>                 pp = call_gro_receive(udp_gro_receive_segment, head, skb);
>                 return pp;
>         }
> --
> 2.30.0
>
>

Re: [PATCH net-next 2/2] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets

[Alexander Lobakin]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Thu, 21 Jan 2021 21:47:47 -0500

> On Mon, Jan 18, 2021 at 2:33 PM Alexander Lobakin <alobakin@pm.me> 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.
> > Commit 2e4ef10f5850 ("net: add GSO UDP L4 and GSO fraglists to the
> > list of software-backed types") added GSO UDP L4 to the list of
> > software GSO to allow virtual netdevs to forward them as is up to
> > the real drivers.
> >
> > 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.
> > 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.
> 
> That is surprising. The choice for fraglist based forwarding was made
> on the assumption that it is cheaper if software segmentation is needed.
> 
> Do you have a more specific definition of the relevant cases?

"Classic" UDP GRO shows better performance when forwarding to a NIC
that supports GSO UDP L4 (i.e. no software segmentation occurs), like
the one that I test kernel on.
I don't have much info about performance without UDP GSO offload
as I usually test NAT, and fralisted UDP GRO currently fails on
this [0].

> There currently is no option to enable GRO for forwarding, without
> fraglist if to a device with h/w udp segmentation offload. This would
> add that option too.

Yes, that's exactly what I want. I want to maximize UDP
forwarding/NATing performance when NIC is capable of UDP GSO offload,
as I said above, non-fraglisted UDP GRO is better for that case.

> Though under admin control, which may make it a rarely exercised option.
> Assuming most hosts to have single or homogeneous NICs, the OS should
> be able to choose the preferred option in most cases (e.g.,: use fraglist
> unless all devices support h/w gro).

I though about some sort of auto-selection, but at the moment of
receiving we can't know which interface this skb will be forwarded
to.
Also, as Paolo Abeni said in a comment to v2, UDP GRO may cause
sensible delays, which may be inacceptable in some environments.
That's why we have to use a sockopt and netdev features to explicitly
enable UDP GRO.

Regarding all this, I introduced NETIF_F_UDP_GRO to have the
following chose:
 - both NETIF_F_UDP_GRO and NETIF_F_GRO_FRAGLIST is off - no UDP GRO;
 - NETIF_F_UDP_GRO is on, NETIF_F_GRO_FRAGLIST is off - classic GRO;
 - both NETIF_F_UDP_GRO and NETIF_F_GRO_FRAGLIST is on - fraglisted
   UDP GRO.

> > Add the last element and allow to form plain UDP GRO packets if
> > there is no socket -> we are on forwarding path, and the new
> > NETIF_F_GRO_UDP is enabled on a receiving netdevice.
> > Note that fraglisted UDP GRO now also depends on this feature, as
> 
> That may cause a regression for applications that currently enable
> that device feature.

Thought about this one too. Not sure if it would be better to leave
it as it is for now or how it's done in this series. The problem
that we may have in future is that in some day we may get fraglisted
TCP GRO, and then NETIF_F_GRO_FRAGLIST will affect both TCP and UDP,
which is not desirable as for me. So I decided to guard this possible
case.

> > NETIF_F_GRO_FRAGLIST isn't tied to any particular L4 protocol.
> >
> > Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> > ---
> >  net/ipv4/udp_offload.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index ff39e94781bf..781a035de5a9 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -454,13 +454,19 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
> >         struct sk_buff *p;
> >         struct udphdr *uh2;
> >         unsigned int off = skb_gro_offset(skb);
> > -       int flush = 1;
> > +       int flist = 0, flush = 1;
> > +       bool gro_by_feat = false;
> 
> What is this variable shorthand for? By feature? Perhaps
> gro_forwarding is more descriptive.

Yes, I chose "by feature" because fraglisted GRO also starts to
work for local traffic if enabled, so "gro_forwarding" would be
inaccurate naming.

> >
> > -       NAPI_GRO_CB(skb)->is_flist = 0;
> > -       if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> > -               NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;

I mean this. is_flist gets enabled if socket GRO option is disabled.

> > +       if (skb->dev->features & NETIF_F_GRO_UDP) {
> > +               if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> > +                       flist = !sk || !udp_sk(sk)->gro_enabled;
> >
> > -       if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {
> 
> I would almost rename NETIF_F_GRO_FRAGLIST to NETIF_F_UDP_GRO_FWD.
> Then this could be a !NETIF_F_UDP_GRO_FWD_FRAGLIST toggle on top of
> that. If it wasn't for this fraglist option also enabling UDP GRO to
> local sockets if set.
> 
> That is, if the performance difference is significant enough to
> require supporting both types of forwarding, under admin control.
> 
> Perhaps the simplest alternative is to add the new feature without
> making fraglist dependent on it:
> 
>   if ((sk && udp_sk(sk)->gro_enabled) ||
>       (skb->dev->features & NETIF_F_GRO_FRAGLIST) ||
>       (!sk && skb->dev->features & NETIF_F_GRO_UDP_FWD))

Yep, this will be the exact code if we end up with that
NETIF_F_GRO_FRAGLIST should not depends on new netdev feature.
But again, I wanted to protect TCP GRO if fraglisted TCP GRO will
ever land the kernel. May be it's too much for the feature that
currently doesn't exists even as a draft or plan, not sure.

So, I'd stick to this variant (NETIF_F_UDP_GRO_FWD for plain,
NETIF_F_GRO_FRAGLIST without changes for fraglisted) if preferred.

> > +               gro_by_feat = !sk || flist;
> > +       }
> > +
> > +       NAPI_GRO_CB(skb)->is_flist = flist;
> > +
> > +       if (gro_by_feat || (sk && udp_sk(sk)->gro_enabled)) {
> >                 pp = call_gro_receive(udp_gro_receive_segment, head, skb);
> >                 return pp;
> >         }
> > --
> > 2.30.0

[0] https://lore.kernel.org/netdev/1611235479-39399-1-git-send-email-dseok.yi@samsung.com

Thanks,
Al

Re: [PATCH net-next 2/2] udp: allow forwarding of plain (non-fraglisted) UDP GRO packets

[Willem de Bruijn]

On Fri, Jan 22, 2021 at 6:25 AM Alexander Lobakin <alobakin@pm.me> wrote:
>
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Thu, 21 Jan 2021 21:47:47 -0500
>
> > On Mon, Jan 18, 2021 at 2:33 PM Alexander Lobakin <alobakin@pm.me> 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.
> > > Commit 2e4ef10f5850 ("net: add GSO UDP L4 and GSO fraglists to the
> > > list of software-backed types") added GSO UDP L4 to the list of
> > > software GSO to allow virtual netdevs to forward them as is up to
> > > the real drivers.
> > >
> > > 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.
> > > 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.
> >
> > That is surprising. The choice for fraglist based forwarding was made
> > on the assumption that it is cheaper if software segmentation is needed.
> >
> > Do you have a more specific definition of the relevant cases?
>
> "Classic" UDP GRO shows better performance when forwarding to a NIC
> that supports GSO UDP L4 (i.e. no software segmentation occurs), like
> the one that I test kernel on.
> I don't have much info about performance without UDP GSO offload
> as I usually test NAT, and fralisted UDP GRO currently fails on
> this [0].
>
> > There currently is no option to enable GRO for forwarding, without
> > fraglist if to a device with h/w udp segmentation offload. This would
> > add that option too.
>
> Yes, that's exactly what I want. I want to maximize UDP
> forwarding/NATing performance when NIC is capable of UDP GSO offload,
> as I said above, non-fraglisted UDP GRO is better for that case.

That makes sense. Better to make explicit that that is the case
targeted here, rather than "some cases".

> > Though under admin control, which may make it a rarely exercised option.
> > Assuming most hosts to have single or homogeneous NICs, the OS should
> > be able to choose the preferred option in most cases (e.g.,: use fraglist
> > unless all devices support h/w gro).
>
> I though about some sort of auto-selection, but at the moment of
> receiving we can't know which interface this skb will be forwarded
> to.
> Also, as Paolo Abeni said in a comment to v2, UDP GRO may cause
> sensible delays, which may be inacceptable in some environments.
> That's why we have to use a sockopt and netdev features to explicitly
> enable UDP GRO.

I'm suspect that such fine-grained toggles end up broadly unused.

Agreed that it is not always possible to predict the destination NIC,
but that is why I suggested a very low bar that I believe captures the
majority of installed systems: where all NICs support the feature.
Anyway, that can always be added later -- as long as having this flag
off is not interpreted as demanding fraglist on forwarding.

> Regarding all this, I introduced NETIF_F_UDP_GRO to have the
> following chose:
>  - both NETIF_F_UDP_GRO and NETIF_F_GRO_FRAGLIST is off - no UDP GRO;
>  - NETIF_F_UDP_GRO is on, NETIF_F_GRO_FRAGLIST is off - classic GRO;
>  - both NETIF_F_UDP_GRO and NETIF_F_GRO_FRAGLIST is on - fraglisted
>    UDP GRO.
>
> > > Add the last element and allow to form plain UDP GRO packets if
> > > there is no socket -> we are on forwarding path, and the new
> > > NETIF_F_GRO_UDP is enabled on a receiving netdevice.
> > > Note that fraglisted UDP GRO now also depends on this feature, as
> >
> > That may cause a regression for applications that currently enable
> > that device feature.
>
> Thought about this one too. Not sure if it would be better to leave
> it as it is for now or how it's done in this series. The problem
> that we may have in future is that in some day we may get fraglisted
> TCP GRO, and then NETIF_F_GRO_FRAGLIST will affect both TCP and UDP,
> which is not desirable as for me. So I decided to guard this possible
> case.
>
> > > NETIF_F_GRO_FRAGLIST isn't tied to any particular L4 protocol.

As its name implies. I think it makes more sense to see it as an
explicit request to use fraglist for any protocol that supports it.

> > >
> > > Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> > > ---
> > >  net/ipv4/udp_offload.c | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > > index ff39e94781bf..781a035de5a9 100644
> > > --- a/net/ipv4/udp_offload.c
> > > +++ b/net/ipv4/udp_offload.c
> > > @@ -454,13 +454,19 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
> > >         struct sk_buff *p;
> > >         struct udphdr *uh2;
> > >         unsigned int off = skb_gro_offset(skb);
> > > -       int flush = 1;
> > > +       int flist = 0, flush = 1;
> > > +       bool gro_by_feat = false;
> >
> > What is this variable shorthand for? By feature? Perhaps
> > gro_forwarding is more descriptive.
>
> Yes, I chose "by feature" because fraglisted GRO also starts to
> work for local traffic if enabled, so "gro_forwarding" would be
> inaccurate naming.
>
> > >
> > > -       NAPI_GRO_CB(skb)->is_flist = 0;
> > > -       if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> > > -               NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
>
> I mean this. is_flist gets enabled if socket GRO option is disabled.
>
> > > +       if (skb->dev->features & NETIF_F_GRO_UDP) {
> > > +               if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> > > +                       flist = !sk || !udp_sk(sk)->gro_enabled;
> > >
> > > -       if ((sk && udp_sk(sk)->gro_enabled) || NAPI_GRO_CB(skb)->is_flist) {
> >
> > I would almost rename NETIF_F_GRO_FRAGLIST to NETIF_F_UDP_GRO_FWD.
> > Then this could be a !NETIF_F_UDP_GRO_FWD_FRAGLIST toggle on top of
> > that. If it wasn't for this fraglist option also enabling UDP GRO to
> > local sockets if set.
> >
> > That is, if the performance difference is significant enough to
> > require supporting both types of forwarding, under admin control.
> >
> > Perhaps the simplest alternative is to add the new feature without
> > making fraglist dependent on it:
> >
> >   if ((sk && udp_sk(sk)->gro_enabled) ||
> >       (skb->dev->features & NETIF_F_GRO_FRAGLIST) ||
> >       (!sk && skb->dev->features & NETIF_F_GRO_UDP_FWD))
>
> Yep, this will be the exact code if we end up with that
> NETIF_F_GRO_FRAGLIST should not depends on new netdev feature.
> But again, I wanted to protect TCP GRO if fraglisted TCP GRO will
> ever land the kernel. May be it's too much for the feature that
> currently doesn't exists even as a draft or plan, not sure.

If a protocol lands an fraglist implementation, I think the
expectation is that it will respond to the fraglist bit. I don't
understand why to preemptively block this. Then we would need yet
another feature bit for "forward <protocol> with fraglist".

>
> So, I'd stick to this variant (NETIF_F_UDP_GRO_FWD for plain,
> NETIF_F_GRO_FRAGLIST without changes for fraglisted) if preferred.
>
> > > +               gro_by_feat = !sk || flist;
> > > +       }
> > > +
> > > +       NAPI_GRO_CB(skb)->is_flist = flist;
> > > +
> > > +       if (gro_by_feat || (sk && udp_sk(sk)->gro_enabled)) {
> > >                 pp = call_gro_receive(udp_gro_receive_segment, head, skb);
> > >                 return pp;
> > >         }
> > > --
> > > 2.30.0
>
> [0] https://lore.kernel.org/netdev/1611235479-39399-1-git-send-email-dseok.yi@samsung.com
>
> Thanks,
> Al
>