[PATCH net-next 0/2] net: remove two MAX_SKB_FRAGS dependencies

[PATCH net-next 0/2] net: remove two MAX_SKB_FRAGS dependencies

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

My first series for BIG TCP met two issues that various bots pointed out,
because of MAX_SKB_FRAGS going from 17 to 45.

- typhoon driver would emit a compile error if MAX_SKB_FRAGS > 32

- include/linux/skmsg.h had a similar issue when MAX_SKB_FRAGS is
slightly over BITS_PER_LONG, so the build broke on 32bit arches.

This patch series is meant to be merged before BIG TCP one,
in order to keep the latter not too big.

Eric Dumazet (2):
  net: typhoon: implement ndo_features_check method
  skmsg: convert struct sk_msg_sg::copy to a bitmap

 drivers/net/ethernet/3com/typhoon.c | 23 ++++++++++++++++++-----
 include/linux/skmsg.h               | 11 +++++------
 net/core/filter.c                   |  4 ++--
 3 files changed, 25 insertions(+), 13 deletions(-)

-- 
2.35.0.263.gb82422642f-goog

[PATCH net-next 1/2] net: typhoon: implement ndo_features_check method

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Instead of disabling TSO if MAX_SKB_FRAGS > 32, implement
ndo_features_check() method for this driver.

If skb has more than 32 frags, use the following heuristic:

1) force GSO for gso packets.
2) Otherwise force linearization.

Most locally generated packets will use a small number
of fragments anyway.

For forwarding workloads, we can limit gro_max_size at ingress,
we might also implement gro_max_segs if needed.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/ethernet/3com/typhoon.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/3com/typhoon.c b/drivers/net/ethernet/3com/typhoon.c
index 8aec5d9fbfef2803c181387537300502a937caf0..67b1a1f439d841ed0ed0f620e9477607ac6e2fae 100644
--- a/drivers/net/ethernet/3com/typhoon.c
+++ b/drivers/net/ethernet/3com/typhoon.c
@@ -138,11 +138,6 @@ MODULE_PARM_DESC(use_mmio, "Use MMIO (1) or PIO(0) to access the NIC. "
 module_param(rx_copybreak, int, 0);
 module_param(use_mmio, int, 0);
 
-#if defined(NETIF_F_TSO) && MAX_SKB_FRAGS > 32
-#warning Typhoon only supports 32 entries in its SG list for TSO, disabling TSO
-#undef NETIF_F_TSO
-#endif
-
 #if TXLO_ENTRIES <= (2 * MAX_SKB_FRAGS)
 #error TX ring too small!
 #endif
@@ -2261,9 +2256,27 @@ typhoon_test_mmio(struct pci_dev *pdev)
 	return mode;
 }
 
