Re: [PATCH net-next 3/5] net: napi: Make gro_flush_timeout per-NAPI

Eric Dumazet
2024-08-29T13:48:20Z

On Thu, Aug 29, 2024 at 3:13 PM Joe Damato <jdamato@fastly.com> wrote:
>
> Allow per-NAPI gro_flush_timeout setting.
>
> The existing sysfs parameter is respected; writes to sysfs will write to
> all NAPI structs for the device and the net_device gro_flush_timeout
> field. Reads from sysfs will read from the net_device field.
>
> The ability to set gro_flush_timeout on specific NAPI instances will be
> added in a later commit, via netdev-genl.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> Reviewed-by: Martin Karsten <mkarsten@uwaterloo.ca>
> Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
> ---
> include/linux/netdevice.h | 26 ++++++++++++++++++++++++++
> net/core/dev.c | 32 ++++++++++++++++++++++++++++----
> net/core/net-sysfs.c | 2 +-
> 3 files changed, 55 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7d53380da4c0..d00024d9f857 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -372,6 +372,7 @@ struct napi_struct {
> int rx_count; /* length of rx_list */
> unsigned int napi_id;
> int defer_hard_irqs;
> + unsigned long gro_flush_timeout;
> struct hrtimer timer;
> struct task_struct *thread;
> /* control-path-only fields follow */
> @@ -557,6 +558,31 @@ void napi_set_defer_hard_irqs(struct napi_struct *n, int defer);
> */
> void netdev_set_defer_hard_irqs(struct net_device *netdev, int defer);
>

Same remark : dev->gro_flush_timeout is no longer read in the fast path.

Please move gro_flush_timeout out of net_device_read_txrx and update
Documentation/networking/net_cachelines/net_device.rst

> +/**
> + * napi_get_gro_flush_timeout - get the gro_flush_timeout
> + * @n: napi struct to get the gro_flush_timeout from
> + *
> + * Returns the per-NAPI value of the gro_flush_timeout field.
> + */
> +unsigned long napi_get_gro_flush_timeout(const struct napi_struct *n);
> +
> +/**
> + * napi_set_gro_flush_timeout - set the gro_flush_timeout for a napi
> + * @n: napi struct to set the gro_flush_timeout
> + * @timeout: timeout value to set
> + *
> + * napi_set_gro_flush_timeout sets the per-NAPI gro_flush_timeout
> + */
> +void napi_set_gro_flush_timeout(struct napi_struct *n, unsigned long timeout);
> +
> +/**
> + * netdev_set_gro_flush_timeout - set gro_flush_timeout for all NAPIs of a netdev
> + * @netdev: the net_device for which all NAPIs will have their gro_flush_timeout set
> + * @timeout: the timeout value to set
> + */
> +void netdev_set_gro_flush_timeout(struct net_device *netdev,
> + unsigned long timeout);
> +
> /**
> * napi_complete_done - NAPI processing complete
> * @n: NAPI context
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f7baff0da057..3f7cb1085efa 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6234,6 +6234,29 @@ void netdev_set_defer_hard_irqs(struct net_device *netdev, int defer)
> }
> EXPORT_SYMBOL_GPL(netdev_set_defer_hard_irqs);
>
> +unsigned long napi_get_gro_flush_timeout(const struct napi_struct *n)
> +{
> + return READ_ONCE(n->gro_flush_timeout);
> +}
> +EXPORT_SYMBOL_GPL(napi_get_gro_flush_timeout);
> +
> +void napi_set_gro_flush_timeout(struct napi_struct *n, unsigned long timeout)
> +{
> + WRITE_ONCE(n->gro_flush_timeout, timeout);
> +}
> +EXPORT_SYMBOL_GPL(napi_set_gro_flush_timeout);
> +
> +void netdev_set_gro_flush_timeout(struct net_device *netdev,
> + unsigned long timeout)
> +{
> + struct napi_struct *napi;
> +
> + WRITE_ONCE(netdev->gro_flush_timeout, timeout);
> + list_for_each_entry(napi, &netdev->napi_list, dev_list)
> + napi_set_gro_flush_timeout(napi, timeout);
> +}
> +EXPORT_SYMBOL_GPL(netdev_set_gro_flush_timeout);
> +
> bool napi_complete_done(struct napi_struct *n, int work_done)
> {
> unsigned long flags, val, new, timeout = 0;
> @@ -6251,12 +6274,12 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
>
> if (work_done) {
> if (n->gro_bitmask)
> - timeout = READ_ONCE(n->dev->gro_flush_timeout);
> + timeout = napi_get_gro_flush_timeout(n);
> n->defer_hard_irqs_count = napi_get_defer_hard_irqs(n);
> }
> if (n->defer_hard_irqs_count > 0) {
> n->defer_hard_irqs_count--;
> - timeout = READ_ONCE(n->dev->gro_flush_timeout);
> + timeout = napi_get_gro_flush_timeout(n);
> if (timeout)
> ret = false;
> }
> @@ -6391,7 +6414,7 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock,
>
> if (flags & NAPI_F_PREFER_BUSY_POLL) {
> napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> - timeout = READ_ONCE(napi->dev->gro_flush_timeout);
> + timeout = napi_get_gro_flush_timeout(napi);
> if (napi->defer_hard_irqs_count && timeout) {
> hrtimer_start(&napi->timer, ns_to_ktime(timeout), HRTIMER_MODE_REL_PINNED);
> skip_schedule = true;
> @@ -6673,6 +6696,7 @@ void netif_napi_add_weight(struct net_device *dev, struct napi_struct *napi,
> hrtimer_init(&napi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> napi->timer.function = napi_watchdog;
> napi_set_defer_hard_irqs(napi, READ_ONCE(dev->napi_defer_hard_irqs));
> + napi_set_gro_flush_timeout(napi, READ_ONCE(dev->gro_flush_timeout));
> init_gro_hash(napi);
> napi->skb = NULL;
> INIT_LIST_HEAD(&napi->rx_list);
> @@ -11054,7 +11078,7 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev)
> WARN_ON(dev->reg_state == NETREG_REGISTERED);
>
> if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
> - dev->gro_flush_timeout = 20000;
> + netdev_set_gro_flush_timeout(dev, 20000);
> netdev_set_defer_hard_irqs(dev, 1);
> }
> }
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 8272f0144d81..ff545a422b1f 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -408,7 +408,7 @@ NETDEVICE_SHOW_RW(tx_queue_len, fmt_dec);
>
> static int change_gro_flush_timeout(struct net_device *dev, unsigned long val)
> {
> - WRITE_ONCE(dev->gro_flush_timeout, val);
> + netdev_set_gro_flush_timeout(dev, val);
> return 0;
> }
>
> --
> 2.25.1
>