| Message ID | 1712664204-83147-5-git-send-email-hengqi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: provide the dim profile fine-tuning channel | expand |
On Tue, 9 Apr 2024 20:03:24 +0800 Heng Qi wrote: > + /* DIM profile list */ > + struct dim_cq_moder rx_eqe_conf[NET_DIM_PARAMS_NUM_PROFILES]; Can you please wrap this into a structure with other necessary information and add a pointer in struct net_device instead. What's the point of every single driver implementing the same boilerplate memcpy() in its get_coalesce / set_coalesce callbacks?
在 2024/4/10 上午9:40, Jakub Kicinski 写道: > On Tue, 9 Apr 2024 20:03:24 +0800 Heng Qi wrote: >> + /* DIM profile list */ >> + struct dim_cq_moder rx_eqe_conf[NET_DIM_PARAMS_NUM_PROFILES]; > Can you please wrap this into a structure with other necessary > information and add a pointer in struct net_device instead. > > What's the point of every single driver implementing the same > boilerplate memcpy() in its get_coalesce / set_coalesce callbacks? The point is that the driver may check whether the user has set parameters that it does not want. For example, virtio may not want the modification of comps, and ice/idpf may not want the modification of comps and pkts. But you inspired me to think from another perspective. If parameters are placed in netdevice, the goal of user modification is to modify the profile of netdevice, and the driver can obtain its own target parameters from it as needed. Do you like this? In addition, if the netdevice way is preferred, I would like to confirm whether we still continue the "ethtool -C" way, that is, complete the modification of netdevice profile in __ethnl_set_coalesce()? Thanks.
On Wed, 10 Apr 2024 11:09:16 +0800 Heng Qi wrote: > The point is that the driver may check whether the user has set > parameters that it does not want. > For example, virtio may not want the modification of comps, and ice/idpf > may not want the modification > of comps and pkts. If it's simply about the fields, not the range of values, flags of what's supported would suffice. If we need more complicated checks you can treat the driver callback as a validation callback, and when driver returns success - copy in the core. > But you inspired me to think from another perspective. If parameters are > placed in netdevice, the > goal of user modification is to modify the profile of netdevice, and the > driver can obtain its own > target parameters from it as needed. Do you like this? Yes, IIUC. > In addition, if the netdevice way is preferred, I would like to confirm > whether we still continue the > "ethtool -C" way, that is, complete the modification of netdevice > profile in __ethnl_set_coalesce()? That'd be great. If the driver validation is complex we can keep some form of the SET callback. But we definitely don't need to extend the GET callback since core will have the values, and can return them to the user.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index a64656e..03840e9 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -57,6 +57,8 @@ #define VIRTNET_DRIVER_VERSION "1.0.0" +static const struct dim_cq_moder rx_eqe_conf[] = NET_DIM_RX_EQE_PROFILES; + static const unsigned long guest_offloads[] = { VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, @@ -335,6 +337,9 @@ struct virtnet_info { struct virtnet_interrupt_coalesce intr_coal_tx; struct virtnet_interrupt_coalesce intr_coal_rx; + /* DIM profile list */ + struct dim_cq_moder rx_eqe_conf[NET_DIM_PARAMS_NUM_PROFILES]; + unsigned long guest_offloads; unsigned long guest_offloads_capable; @@ -3584,7 +3589,7 @@ static void virtnet_rx_dim_work(struct work_struct *work) if (!rq->dim_enabled) continue; - update_moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix); + update_moder = vi->rx_eqe_conf[dim->profile_ix]; if (update_moder.usec != rq->intr_coal.max_usecs || update_moder.pkts != rq->intr_coal.max_packets) { err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, qnum, @@ -3627,6 +3632,27 @@ static int virtnet_should_update_vq_weight(int dev_flags, int weight, return 0; } +static int virtnet_update_profile(struct virtnet_info *vi, + struct kernel_ethtool_coalesce *kc) +{ + int i; + + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { + for (i = 0; i < NET_DIM_PARAMS_NUM_PROFILES; i++) + if (kc->rx_eqe_profile[i].comps) + return -EINVAL; + } else { + return -EOPNOTSUPP; + } + + for (i = 0; i < NET_DIM_PARAMS_NUM_PROFILES; i++) { + vi->rx_eqe_conf[i].usec = kc->rx_eqe_profile[i].usec; + vi->rx_eqe_conf[i].pkts = kc->rx_eqe_profile[i].pkts; + } + + return 0; +} + static int virtnet_set_coalesce(struct net_device *dev, struct ethtool_coalesce *ec, struct kernel_ethtool_coalesce *kernel_coal, @@ -3653,6 +3679,10 @@ static int virtnet_set_coalesce(struct net_device *dev, } } + ret = virtnet_update_profile(vi, kernel_coal); + if (ret) + return ret; + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) ret = virtnet_send_notf_coal_cmds(vi, ec); else @@ -3689,6 +3719,10 @@ static int virtnet_get_coalesce(struct net_device *dev, ec->tx_max_coalesced_frames = 1; } + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) + memcpy(kernel_coal->rx_eqe_profile, vi->rx_eqe_conf, + sizeof(vi->rx_eqe_conf)); + return 0; } @@ -3868,7 +3902,8 @@ static int virtnet_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info) static const struct ethtool_ops virtnet_ethtool_ops = { .supported_coalesce_params = ETHTOOL_COALESCE_MAX_FRAMES | - ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_USE_ADAPTIVE_RX, + ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_USE_ADAPTIVE_RX | + ETHTOOL_COALESCE_RX_EQE_PROFILE, .get_drvinfo = virtnet_get_drvinfo, .get_link = ethtool_op_get_link, .get_ringparam = virtnet_get_ringparam, @@ -4433,6 +4468,8 @@ static void virtnet_dim_init(struct virtnet_info *vi) INIT_WORK(&vi->rq[i].dim.work, virtnet_rx_dim_work); vi->rq[i].dim.mode = DIM_CQ_PERIOD_MODE_START_FROM_EQE; } + + memcpy(vi->rx_eqe_conf, rx_eqe_conf, sizeof(vi->rx_eqe_conf)); } static int virtnet_alloc_queues(struct virtnet_info *vi)
Virtio-net has different types of back-end device implementations. In order to effectively optimize the dim library's gains for different device implementations, let's use the new interface params to fine-tune the profile list. Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> --- drivers/net/virtio_net.c | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)