Re: [PATCH net-next v2 2/2] virtio_net: improve dim command request efficiency

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 6 Jun 2024 08:27:35 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Wed, Jun 5, 2024 at 10:56 PM Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> wrote:
> >
> > Currently, the control queue (ctrlq) handles commands synchronously,
> > leading to increased delays for dim commands during multi-queue
> > VM configuration and directly impacting dim performance.
> >
> > To address this, we are shifting to asynchronous processing of
> > ctrlq's dim commands.
> >
> > Signed-off-by: Heng Qi <hengqi@xxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/net/virtio_net.c | 260 +++++++++++++++++++++++++++++++++++----
> >  1 file changed, 236 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 9b556ce89546..7975084052ad 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -270,6 +270,14 @@ struct virtnet_interrupt_coalesce {
> >         u32 max_usecs;
> >  };
> >
> > +struct virtnet_coal_node {
> > +       struct virtio_net_ctrl_hdr hdr;
> > +       virtio_net_ctrl_ack status;
> > +       struct virtio_net_ctrl_coal_vq coal_vqs;
> > +       bool is_coal_wait;
> > +       struct list_head list;
> > +};
> > +
> >  /* The dma information of pages allocated at a time. */
> >  struct virtnet_rq_dma {
> >         dma_addr_t addr;
> > @@ -421,6 +429,9 @@ struct virtnet_info {
> >         /* Wait for the device to complete the cvq request. */
> >         struct completion completion;
> >
> > +       /* Work struct for acquisition of cvq processing results. */
> > +       struct work_struct get_cvq;
> > +
> >         /* Host can handle any s/g split between our header and packet data */
> >         bool any_header_sg;
> >
> > @@ -465,6 +476,14 @@ struct virtnet_info {
> >         struct virtnet_interrupt_coalesce intr_coal_tx;
> >         struct virtnet_interrupt_coalesce intr_coal_rx;
> >
> > +       /* Free nodes used for concurrent delivery */
> > +       struct mutex coal_free_lock;
> > +       struct list_head coal_free_list;
> > +
> > +       /* Filled when there are no free nodes or cvq buffers */
> > +       struct mutex coal_wait_lock;
> > +       struct list_head coal_wait_list;
> > +
> >         unsigned long guest_offloads;
> >         unsigned long guest_offloads_capable;
> >
> > @@ -671,7 +690,7 @@ static void virtnet_cvq_done(struct virtqueue *cvq)
> >  {
> >         struct virtnet_info *vi = cvq->vdev->priv;
> >
> > -       complete(&vi->completion);
> > +       schedule_work(&vi->get_cvq);
> >  }
> >
> >  static void skb_xmit_done(struct virtqueue *vq)
> > @@ -2183,6 +2202,113 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> >         return !oom;
> >  }
> >
> > +static int __virtnet_add_dim_command(struct virtnet_info *vi,
> > +                                    struct virtnet_coal_node *ctrl)
> > +{
> > +       struct scatterlist *sgs[4], hdr, stat, out;
> > +       unsigned int out_num = 0;
> > +       int ret;
> > +
> > +       BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
> > +
> > +       ctrl->hdr.class = VIRTIO_NET_CTRL_NOTF_COAL;
> > +       ctrl->hdr.cmd = VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET;
> > +
> > +       sg_init_one(&hdr, &ctrl->hdr, sizeof(ctrl->hdr));
> > +       sgs[out_num++] = &hdr;
> > +
> > +       sg_init_one(&out, &ctrl->coal_vqs, sizeof(ctrl->coal_vqs));
> > +       sgs[out_num++] = &out;
> > +
> > +       ctrl->status = ~0;
> > +       sg_init_one(&stat, &ctrl->status, sizeof(ctrl->status));
> > +       sgs[out_num] = &stat;
> > +
> > +       BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
> > +       ret = virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, ctrl, GFP_ATOMIC);
> > +       if (ret < 0) {
> > +               dev_warn(&vi->vdev->dev,
> > +                        "Failed to add sgs for command vq: %d\n.", ret);
> > +               return ret;
> > +       }
> > +
> > +       if (unlikely(!virtqueue_kick(vi->cvq)))
> > +               return -EIO;
> > +
> > +       return 0;
> > +}
> > +
> > +static int virtnet_add_dim_command(struct virtnet_info *vi,
> > +                                  struct virtnet_coal_node *ctrl)
> > +{
> > +       int ret;
> > +
> > +       mutex_lock(&vi->cvq_lock);
> > +       ret = __virtnet_add_dim_command(vi, ctrl);
> > +       mutex_unlock(&vi->cvq_lock);
> > +
> > +       return ret;
> > +}
> > +
> > +static void virtnet_process_dim_cmd(struct virtnet_info *vi, void *res)
> > +{
> > +       struct virtnet_coal_node *node = (struct virtnet_coal_node *)res;
> > +       u16 qnum = le16_to_cpu(node->coal_vqs.vqn) / 2;
> > +
> > +       mutex_lock(&vi->rq[qnum].dim_lock);
> > +       vi->rq[qnum].intr_coal.max_usecs =
> > +               le32_to_cpu(node->coal_vqs.coal.max_usecs);
> > +       vi->rq[qnum].intr_coal.max_packets =
> > +               le32_to_cpu(node->coal_vqs.coal.max_packets);
> > +       vi->rq[qnum].dim.state = DIM_START_MEASURE;
> > +       mutex_unlock(&vi->rq[qnum].dim_lock);
> > +
> > +       if (node->is_coal_wait) {
> > +               kfree(node);
> > +       } else {
> > +               mutex_lock(&vi->coal_free_lock);
> > +               list_add(&node->list, &vi->coal_free_list);
> > +               mutex_unlock(&vi->coal_free_lock);
> > +       }
> > +}
> > +
> > +static void virtnet_get_cvq_work(struct work_struct *work)
> > +{
> > +       struct virtnet_info *vi =
> > +               container_of(work, struct virtnet_info, get_cvq);
> > +       struct virtnet_coal_node *wait_coal;
> > +       bool valid = false;
> > +       unsigned int tmp;
> > +       void *res;
> > +
> > +       mutex_lock(&vi->cvq_lock);
> > +       while ((res = virtqueue_get_buf(vi->cvq, &tmp)) != NULL) {
> > +               if (res == ((void *)vi))
> > +                       complete(&vi->completion);
> > +               else
> > +                       virtnet_process_dim_cmd(vi, res);
> 
> This seems an ad-hoc optimization for dim? Instead of doing this, I
> wonder if we can make it re-usable for other possible use cases.
> 

The purpose of this is that I don't see the need for concurrent ctrlq requests
in addition to dim. But I can try your suggestion.

Thanks.

> For example, just allocation completion and store the completion as a
> token. Then virtqueue_get_buf can wake up the correct completion?
> 
> Thanks
> 
> > +
> > +               valid = true;
> > +       }
> > +
> > +       if (!valid) {
> > +               mutex_unlock(&vi->cvq_lock);
> > +               return;
> > +       }
> > +
> > +       mutex_lock(&vi->coal_wait_lock);
> > +       while (!list_empty(&vi->coal_wait_list)) {
> > +               wait_coal = list_first_entry(&vi->coal_wait_list,
> > +                                            struct virtnet_coal_node, list);
> > +               if (__virtnet_add_dim_command(vi, wait_coal))
> > +                       break;
> > +               list_del(&wait_coal->list);
> > +       }
> > +       mutex_unlock(&vi->coal_wait_lock);
> > +
> > +       mutex_unlock(&vi->cvq_lock);
> > +}
> > +
> >  static void skb_recv_done(struct virtqueue *rvq)
> >  {
> >         struct virtnet_info *vi = rvq->vdev->priv;
> > @@ -2695,7 +2821,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
> >                                        struct scatterlist *in)
> >  {
> >         struct scatterlist *sgs[5], hdr, stat;
> > -       u32 out_num = 0, tmp, in_num = 0;
> > +       u32 out_num = 0, in_num = 0;
> >         int ret;
> >
> >         /* Caller should know better */
> > @@ -2728,14 +2854,15 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
> >                 return false;
> >         }
> >
> > -       if (unlikely(!virtqueue_kick(vi->cvq)))
> > -               goto unlock;
> > +       if (unlikely(!virtqueue_kick(vi->cvq))) {
> > +               mutex_unlock(&vi->cvq_lock);
> > +               return false;
> > +       }
> > +
> > +       mutex_unlock(&vi->cvq_lock);
> >
> >         wait_for_completion(&vi->completion);
> > -       virtqueue_get_buf(vi->cvq, &tmp);
> >
> > -unlock:
> > -       mutex_unlock(&vi->cvq_lock);
> >         return vi->ctrl->status == VIRTIO_NET_OK;
> >  }
> >
> > @@ -4396,35 +4523,73 @@ static int virtnet_send_notf_coal_vq_cmds(struct virtnet_info *vi,
> >         return 0;
> >  }
> >
> > +static void virtnet_put_wait_coal(struct virtnet_info *vi,
> > +                                 struct receive_queue *rq,
> > +                                 struct dim_cq_moder moder)
> > +{
> > +       struct virtnet_coal_node *wait_node;
> > +
> > +       wait_node = kzalloc(sizeof(*wait_node), GFP_KERNEL);
> > +       if (!wait_node) {
> > +               rq->dim.state = DIM_START_MEASURE;
> > +               return;
> > +       }
> > +
> > +       wait_node->is_coal_wait = true;
> > +       wait_node->coal_vqs.vqn = cpu_to_le16(rxq2vq(rq - vi->rq));
> > +       wait_node->coal_vqs.coal.max_usecs = cpu_to_le32(moder.usec);
> > +       wait_node->coal_vqs.coal.max_packets = cpu_to_le32(moder.pkts);
> > +       mutex_lock(&vi->coal_wait_lock);
> > +       list_add_tail(&wait_node->list, &vi->coal_wait_list);
> > +       mutex_unlock(&vi->coal_wait_lock);
> > +}
> > +
> >  static void virtnet_rx_dim_work(struct work_struct *work)
> >  {
> >         struct dim *dim = container_of(work, struct dim, work);
> >         struct receive_queue *rq = container_of(dim,
> >                         struct receive_queue, dim);
> >         struct virtnet_info *vi = rq->vq->vdev->priv;
> > -       struct net_device *dev = vi->dev;
> > +       struct virtnet_coal_node *avail_coal;
> >         struct dim_cq_moder update_moder;
> > -       int qnum, err;
> >
> > -       qnum = rq - vi->rq;
> > +       update_moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
> >
> >         mutex_lock(&rq->dim_lock);
> > -       if (!rq->dim_enabled)
> > -               goto out;
> > -
> > -       update_moder = net_dim_get_rx_moderation(dim->mode, 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,
> > -                                                      update_moder.usec,
> > -                                                      update_moder.pkts);
> > -               if (err)
> > -                       pr_debug("%s: Failed to send dim parameters on rxq%d\n",
> > -                                dev->name, qnum);
> > -               dim->state = DIM_START_MEASURE;
> > +       if (!rq->dim_enabled ||
> > +           (update_moder.usec == rq->intr_coal.max_usecs &&
> > +            update_moder.pkts == rq->intr_coal.max_packets)) {
> > +               rq->dim.state = DIM_START_MEASURE;
> > +               mutex_unlock(&rq->dim_lock);
> > +               return;
> >         }
> > -out:
> >         mutex_unlock(&rq->dim_lock);
> > +
> > +       mutex_lock(&vi->cvq_lock);
> > +       if (vi->cvq->num_free < 3) {
> > +               virtnet_put_wait_coal(vi, rq, update_moder);
> > +               mutex_unlock(&vi->cvq_lock);
> > +               return;
> > +       }
> > +       mutex_unlock(&vi->cvq_lock);
> > +
> > +       mutex_lock(&vi->coal_free_lock);
> > +       if (list_empty(&vi->coal_free_list)) {
> > +               virtnet_put_wait_coal(vi, rq, update_moder);
> > +               mutex_unlock(&vi->coal_free_lock);
> > +               return;
> > +       }
> > +
> > +       avail_coal = list_first_entry(&vi->coal_free_list,
> > +                                     struct virtnet_coal_node, list);
> > +       avail_coal->coal_vqs.vqn = cpu_to_le16(rxq2vq(rq - vi->rq));
> > +       avail_coal->coal_vqs.coal.max_usecs = cpu_to_le32(update_moder.usec);
> > +       avail_coal->coal_vqs.coal.max_packets = cpu_to_le32(update_moder.pkts);
> > +
> > +       list_del(&avail_coal->list);
> > +       mutex_unlock(&vi->coal_free_lock);
> > +
> > +       virtnet_add_dim_command(vi, avail_coal);
> >  }
> >
> >  static int virtnet_coal_params_supported(struct ethtool_coalesce *ec)
> > @@ -4837,6 +5002,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
> >         flush_work(&vi->config_work);
> >         disable_rx_mode_work(vi);
> >         flush_work(&vi->rx_mode_work);
> > +       flush_work(&vi->get_cvq);
> >
> >         netif_tx_lock_bh(vi->dev);
> >         netif_device_detach(vi->dev);
> > @@ -5610,6 +5776,45 @@ static const struct xdp_metadata_ops virtnet_xdp_metadata_ops = {
> >         .xmo_rx_hash                    = virtnet_xdp_rx_hash,
> >  };
> >
> > +static void virtnet_del_coal_free_list(struct virtnet_info *vi)
> > +{
> > +       struct virtnet_coal_node *coal_node, *tmp;
> > +
> > +       list_for_each_entry_safe(coal_node, tmp,  &vi->coal_free_list, list) {
> > +               list_del(&coal_node->list);
> > +               kfree(coal_node);
> > +       }
> > +}
> > +
> > +static int virtnet_init_coal_list(struct virtnet_info *vi)
> > +{
> > +       struct virtnet_coal_node *coal_node;
> > +       int batch_dim_nums;
> > +       int i;
> > +
> > +       INIT_LIST_HEAD(&vi->coal_free_list);
> > +       mutex_init(&vi->coal_free_lock);
> > +
> > +       INIT_LIST_HEAD(&vi->coal_wait_list);
> > +       mutex_init(&vi->coal_wait_lock);
> > +
> > +       if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL))
> > +               return 0;
> > +
> > +       batch_dim_nums = min((unsigned int)vi->max_queue_pairs,
> > +                            virtqueue_get_vring_size(vi->cvq) / 3);
> > +       for (i = 0; i < batch_dim_nums; i++) {
> > +               coal_node = kzalloc(sizeof(*coal_node), GFP_KERNEL);
> > +               if (!coal_node) {
> > +                       virtnet_del_coal_free_list(vi);
> > +                       return -ENOMEM;
> > +               }
> > +               list_add(&coal_node->list, &vi->coal_free_list);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int virtnet_probe(struct virtio_device *vdev)
> >  {
> >         int i, err = -ENOMEM;
> > @@ -5795,6 +6000,9 @@ static int virtnet_probe(struct virtio_device *vdev)
> >         if (err)
> >                 goto free;
> >
> > +       if (virtnet_init_coal_list(vi))
> > +               goto free;
> > +
> >         if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) {
> >                 vi->intr_coal_rx.max_usecs = 0;
> >                 vi->intr_coal_tx.max_usecs = 0;
> > @@ -5836,6 +6044,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> >         if (vi->has_rss || vi->has_rss_hash_report)
> >                 virtnet_init_default_rss(vi);
> >
> > +       INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work);
> >         init_completion(&vi->completion);
> >         enable_rx_mode_work(vi);
> >
> > @@ -5965,11 +6174,14 @@ static void virtnet_remove(struct virtio_device *vdev)
> >         flush_work(&vi->config_work);
> >         disable_rx_mode_work(vi);
> >         flush_work(&vi->rx_mode_work);
> > +       flush_work(&vi->get_cvq);
> >
> >         unregister_netdev(vi->dev);
> >
> >         net_failover_destroy(vi->failover);
> >
> > +       virtnet_del_coal_free_list(vi);
> > +
> >         remove_vq_common(vi);
> >
> >         free_netdev(vi->dev);
> > --
> > 2.32.0.3.g01195cf9f
> >
> 





[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux