| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zsjt9XwIiqAVk0Et@LQ3V64L9R2>
Date: Fri, 23 Aug 2024 21:15:49 +0100
From: Joe Damato <jdamato@...tly.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: netdev@...r.kernel.org, amritha.nambiar@...el.com,
sridhar.samudrala@...el.com, sdf@...ichev.me, peter@...eblog.net,
m2shafiei@...terloo.ca, bjorn@...osinc.com, hch@...radead.org,
willy@...radead.org, willemdebruijn.kernel@...il.com,
skhawaja@...gle.com, kuba@...nel.org,
Martin Karsten <mkarsten@...terloo.ca>,
"David S. Miller" <davem@...emloft.net>,
Paolo Abeni <pabeni@...hat.com>, Jiri Pirko <jiri@...nulli.us>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Lorenzo Bianconi <lorenzo@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Breno Leitao <leitao@...ian.org>,
Johannes Berg <johannes.berg@...el.com>,
Heiner Kallweit <hkallweit1@...il.com>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 1/6] net: Add sysfs parameter irq_suspend_timeout
On Fri, Aug 23, 2024 at 07:39:56PM +0200, Eric Dumazet wrote:
> On Fri, Aug 23, 2024 at 7:31 PM Joe Damato <jdamato@...tly.com> wrote:
> >
> > From: Martin Karsten <mkarsten@...terloo.ca>
> >
> > This patch doesn't change any behavior but prepares the code for other
> > changes in the following commits which use irq_suspend_timeout as a
> > timeout for IRQ suspension.
> >
> > Signed-off-by: Martin Karsten <mkarsten@...terloo.ca>
> > Co-developed-by: Joe Damato <jdamato@...tly.com>
> > Signed-off-by: Joe Damato <jdamato@...tly.com>
> > Tested-by: Joe Damato <jdamato@...tly.com>
> > Tested-by: Martin Karsten <mkarsten@...terloo.ca>
> > ---
> > rfc -> v1:
> > - Removed napi.rst documentation from this patch; added to patch 6.
> >
> > include/linux/netdevice.h | 2 ++
> > net/core/dev.c | 3 ++-
> > net/core/net-sysfs.c | 18 ++++++++++++++++++
> > 3 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 0ef3eaa23f4b..31867bb2ff65 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1857,6 +1857,7 @@ enum netdev_reg_state {
> > * @gro_flush_timeout: timeout for GRO layer in NAPI
> > * @napi_defer_hard_irqs: If not zero, provides a counter that would
> > * allow to avoid NIC hard IRQ, on busy queues.
> > + * @irq_suspend_timeout: IRQ suspension timeout
> > *
> > * @rx_handler: handler for received packets
> > * @rx_handler_data: XXX: need comments on this one
> > @@ -2060,6 +2061,7 @@ struct net_device {
> > struct netdev_rx_queue *_rx;
> > unsigned long gro_flush_timeout;
> > int napi_defer_hard_irqs;
> > + unsigned long irq_suspend_timeout;
> > unsigned int gro_max_size;
> > unsigned int gro_ipv4_max_size;
> > rx_handler_func_t __rcu *rx_handler;
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index e7260889d4cb..3bf325ec25a3 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -11945,6 +11945,7 @@ static void __init net_dev_struct_check(void)
> > CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, _rx);
> > CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, gro_flush_timeout);
> > CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, napi_defer_hard_irqs);
> > + CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, irq_suspend_timeout);
> > CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, gro_max_size);
> > CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, gro_ipv4_max_size);
> > CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, rx_handler);
> > @@ -11956,7 +11957,7 @@ static void __init net_dev_struct_check(void)
> > #ifdef CONFIG_NET_XGRESS
> > CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, tcx_ingress);
> > #endif
> > - CACHELINE_ASSERT_GROUP_SIZE(struct net_device, net_device_read_rx, 104);
> > + CACHELINE_ASSERT_GROUP_SIZE(struct net_device, net_device_read_rx, 112);
> > }
> >
> > /*
> > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> > index 0e2084ce7b75..fb6f3327310f 100644
> > --- a/net/core/net-sysfs.c
> > +++ b/net/core/net-sysfs.c
> > @@ -440,6 +440,23 @@ static ssize_t napi_defer_hard_irqs_store(struct device *dev,
> > }
> > NETDEVICE_SHOW_RW(napi_defer_hard_irqs, fmt_dec);
> >
> > +static int change_irq_suspend_timeout(struct net_device *dev, unsigned long val)
> > +{
> > + WRITE_ONCE(dev->irq_suspend_timeout, val);
> > + return 0;
> > +}
> > +
> > +static ssize_t irq_suspend_timeout_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + if (!capable(CAP_NET_ADMIN))
> > + return -EPERM;
> > +
> > + return netdev_store(dev, attr, buf, len, change_irq_suspend_timeout);
> > +}
> > +NETDEVICE_SHOW_RW(irq_suspend_timeout, fmt_ulong);
> > +
> > static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
> > const char *buf, size_t len)
> > {
> > @@ -664,6 +681,7 @@ static struct attribute *net_class_attrs[] __ro_after_init = {
> > &dev_attr_tx_queue_len.attr,
> > &dev_attr_gro_flush_timeout.attr,
> > &dev_attr_napi_defer_hard_irqs.attr,
> > + &dev_attr_irq_suspend_timeout.attr,
> > &dev_attr_phys_port_id.attr,
> > &dev_attr_phys_port_name.attr,
> > &dev_attr_phys_switch_id.attr,
> > --
> > 2.25.1
>
>
> Please no more per-device sysfs entry, shared by all the users of the device.
>
> Let's not repeat past mistakes.
>
> Nowadays, we need/want per receive-queue tuning, preferably set with netlink.
Thanks for the feedback, Eric. We appreciate your consideration
and guidance.
May we ask what your thoughts are, overall, about getting a
mechanism like this accepted?
We want to make sure that this, in principle, is acceptable before
iterating further and going down the path of netlink, if required.
On the specific netlink bit in your comment, we agree in principle,
however:
1. Our code integrates directly with existing sysfs parameters.
If we make our parameter settable via netlink, but the others
remain as sysfs parameters then the interface for users
becomes a bit cumbersome.
And, so the urge will be to move all parameters to netlink
for ease of use for the user.
As we mentioned in our cover letter: we agree that doing so is
a good idea, but we hope to convince you (and the other
maintainers) that the netlink work can come later as a separate
change which affects the existing parameters we integrate with
as well as the parameter we are introducing, at the same time.
2. The proposed mechanism yields a substantial performance and
efficiency improvement which would be valuable to users. It would
be unfortunate to block until all of this could be moved to
netlink, first.
3. While adding a new sysfs parameter does affect the ABI
permanently, it doesn't prevent us from making these parameters
per-NAPI in the future. This series adds strength to the argument
that these parameters should be per-NAPI because our results show
the impact these parameters can have on network processing very
clearly in a range of scenarios.
We appreciate your thoughts on the above as we want to ensure we
are moving in the right direction.
Powered by blists - more mailing lists