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