On Mon, Nov 20, 2023 at 8:38 PM Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> wrote: > > By comparing the traffic information in the complete napi processes, > let the virtio-net driver automatically adjust the coalescing > moderation parameters of each receive queue. > > Signed-off-by: Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> > --- > v2->v3: > - Some minor modifications. > > v1->v2: > - Improved the judgment of dim switch conditions. > - Cancel the work when vq reset. > > drivers/net/virtio_net.c | 191 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 169 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 69fe09e99b3c..bc32d5aae005 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -19,6 +19,7 @@ > #include <linux/average.h> > #include <linux/filter.h> > #include <linux/kernel.h> > +#include <linux/dim.h> > #include <net/route.h> > #include <net/xdp.h> > #include <net/net_failover.h> > @@ -172,6 +173,17 @@ struct receive_queue { > > struct virtnet_rq_stats stats; > > + /* The number of rx notifications */ > + u16 calls; > + > + /* Is dynamic interrupt moderation enabled? */ > + bool dim_enabled; > + > + /* Dynamic Interrupt Moderation */ > + struct dim dim; > + > + u32 packets_in_napi; > + > struct virtnet_interrupt_coalesce intr_coal; > > /* Chain pages by the private ptr. */ > @@ -305,6 +317,9 @@ struct virtnet_info { > u8 duplex; > u32 speed; > > + /* Is rx dynamic interrupt moderation enabled? */ > + bool rx_dim_enabled; > + > /* Interrupt coalescing settings */ > struct virtnet_interrupt_coalesce intr_coal_tx; > struct virtnet_interrupt_coalesce intr_coal_rx; > @@ -2001,6 +2016,7 @@ static void skb_recv_done(struct virtqueue *rvq) > struct virtnet_info *vi = rvq->vdev->priv; > struct receive_queue *rq = &vi->rq[vq2rxq(rvq)]; > > + rq->calls++; > virtqueue_napi_schedule(&rq->napi, rvq); > } > > @@ -2141,6 +2157,26 @@ static void virtnet_poll_cleantx(struct receive_queue *rq) > } > } > > +static void virtnet_rx_dim_work(struct work_struct *work); > + > +static void virtnet_rx_dim_update(struct virtnet_info *vi, struct receive_queue *rq) > +{ > + struct dim_sample cur_sample = {}; > + > + if (!rq->packets_in_napi) > + return; > + > + u64_stats_update_begin(&rq->stats.syncp); > + dim_update_sample(rq->calls, > + u64_stats_read(&rq->stats.packets), > + u64_stats_read(&rq->stats.bytes), > + &cur_sample); > + u64_stats_update_end(&rq->stats.syncp); > + > + net_dim(&rq->dim, cur_sample); > + rq->packets_in_napi = 0; > +} > + > static int virtnet_poll(struct napi_struct *napi, int budget) > { > struct receive_queue *rq = > @@ -2149,17 +2185,22 @@ static int virtnet_poll(struct napi_struct *napi, int budget) > struct send_queue *sq; > unsigned int received; > unsigned int xdp_xmit = 0; > + bool napi_complete; > > virtnet_poll_cleantx(rq); > > received = virtnet_receive(rq, budget, &xdp_xmit); > + rq->packets_in_napi += received; > > if (xdp_xmit & VIRTIO_XDP_REDIR) > xdp_do_flush(); > > /* Out of packets? */ > - if (received < budget) > - virtqueue_napi_complete(napi, rq->vq, received); > + if (received < budget) { > + napi_complete = virtqueue_napi_complete(napi, rq->vq, received); > + if (napi_complete && rq->dim_enabled) > + virtnet_rx_dim_update(vi, rq); > + } > > if (xdp_xmit & VIRTIO_XDP_TX) { > sq = virtnet_xdp_get_sq(vi); > @@ -2179,6 +2220,7 @@ static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index) > virtnet_napi_tx_disable(&vi->sq[qp_index].napi); > napi_disable(&vi->rq[qp_index].napi); > xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq); > + cancel_work_sync(&vi->rq[qp_index].dim.work); > } > > static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index) > @@ -2196,6 +2238,9 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index) > if (err < 0) > goto err_xdp_reg_mem_model; > > + INIT_WORK(&vi->rq[qp_index].dim.work, virtnet_rx_dim_work); > + vi->rq[qp_index].dim.mode = DIM_CQ_PERIOD_MODE_START_FROM_EQE; So in V2, you explained it can be done here but I want to know why it must be done here. For example, the refill_work is initialized in alloc_queues(). > + > virtnet_napi_enable(vi->rq[qp_index].vq, &vi->rq[qp_index].napi); > virtnet_napi_tx_enable(vi, vi->sq[qp_index].vq, &vi->sq[qp_index].napi); > > @@ -2393,8 +2438,10 @@ static int virtnet_rx_resize(struct virtnet_info *vi, > > qindex = rq - vi->rq; > > - if (running) > + if (running) { > napi_disable(&rq->napi); > + cancel_work_sync(&rq->dim.work); > + } > > err = virtqueue_resize(rq->vq, ring_num, virtnet_rq_free_unused_buf); > if (err) > @@ -2403,8 +2450,10 @@ static int virtnet_rx_resize(struct virtnet_info *vi, > if (!try_fill_recv(vi, rq, GFP_KERNEL)) > schedule_delayed_work(&vi->refill, 0); > > - if (running) > + if (running) { > + INIT_WORK(&rq->dim.work, virtnet_rx_dim_work); > virtnet_napi_enable(rq->vq, &rq->napi); > + } > return err; > } > > @@ -3341,24 +3390,55 @@ static int virtnet_send_tx_notf_coal_cmds(struct virtnet_info *vi, > static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi, > struct ethtool_coalesce *ec) > { > + bool rx_ctrl_dim_on = !!ec->use_adaptive_rx_coalesce; > + bool update = false, switch_dim; > struct scatterlist sgs_rx; > int i; > > - vi->ctrl->coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs); > - vi->ctrl->coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames); > - sg_init_one(&sgs_rx, &vi->ctrl->coal_rx, sizeof(vi->ctrl->coal_rx)); > - > - if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL, > - VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, > - &sgs_rx)) > - return -EINVAL; > + switch_dim = rx_ctrl_dim_on != vi->rx_dim_enabled; > + if (switch_dim) { > + if (rx_ctrl_dim_on) { > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { So basically, I'm asking why we need to duplicate the check here? E.g caller has done the check for us: if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) ret = virtnet_send_notf_coal_cmds(vi, ec); else ret = virtnet_coal_params_supported(ec); ? Please also check other nested if/else, usually, too many levels of nesting is a hint that the logic needs to be optimized. Thanks