+#if MAX_SKB_FRAGS > 32
+static netdev_features_t typhoon_features_check(struct sk_buff *skb,
+						struct net_device *dev,
+						netdev_features_t features)
+{
+	if (skb_shinfo(skb)->nr_frags > 32) {
+		if (skb_is_gso(skb))
+			features &= ~NETIF_F_GSO_MASK;
+		else
+			features &= ~NETIF_F_SG;
+	}
+	return features;
+}
+#endif
+
 static const struct net_device_ops typhoon_netdev_ops = {
 	.ndo_open		= typhoon_open,
 	.ndo_stop		= typhoon_close,
+#if MAX_SKB_FRAGS > 32
+	.ndo_features_check	= typhoon_features_check,
+#endif
 	.ndo_start_xmit		= typhoon_start_tx,
 	.ndo_set_rx_mode	= typhoon_set_rx_mode,
 	.ndo_tx_timeout		= typhoon_tx_timeout,
-- 
2.35.0.263.gb82422642f-goog

[PATCH net-next 2/2] skmsg: convert struct sk_msg_sg::copy to a bitmap

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

We have plans for increasing MAX_SKB_FRAGS, but sk_msg_sg::copy
is currently an unsigned long, limiting MAX_SKB_FRAGS to 30 on 32bit arches.

Convert it to a bitmap, as Jakub suggested.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/skmsg.h | 11 +++++------
 net/core/filter.c     |  4 ++--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 18a717fe62eb049758bc1502da97365cf7587ffd..1ff68a88c58de0dc30a50fd030b49ea55a6c2cb9 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -29,7 +29,7 @@ struct sk_msg_sg {
 	u32				end;
 	u32				size;
 	u32				copybreak;
-	unsigned long			copy;
+	DECLARE_BITMAP(copy, MAX_MSG_FRAGS + 2);
 	/* The extra two elements:
 	 * 1) used for chaining the front and sections when the list becomes
 	 *    partitioned (e.g. end < start). The crypto APIs require the
@@ -38,7 +38,6 @@ struct sk_msg_sg {
 	 */
 	struct scatterlist		data[MAX_MSG_FRAGS + 2];
 };
-static_assert(BITS_PER_LONG >= NR_MSG_FRAG_IDS);
 
 /* UAPI in filter.c depends on struct sk_msg_sg being first element. */
 struct sk_msg {
@@ -234,7 +233,7 @@ static inline void sk_msg_compute_data_pointers(struct sk_msg *msg)
 {
 	struct scatterlist *sge = sk_msg_elem(msg, msg->sg.start);
 
-	if (test_bit(msg->sg.start, &msg->sg.copy)) {
+	if (test_bit(msg->sg.start, msg->sg.copy)) {
 		msg->data = NULL;
 		msg->data_end = NULL;
 	} else {
@@ -253,7 +252,7 @@ static inline void sk_msg_page_add(struct sk_msg *msg, struct page *page,
 	sg_set_page(sge, page, len, offset);
 	sg_unmark_end(sge);
 
-	__set_bit(msg->sg.end, &msg->sg.copy);
+	__set_bit(msg->sg.end, msg->sg.copy);
 	msg->sg.size += len;
 	sk_msg_iter_next(msg, end);
 }
@@ -262,9 +261,9 @@ static inline void sk_msg_sg_copy(struct sk_msg *msg, u32 i, bool copy_state)
 {
 	do {
 		if (copy_state)
-			__set_bit(i, &msg->sg.copy);
+			__set_bit(i, msg->sg.copy);
 		else
-			__clear_bit(i, &msg->sg.copy);
+			__clear_bit(i, msg->sg.copy);
 		sk_msg_iter_var_next(i);
 		if (i == msg->sg.end)
 			break;
diff --git a/net/core/filter.c b/net/core/filter.c
index 9615ae1ab530a9735e3d862b1ddf12b1b1f55060..f497ca7a16d223855004876e3a868d5de8cea879 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2603,7 +2603,7 @@ BPF_CALL_4(bpf_msg_pull_data, struct sk_msg *, msg, u32, start,
 	 * account for the headroom.
 	 */
 	bytes_sg_total = start - offset + bytes;
-	if (!test_bit(i, &msg->sg.copy) && bytes_sg_total <= len)
+	if (!test_bit(i, msg->sg.copy) && bytes_sg_total <= len)
 		goto out;
 
 	/* At this point we need to linearize multiple scatterlist
@@ -2809,7 +2809,7 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
 	/* Place newly allocated data buffer */
 	sk_mem_charge(msg->sk, len);
 	msg->sg.size += len;
-	__clear_bit(new, &msg->sg.copy);
+	__clear_bit(new, msg->sg.copy);
 	sg_set_page(&msg->sg.data[new], page, len + copy, 0);
 	if (rsge.length) {
 		get_page(sg_page(&rsge));
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH net-next 1/2] net: typhoon: implement ndo_features_check method

[Jakub Kicinski]

On Thu,  3 Feb 2022 10:02:26 -0800 Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Instead of disabling TSO if MAX_SKB_FRAGS > 32, implement
> ndo_features_check() method for this driver.
> 
> If skb has more than 32 frags, use the following heuristic:
> 
> 1) force GSO for gso packets.
> 2) Otherwise force linearization.
> 
> Most locally generated packets will use a small number
> of fragments anyway.
> 
> For forwarding workloads, we can limit gro_max_size at ingress,
> we might also implement gro_max_segs if needed.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  drivers/net/ethernet/3com/typhoon.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/3com/typhoon.c b/drivers/net/ethernet/3com/typhoon.c
> index 8aec5d9fbfef2803c181387537300502a937caf0..67b1a1f439d841ed0ed0f620e9477607ac6e2fae 100644
> --- a/drivers/net/ethernet/3com/typhoon.c
> +++ b/drivers/net/ethernet/3com/typhoon.c
> @@ -138,11 +138,6 @@ MODULE_PARM_DESC(use_mmio, "Use MMIO (1) or PIO(0) to access the NIC. "
>  module_param(rx_copybreak, int, 0);
>  module_param(use_mmio, int, 0);
>  
> -#if defined(NETIF_F_TSO) && MAX_SKB_FRAGS > 32
> -#warning Typhoon only supports 32 entries in its SG list for TSO, disabling TSO
> -#undef NETIF_F_TSO
> -#endif
> -
>  #if TXLO_ENTRIES <= (2 * MAX_SKB_FRAGS)
>  #error TX ring too small!
>  #endif
> @@ -2261,9 +2256,27 @@ typhoon_test_mmio(struct pci_dev *pdev)
>  	return mode;
>  }
>  
> +#if MAX_SKB_FRAGS > 32
> +static netdev_features_t typhoon_features_check(struct sk_buff *skb,
> +						struct net_device *dev,
> +						netdev_features_t features)
> +{
> +	if (skb_shinfo(skb)->nr_frags > 32) {
> +		if (skb_is_gso(skb))
> +			features &= ~NETIF_F_GSO_MASK;
> +		else
> +			features &= ~NETIF_F_SG;

Should we always clear SG? If we want to make the assumption that
non-gso skbs are never this long (like the driver did before) then
we should never clear SG. If we do we risk one of the gso-generated
segs will also be longer than 32 frags.

Thought I should ask.

> +	}
> +	return features;

return vlan_features_check(skb, features) ?

> +}
> +#endif
> +
>  static const struct net_device_ops typhoon_netdev_ops = {
>  	.ndo_open		= typhoon_open,
>  	.ndo_stop		= typhoon_close,
> +#if MAX_SKB_FRAGS > 32
> +	.ndo_features_check	= typhoon_features_check,
> +#endif
>  	.ndo_start_xmit		= typhoon_start_tx,
>  	.ndo_set_rx_mode	= typhoon_set_rx_mode,
>  	.ndo_tx_timeout		= typhoon_tx_timeout,

Re: [PATCH net-next 1/2] net: typhoon: implement ndo_features_check method

[Eric Dumazet]

On Fri, Feb 4, 2022 at 7:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu,  3 Feb 2022 10:02:26 -0800 Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Instead of disabling TSO if MAX_SKB_FRAGS > 32, implement
> > ndo_features_check() method for this driver.
> >
> > If skb has more than 32 frags, use the following heuristic:
> >
> > 1) force GSO for gso packets.
> > 2) Otherwise force linearization.
> >
> > Most locally generated packets will use a small number
> > of fragments anyway.
> >
> > For forwarding workloads, we can limit gro_max_size at ingress,
> > we might also implement gro_max_segs if needed.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  drivers/net/ethernet/3com/typhoon.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/3com/typhoon.c b/drivers/net/ethernet/3com/typhoon.c
> > index 8aec5d9fbfef2803c181387537300502a937caf0..67b1a1f439d841ed0ed0f620e9477607ac6e2fae 100644
> > --- a/drivers/net/ethernet/3com/typhoon.c
> > +++ b/drivers/net/ethernet/3com/typhoon.c
> > @@ -138,11 +138,6 @@ MODULE_PARM_DESC(use_mmio, "Use MMIO (1) or PIO(0) to access the NIC. "
> >  module_param(rx_copybreak, int, 0);
> >  module_param(use_mmio, int, 0);
> >
> > -#if defined(NETIF_F_TSO) && MAX_SKB_FRAGS > 32
> > -#warning Typhoon only supports 32 entries in its SG list for TSO, disabling TSO
> > -#undef NETIF_F_TSO
> > -#endif
> > -
> >  #if TXLO_ENTRIES <= (2 * MAX_SKB_FRAGS)
> >  #error TX ring too small!
> >  #endif
> > @@ -2261,9 +2256,27 @@ typhoon_test_mmio(struct pci_dev *pdev)
> >       return mode;
> >  }
> >
> > +#if MAX_SKB_FRAGS > 32
> > +static netdev_features_t typhoon_features_check(struct sk_buff *skb,
> > +                                             struct net_device *dev,
> > +                                             netdev_features_t features)
> > +{
> > +     if (skb_shinfo(skb)->nr_frags > 32) {
> > +             if (skb_is_gso(skb))
> > +                     features &= ~NETIF_F_GSO_MASK;
> > +             else
> > +                     features &= ~NETIF_F_SG;
>
> Should we always clear SG? If we want to make the assumption that
> non-gso skbs are never this long (like the driver did before) then
> we should never clear SG. If we do we risk one of the gso-generated
> segs will also be longer than 32 frags.

If I read the comment (deleted in this patch), it seems the 32 limits
is about TSO only ?

#warning Typhoon only supports 32 entries in its SG list for TSO, disabling TSO

This is why I chose this implementation.

>
> Thought I should ask.
>
> > +     }
> > +     return features;
>
> return vlan_features_check(skb, features) ?

Hmm... not sure why we duplicate vlan_features_check() &
vxlan_features_check() in all ndo_features_check() handlers :/

>
> > +}
> > +#endif
> > +
> >  static const struct net_device_ops typhoon_netdev_ops = {
> >       .ndo_open               = typhoon_open,
> >       .ndo_stop               = typhoon_close,
> > +#if MAX_SKB_FRAGS > 32
> > +     .ndo_features_check     = typhoon_features_check,
> > +#endif
> >       .ndo_start_xmit         = typhoon_start_tx,
> >       .ndo_set_rx_mode        = typhoon_set_rx_mode,
> >       .ndo_tx_timeout         = typhoon_tx_timeout,
>

Re: [PATCH net-next 1/2] net: typhoon: implement ndo_features_check method

[Jakub Kicinski]

On Fri, 4 Feb 2022 20:26:58 -0800 Eric Dumazet wrote:
> > Should we always clear SG? If we want to make the assumption that
> > non-gso skbs are never this long (like the driver did before) then
> > we should never clear SG. If we do we risk one of the gso-generated
> > segs will also be longer than 32 frags.  
> 
> If I read the comment (deleted in this patch), it seems the 32 limits
> is about TSO only ?
> 
> #warning Typhoon only supports 32 entries in its SG list for TSO, disabling TSO
> 
> This is why I chose this implementation.

Right, sort of my point - to stay true to old code we don't need to
worry about SG ? The old code didn't..

> > Thought I should ask.
> >  
> > > +     }
> > > +     return features;  
> >
> > return vlan_features_check(skb, features) ?  
> 
> Hmm... not sure why we duplicate vlan_features_check() &
> vxlan_features_check() in all ndo_features_check() handlers :/

I was wondering as well. I can only speculate.. :S

Re: [PATCH net-next 1/2] net: typhoon: implement ndo_features_check method

[Eric Dumazet]

On Fri, Feb 4, 2022 at 8:34 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 4 Feb 2022 20:26:58 -0800 Eric Dumazet wrote:
> > > Should we always clear SG? If we want to make the assumption that
> > > non-gso skbs are never this long (like the driver did before) then
> > > we should never clear SG. If we do we risk one of the gso-generated
> > > segs will also be longer than 32 frags.
> >
> > If I read the comment (deleted in this patch), it seems the 32 limits
> > is about TSO only ?
> >
> > #warning Typhoon only supports 32 entries in its SG list for TSO, disabling TSO
> >
> > This is why I chose this implementation.
>
> Right, sort of my point - to stay true to old code we don't need to
> worry about SG ? The old code didn't..

I misread your comment.

I thought you suggested to always clear SG, regardless of GSO or not, as in

       if (skb_shinfo(skb)->nr_frags > 32) {
               if (skb_is_gso(skb))
                       features &= ~NETIF_F_GSO_MASK;
               features &= ~NETIF_F_SG;
       }


>
> > > Thought I should ask.
> > >
> > > > +     }
> > > > +     return features;
> > >
> > > return vlan_features_check(skb, features) ?
> >
> > Hmm... not sure why we duplicate vlan_features_check() &
> > vxlan_features_check() in all ndo_features_check() handlers :/
>
> I was wondering as well. I can only speculate.. :S