| 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
| ||
|
Date: Thu, 25 Apr 2024 00:49:26 +0800
From: Heng Qi <hengqi@...ux.alibaba.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, virtualization@...ts.linux.dev,
"David S . Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.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"
<justinstitt@...gle.com>
Subject: Re: [PATCH net-next v9 2/4] ethtool: provide customized dim profile
management
在 2024/4/25 上午12:18, Jakub Kicinski 写道:
> On Wed, 24 Apr 2024 21:41:55 +0800 Heng Qi wrote:
>> +struct dim_irq_moder {
>> + /* See DIM_PROFILE_* */
>> + u8 profile_flags;
>> +
>> + /* See DIM_COALESCE_* for Rx and Tx */
>> + u8 coal_flags;
>> +
>> + /* Rx DIM period count mode: CQE or EQE */
>> + u8 dim_rx_mode;
>> +
>> + /* Tx DIM period count mode: CQE or EQE */
>> + u8 dim_tx_mode;
>> +
>> + /* DIM profile list for Rx */
>> + struct dim_cq_moder *rx_profile;
>> +
>> + /* DIM profile list for Tx */
>> + struct dim_cq_moder *tx_profile;
>> +
>> + /* Rx DIM worker function scheduled by net_dim() */
>> + void (*rx_dim_work)(struct work_struct *work);
>> +
>> + /* Tx DIM worker function scheduled by net_dim() */
>> + void (*tx_dim_work)(struct work_struct *work);
>> +};
>> +
>>
>> .....
>>
>>
>> + .ndo_init_irq_moder = virtnet_init_irq_moder,
> The init callback mostly fills in static data, can we not
> declare the driver information statically and move the init
> code into the core?
Now the init callback is used as following
In dim.c:
+int net_dim_init_irq_moder(struct net_device *dev)
+{
+ if (dev->netdev_ops && dev->netdev_ops->ndo_init_irq_moder)
+ return dev->netdev_ops->ndo_init_irq_moder(dev);
+
+ return 0;
+}
+EXPORT_SYMBOL(net_dim_init_irq_moder);
In dev.c
@@ -10258,6 +10259,10 @@ int register_netdevice(struct net_device *dev)
if (ret)
return ret;
+ ret = net_dim_init_irq_moder(dev);
+ if (ret)
+ return ret;
+
spin_lock_init(&dev->addr_list_lock);
netdev_set_addr_lockdep_class(dev);
The collected flags, mode, and work must obtain driver-specific
values from the driver. If I'm not wrong, you don't want an interface
like .ndo_init_irq_moder, but instead provide a generic interface in
dim.c (e.g. net_dim_init_irq_moder() with parameters dev and struct
dim_irq_moder or separate flags,mode,work). Then this func is called
by the driver in the probe phase?
>> ....
>>
>>
>> +static int virtnet_init_irq_moder(struct net_device *dev)
>> +{
>> + struct virtnet_info *vi = netdev_priv(dev);
>> + struct dim_irq_moder *moder;
>> + int len;
>> +
>> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL))
>> + return 0;
>> +
>> + dev->irq_moder = kzalloc(sizeof(*dev->irq_moder), GFP_KERNEL);
>> + if (!dev->irq_moder)
>> + goto err_moder;
>> +
>> + moder = dev->irq_moder;
>> + len = NET_DIM_PARAMS_NUM_PROFILES * sizeof(*moder->rx_profile);
>> +
>> + moder->profile_flags |= DIM_PROFILE_RX;
>> + moder->coal_flags |= DIM_COALESCE_USEC | DIM_COALESCE_PKTS;
>> + moder->dim_rx_mode = DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>> +
>> + moder->rx_dim_work = virtnet_rx_dim_work;
>> +
>> + moder->rx_profile = kmemdup(dim_rx_profile[moder->dim_rx_mode],
>> + len, GFP_KERNEL);
>> + if (!moder->rx_profile)
>> + goto err_profile;
>> +
>> + return 0;
>> +
>> +err_profile:
>> + kfree(moder);
>> +err_moder:
>> + return -ENOMEM;
>> +}
>> +
>>
>> ......
>>
>> +void net_dim_setting(struct net_device *dev, struct dim *dim, bool is_tx)
>> +{
>> + struct dim_irq_moder *irq_moder = dev->irq_moder;
>> +
>> + if (!irq_moder)
>> + return;
>> +
>> + if (is_tx) {
>> + INIT_WORK(&dim->work, irq_moder->tx_dim_work);
>> + dim->mode = irq_moder->dim_tx_mode;
>> + return;
>> + }
>> +
>> + INIT_WORK(&dim->work, irq_moder->rx_dim_work);
>> + dim->mode = irq_moder->dim_rx_mode;
>> +}
>>
>> .....
>>
>> + for (i = 0; i < vi->max_queue_pairs; i++)
>> + net_dim_setting(vi->dev, &vi->rq[i].dim, false);
>>
>> .....
>>
>> ETHTOOL_A_COALESCE_TX_AGGR_MAX_FRAMES, /* u32 */
>>
>> ETHTOOL_A_COALESCE_TX_AGGR_TIME_USECS, /* u32 */
>>
>> + /* nest - _A_PROFILE_IRQ_MODERATION */
>>
>> + ETHTOOL_A_COALESCE_RX_PROFILE,
>>
>> + /* nest - _A_PROFILE_IRQ_MODERATION */
>> + ETHTOOL_A_COALESCE_TX_PROFILE,
>>
>> ......
>>
>>
>> Almost clear implementation, but the only problem is when I want to
>> reuse "ethtool -C" and append ETHTOOL_A_COALESCE_RX_PROFILE
>> and ETHTOOL_A_COALESCE_TX_PROFILE, *ethnl_set_coalesce_validate()
>> will report an error because there are no ETHTOOL_COALESCE_RX_PROFILE
>> and ETHTOOL_COALESCE_TX_PROFILE, because they are replaced by
>> DIM_PROFILE_RX and DIM_PROFILE_TX in the field profile_flags.*
> I see.
>
>> Should I reuse ETHTOOL_COALESCE_RX_PROFILE and
>> ETHTOOL_A_COALESCE_TX_PROFILE in ethtool_ops->.supported_coalesce_params
>> and remove the field profile_flags from struct dim_irq_moder?
>> Or let ethnl_set_coalesce_validate not verify these two flags?
>> Is there a better solution?
> Maybe create the bits but automatically add them for the driver?
Ok. I think it works.
Thanks!
> diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c
> index 83112c1a71ae..56777d36f7f1 100644
> --- a/net/ethtool/coalesce.c
> +++ b/net/ethtool/coalesce.c
> @@ -243,6 +243,8 @@ ethnl_set_coalesce_validate(struct ethnl_req_info *req_info,
>
> /* make sure that only supported parameters are present */
> supported_params = ops->supported_coalesce_params;
> + if (dev->moder->coal_flags ...)
> + supported_params |= ETHTOOL_COALESCE_...;
> for (a = ETHTOOL_A_COALESCE_RX_USECS; a < __ETHTOOL_A_COALESCE_CNT; a++)
> if (tb[a] && !(supported_params & attr_to_mask(a))) {
> NL_SET_ERR_MSG_ATTR(info->extack, tb[a],
Powered by blists - more mailing lists