Re: [PATCH net-next v12 2/4] ethtool: provide customized dim profile management

[Jakub Kicinski <kuba@kernel.org>]

On Sat,  4 May 2024 14:44:45 +0800 Heng Qi wrote:
> @@ -1325,6 +1354,8 @@ operations:
>              - tx-aggr-max-bytes
>              - tx-aggr-max-frames
>              - tx-aggr-time-usecs
> +            - rx-profile
> +            - tx-profile
>        dump: *coalesce-get-op
>      -
>        name: coalesce-set

set probably needs to get the new attributes, too?

>  Request is rejected if it attributes declared as unsupported by driver (i.e.
> diff --git a/include/linux/dim.h b/include/linux/dim.h
> index 43398f5eade2..d848b790ca50 100644
> --- a/include/linux/dim.h
> +++ b/include/linux/dim.h
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
> +#include <linux/netdevice.h>

looks unnecessary, you just need a forward declaration of 
struct net_device, no?

> diff --git a/lib/dim/net_dim.c b/lib/dim/net_dim.c
> index 67d5beb34dc3..b3e01619f929 100644
> --- a/lib/dim/net_dim.c
> +++ b/lib/dim/net_dim.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/dim.h>
> +#include <linux/rtnetlink.h>
>  
>  /*
>   * Net DIM profiles:
> @@ -95,6 +96,76 @@ net_dim_get_def_tx_moderation(u8 cq_period_mode)
>  }
>  EXPORT_SYMBOL(net_dim_get_def_tx_moderation);
>  
> +int net_dim_init_irq_moder(struct net_device *dev, u8 profile_flags,
> +			   u8 coal_flags, u8 rx_mode, u8 tx_mode,
> +			   void (*rx_dim_work)(struct work_struct *work),
> +			   void (*tx_dim_work)(struct work_struct *work))
> +{
> +	struct dim_cq_moder *rxp = NULL, *txp;
> +	struct dim_irq_moder *moder;
> +	int len;
> +
> +	dev->irq_moder = kzalloc(sizeof(*dev->irq_moder), GFP_KERNEL);
> +	if (!dev->irq_moder)
> +		goto err_moder;

return the error directly here, no need to goto

> +	moder = dev->irq_moder;
> +	len = NET_DIM_PARAMS_NUM_PROFILES * sizeof(*moder->rx_profile);
> +
> +	moder->coal_flags = coal_flags;
> +	moder->profile_flags = profile_flags;
> +
> +	if (profile_flags & DIM_PROFILE_RX) {
> +		moder->rx_dim_work = rx_dim_work;
> +		WRITE_ONCE(moder->dim_rx_mode, rx_mode);

why WRITE_ONCE()? The structure can't be used, yet

> +		rxp = kmemdup(rx_profile[rx_mode], len, GFP_KERNEL);
> +		if (!rxp)
> +			goto err_rx_profile;

name the labels after the target, please, not the source

> +		rcu_assign_pointer(moder->rx_profile, rxp);
> +	}

> +static int ethnl_update_profile(struct net_device *dev,
> +				struct dim_cq_moder __rcu **dst,
> +				const struct nlattr *nests,
> +				struct netlink_ext_ack *extack)

> +	rcu_assign_pointer(*dst, new_profile);
> +	kfree_rcu(old_profile, rcu);
> +
> +	return 0;

Don't we need to inform DIM somehow that profile has switched
and it should restart itself?