[PATCH net] net: napi: Make napi_defer_irqs u32

Joe Damato posted 1 patch 1 year, 5 months ago
Documentation/networking/net_cachelines/net_device.rst | 2 +-
include/linux/netdevice.h                              | 4 ++--
net/core/net-sysfs.c                                   | 6 +++++-
3 files changed, 8 insertions(+), 4 deletions(-)
[PATCH net] net: napi: Make napi_defer_irqs u32
Posted by Joe Damato 1 year, 5 months ago
In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
napi_defer_irqs was added to net_device and napi_defer_irqs_count was
added to napi_struct, both as type int.

This value never goes below zero. Change the type for both from int to
u32, and add an overflow check to sysfs to limit the value to S32_MAX.

Before this patch:

$ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
$ cat /sys/class/net/eth4/napi_defer_hard_irqs
-2147483647

After this patch:

$ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
bash: line 0: echo: write error: Numerical result out of range

Fixes: 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
Cc: stable@kernel.org
Cc: Eric Dumazet <edumazet@google.com>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 Documentation/networking/net_cachelines/net_device.rst | 2 +-
 include/linux/netdevice.h                              | 4 ++--
 net/core/net-sysfs.c                                   | 6 +++++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
index 70c4fb9d4e5c..d68f37f5b1f8 100644
--- a/Documentation/networking/net_cachelines/net_device.rst
+++ b/Documentation/networking/net_cachelines/net_device.rst
@@ -98,7 +98,7 @@ unsigned_int                        num_rx_queues
 unsigned_int                        real_num_rx_queues      -                   read_mostly         get_rps_cpu
 struct_bpf_prog*                    xdp_prog                -                   read_mostly         netif_elide_gro()
 unsigned_long                       gro_flush_timeout       -                   read_mostly         napi_complete_done
