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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240507195752.7275cb63@kernel.org>
Date: Tue, 7 May 2024 19:57:52 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Heng Qi <hengqi@...ux.alibaba.com>
Cc: netdev@...r.kernel.org, virtualization@...ts.linux.dev, "David S .
 Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>, Eric
 Dumazet <edumazet@...gle.com>, Jason Wang <jasowang@...hat.com>, "Michael S
 . Tsirkin" <mst@...hat.com>, Brett Creeley <bcreeley@....com>, Ratheesh
 Kannoth <rkannoth@...vell.com>, Alexander Lobakin
 <aleksander.lobakin@...el.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Tal
 Gilboa <talgi@...dia.com>, Jonathan Corbet <corbet@....net>,
 linux-doc@...r.kernel.org, Maxime Chevallier
 <maxime.chevallier@...tlin.com>, Jiri Pirko <jiri@...nulli.us>, Paul
 Greenwalt <paul.greenwalt@...el.com>, Ahmed Zaki <ahmed.zaki@...el.com>,
 Vladimir Oltean <vladimir.oltean@....com>, Kory Maincent
 <kory.maincent@...tlin.com>, Andrew Lunn <andrew@...n.ch>,
 justinstitt@...gle.com, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH net-next v12 2/4] ethtool: provide customized dim
 profile management

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?