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

[Jakub Kicinski <kuba@kernel.org>]

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?

> ....
> 
> 
> +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?

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],