-int                                 napi_defer_hard_irqs    -                   read_mostly         napi_complete_done
+u32                                 napi_defer_hard_irqs    -                   read_mostly         napi_complete_done
 unsigned_int                        gro_max_size            -                   read_mostly         skb_gro_receive
 unsigned_int                        gro_ipv4_max_size       -                   read_mostly         skb_gro_receive
 rx_handler_func_t*                  rx_handler              read_mostly         -                   __netif_receive_skb_core
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 607009150b5f..39eafd2e2368 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -356,7 +356,7 @@ struct napi_struct {
 
 	unsigned long		state;
 	int			weight;
-	int			defer_hard_irqs_count;
+	u32			defer_hard_irqs_count;
 	unsigned long		gro_bitmask;
 	int			(*poll)(struct napi_struct *, int);
 #ifdef CONFIG_NETPOLL
@@ -2091,7 +2091,7 @@ struct net_device {
 	unsigned int		real_num_rx_queues;
 	struct netdev_rx_queue	*_rx;
 	unsigned long		gro_flush_timeout;
-	int			napi_defer_hard_irqs;
+	u32			napi_defer_hard_irqs;
 	unsigned int		gro_max_size;
 	unsigned int		gro_ipv4_max_size;
 	rx_handler_func_t __rcu	*rx_handler;
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 444f23e74f8e..b34d731524d5 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -32,6 +32,7 @@
 #ifdef CONFIG_SYSFS
 static const char fmt_hex[] = "%#x\n";
 static const char fmt_dec[] = "%d\n";
+static const char fmt_uint[] = "%u\n";
 static const char fmt_ulong[] = "%lu\n";
 static const char fmt_u64[] = "%llu\n";
 
@@ -425,6 +426,9 @@ NETDEVICE_SHOW_RW(gro_flush_timeout, fmt_ulong);
 
 static int change_napi_defer_hard_irqs(struct net_device *dev, unsigned long val)
 {
+	if (val > S32_MAX)
+		return -ERANGE;
+
 	WRITE_ONCE(dev->napi_defer_hard_irqs, val);
 	return 0;
 }
@@ -438,7 +442,7 @@ static ssize_t napi_defer_hard_irqs_store(struct device *dev,
 
 	return netdev_store(dev, attr, buf, len, change_napi_defer_hard_irqs);
 }
-NETDEVICE_SHOW_RW(napi_defer_hard_irqs, fmt_dec);
+NETDEVICE_SHOW_RW(napi_defer_hard_irqs, fmt_uint);
 
 static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
 			     const char *buf, size_t len)
-- 
2.25.1
Re: [PATCH net] net: napi: Make napi_defer_irqs u32
Posted by Eric Dumazet 1 year, 5 months ago
On Sat, Aug 31, 2024 at 1:32 PM Joe Damato <jdamato@fastly.com> wrote:
>
> In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> napi_defer_irqs was added to net_device and napi_defer_irqs_count was
> added to napi_struct, both as type int.
>
> This value never goes below zero. Change the type for both from int to
> u32, and add an overflow check to sysfs to limit the value to S32_MAX.
>
> Before this patch:
>
> $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> $ cat /sys/class/net/eth4/napi_defer_hard_irqs
> -2147483647
>
> After this patch:
>
> $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> bash: line 0: echo: write error: Numerical result out of range
>
> Fixes: 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> Cc: stable@kernel.org
> Cc: Eric Dumazet <edumazet@google.com>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---

I do not think this deserves a change to stable trees.

Signed or unsigned, what is the issue ?

Do you really need one extra bit ?

/sys/class/net/XXXXX/tx_queue_len has a similar behavior.
Re: [PATCH net] net: napi: Make napi_defer_irqs u32
Posted by Joe Damato 1 year, 5 months ago
On Mon, Sep 02, 2024 at 03:01:28PM +0200, Eric Dumazet wrote:
> On Sat, Aug 31, 2024 at 1:32 PM Joe Damato <jdamato@fastly.com> wrote:
> >
> > In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> > napi_defer_irqs was added to net_device and napi_defer_irqs_count was
> > added to napi_struct, both as type int.
> >
> > This value never goes below zero. Change the type for both from int to
> > u32, and add an overflow check to sysfs to limit the value to S32_MAX.
> >
> > Before this patch:
> >
> > $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> > $ cat /sys/class/net/eth4/napi_defer_hard_irqs
> > -2147483647
> >
> > After this patch:
> >
> > $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> > bash: line 0: echo: write error: Numerical result out of range
> >
> > Fixes: 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> > Cc: stable@kernel.org
> > Cc: Eric Dumazet <edumazet@google.com>
> > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> 
> I do not think this deserves a change to stable trees.
> 
> Signed or unsigned, what is the issue ?
> 
> Do you really need one extra bit ?
> 
> /sys/class/net/XXXXX/tx_queue_len has a similar behavior.

Sorry, Eric, I'm not following.

Are you asking me to allow u32_max for napi_defer_hard_irqs the same
way tx_queue_len does and avoid overflow that way:

include/linux/netdevice.h:      unsigned int            tx_queue_len;

dev_change_tx_queue_len(..., unsigned long new_len):

  if (new_len != (unsigned int)new_len)
          return -ERANGE;
Re: [PATCH net] net: napi: Make napi_defer_irqs u32
Posted by Joe Damato 1 year, 5 months ago
On Mon, Sep 02, 2024 at 03:01:28PM +0200, Eric Dumazet wrote:
> On Sat, Aug 31, 2024 at 1:32 PM Joe Damato <jdamato@fastly.com> wrote:
> >
> > In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> > napi_defer_irqs was added to net_device and napi_defer_irqs_count was
> > added to napi_struct, both as type int.
> >
> > This value never goes below zero. Change the type for both from int to
> > u32, and add an overflow check to sysfs to limit the value to S32_MAX.
> >
> > Before this patch:
> >
> > $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> > $ cat /sys/class/net/eth4/napi_defer_hard_irqs
> > -2147483647
> >
> > After this patch:
> >
> > $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> > bash: line 0: echo: write error: Numerical result out of range
> >
> > Fixes: 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> > Cc: stable@kernel.org
> > Cc: Eric Dumazet <edumazet@google.com>
> > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> 
> I do not think this deserves a change to stable trees.

OK, I can send any other revisions to -next, instead.
 
> Signed or unsigned, what is the issue ?
>
> Do you really need one extra bit ?

I made the maximum S32_MAX because the practical limit has always
been S32_MAX. Any larger values overflow. Keeping it at S32_MAX does
not change anything about existing behavior, which was my goal.

Would you prefer if it was U32_MAX instead?

Or are you asking me to leave it the way it is?
Re: [PATCH net] net: napi: Make napi_defer_irqs u32
Posted by Eric Dumazet 1 year, 5 months ago
On Mon, Sep 2, 2024 at 6:29 PM Joe Damato <jdamato@fastly.com> wrote:
>
> On Mon, Sep 02, 2024 at 03:01:28PM +0200, Eric Dumazet wrote:
> > On Sat, Aug 31, 2024 at 1:32 PM Joe Damato <jdamato@fastly.com> wrote:
> > >
> > > In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> > > napi_defer_irqs was added to net_device and napi_defer_irqs_count was
> > > added to napi_struct, both as type int.
> > >
> > > This value never goes below zero. Change the type for both from int to
> > > u32, and add an overflow check to sysfs to limit the value to S32_MAX.
> > >
> > > Before this patch:
> > >
> > > $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> > > $ cat /sys/class/net/eth4/napi_defer_hard_irqs
> > > -2147483647
> > >
> > > After this patch:
> > >
> > > $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> > > bash: line 0: echo: write error: Numerical result out of range
> > >
> > > Fixes: 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> > > Cc: stable@kernel.org
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > > ---
> >
> > I do not think this deserves a change to stable trees.
>
> OK, I can send any other revisions to -next, instead.
>
> > Signed or unsigned, what is the issue ?
> >
> > Do you really need one extra bit ?
>
> I made the maximum S32_MAX because the practical limit has always
> been S32_MAX. Any larger values overflow. Keeping it at S32_MAX does
> not change anything about existing behavior, which was my goal.
>
> Would you prefer if it was U32_MAX instead?
>
> Or are you asking me to leave it the way it is?

I think this would target net-next at most, please lets avoid hassles
for stable teams.
Re: [PATCH net] net: napi: Make napi_defer_irqs u32
Posted by Joe Damato 1 year, 5 months ago
On Mon, Sep 02, 2024 at 07:00:48PM +0200, Eric Dumazet wrote:
> On Mon, Sep 2, 2024 at 6:29 PM Joe Damato <jdamato@fastly.com> wrote:
> >
> > On Mon, Sep 02, 2024 at 03:01:28PM +0200, Eric Dumazet wrote:
> > > On Sat, Aug 31, 2024 at 1:32 PM Joe Damato <jdamato@fastly.com> wrote:
> > > >
> > > > In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> > > > napi_defer_irqs was added to net_device and napi_defer_irqs_count was
> > > > added to napi_struct, both as type int.
> > > >
> > > > This value never goes below zero. Change the type for both from int to
> > > > u32, and add an overflow check to sysfs to limit the value to S32_MAX.
> > > >
> > > > Before this patch:
> > > >
> > > > $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> > > > $ cat /sys/class/net/eth4/napi_defer_hard_irqs
> > > > -2147483647
> > > >
> > > > After this patch:
> > > >
> > > > $ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
> > > > bash: line 0: echo: write error: Numerical result out of range
> > > >
> > > > Fixes: 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> > > > Cc: stable@kernel.org
> > > > Cc: Eric Dumazet <edumazet@google.com>
> > > > Suggested-by: Jakub Kicinski <kuba@kernel.org>
> > > > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > > > ---
> > >
> > > I do not think this deserves a change to stable trees.
> >
> > OK, I can send any other revisions to -next, instead.
> >
> > > Signed or unsigned, what is the issue ?
> > >
> > > Do you really need one extra bit ?
> >
> > I made the maximum S32_MAX because the practical limit has always
> > been S32_MAX. Any larger values overflow. Keeping it at S32_MAX does
> > not change anything about existing behavior, which was my goal.
> >
> > Would you prefer if it was U32_MAX instead?
> >
> > Or are you asking me to leave it the way it is?
> 
> I think this would target net-next at most, please lets avoid hassles
> for stable teams.

Sure, that's fine with me.

I'm just not sure what you meant by your comment about the extra
bit and what you are asking me to make the maximum limit? I have no
preference.

I just want to prevent overflow and then make the per-NAPI stuff
compatible with existing sysfs code as much as possible.