From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out30-98.freemail.mail.aliyun.com (out30-98.freemail.mail.aliyun.com [115.124.30.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4592D18628D for ; Mon, 17 Jun 2024 08:08:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=115.124.30.98 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718611734; cv=none; b=WPSZUmemddqfbE+hbLZUz0APPnmgm0DDRNW4CDCW8KUQPs4ZYo+lv9GuXnUIM3rjIMm0/+VRqxvByodWDw5Ecm0RIAADEXP1SJCx2lCYw+svptQup5MuIuWbdevRGN3ByzjogH+sCwCdHUyuOoR4isPqxvPcLbNjuxyajqEBXfk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718611734; c=relaxed/simple; bh=OjNyNLrCJcKH0nNeI5uFHGiJJgaa2WZleuHU6NxEwg8=; h=Message-ID:Subject:Date:From:To:Cc:References:In-Reply-To: Content-Type; b=O9MwyIUxa+NjsziILV2ROSQyUQhJ4WCVHF8cw7OJurRRaXiw/t+I2lBaPW9gP07v5AmJBLv4DmQTHhWl9pj182h84YUuoeJBnQmOJpdEbUNvCcTXCzJgFh6kvHY56os27wG/9DvVdWKjoeBtatbVtPET4nMHw3ReY4nS+X26QJw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.alibaba.com; spf=pass smtp.mailfrom=linux.alibaba.com; dkim=pass (1024-bit key) header.d=linux.alibaba.com header.i=@linux.alibaba.com header.b=kXnWeXGN; arc=none smtp.client-ip=115.124.30.98 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.alibaba.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.alibaba.com header.i=@linux.alibaba.com header.b="kXnWeXGN" DKIM-Signature:v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.alibaba.com; s=default; t=1718611728; h=Message-ID:Subject:Date:From:To:Content-Type; bh=baTGbjOXYj2449XpstO0V/eCJsz986nZ4IK+u8zw7Mg=; b=kXnWeXGNcxzxSz7RUcPQplaqSb4GKvTsf+YyfWSjlTZnI+CifVrYZXdaDsD33p3x4EaSzqS1L1J0OYUfqAqjCEr1EHBjcrZvxTJyDdWLYD7SjAP6sIVcHHJDJretqhl5BzMmN9Mg3feKs7nqr3JpD130aVPFFiX4fca/7rKikSY= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R141e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=maildocker-contentspam033045075189;MF=hengqi@linux.alibaba.com;NM=1;PH=DS;RN=10;SR=0;TI=SMTPD_---0W8b91p-_1718611727; Received: from localhost(mailfrom:hengqi@linux.alibaba.com fp:SMTPD_---0W8b91p-_1718611727) by smtp.aliyun-inc.com; Mon, 17 Jun 2024 16:08:48 +0800 Message-ID: <1718609268.7814527-9-hengqi@linux.alibaba.com> Subject: Re: [PATCH net-next v3 4/4] virtio_net: improve dim command request efficiency Date: Mon, 17 Jun 2024 15:27:48 +0800 From: Heng Qi To: Jason Wang Cc: netdev@vger.kernel.org, virtualization@lists.linux.dev, "Michael S. Tsirkin" , Xuan Zhuo , =?utf-8?q?Eugenio_P=C3=A9rez?= , Eric Dumazet , "David S. Miller" , Jakub Kicinski , Paolo Abeni References: <20240606061446.127802-1-hengqi@linux.alibaba.com> <20240606061446.127802-5-hengqi@linux.alibaba.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk X-Mailing-List: virtualization@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: On Mon, 17 Jun 2024 12:05:30 +0800, Jason Wang wrote: > On Thu, Jun 6, 2024 at 2:15=E2=80=AFPM Heng Qi = wrote: > > > > Currently, control vq 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 > > --- > > drivers/net/virtio_net.c | 233 ++++++++++++++++++++++++++++++++++----- > > 1 file changed, 208 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index e59e12bb7601..0338528993ab 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -376,6 +376,13 @@ struct control_buf { > > struct completion completion; > > }; > > > > +struct virtnet_coal_node { > > + struct control_buf ctrl; > > + struct virtio_net_ctrl_coal_vq coal_vqs; > > + bool is_coal_wait; > > + struct list_head list; > > +}; > > + > > struct virtnet_info { > > struct virtio_device *vdev; > > struct virtqueue *cvq; > > @@ -420,6 +427,9 @@ struct virtnet_info { > > /* Lock to protect the control VQ */ > > struct mutex cvq_lock; > > > > + /* 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; > > > > @@ -464,6 +474,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; > > > > @@ -670,7 +688,7 @@ static void virtnet_cvq_done(struct virtqueue *cvq) > > { > > struct virtnet_info *vi =3D cvq->vdev->priv; > > > > - complete(&vi->ctrl->completion); > > + schedule_work(&vi->get_cvq); > > } > > > > static void skb_xmit_done(struct virtqueue *vq) > > @@ -2696,7 +2714,7 @@ static bool virtnet_send_command_reply(struct vir= tnet_info *vi, > > struct scatterlist *in) > > { > > struct scatterlist *sgs[5], hdr, stat; > > - u32 out_num =3D 0, tmp, in_num =3D 0; > > + u32 out_num =3D 0, in_num =3D 0; > > int ret; > > > > /* Caller should know better */ > > @@ -2730,14 +2748,14 @@ static bool virtnet_send_command_reply(struct v= irtnet_info *vi, > > 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->ctrl->completion); > > - virtqueue_get_buf(vi->cvq, &tmp); > > + wait_for_completion(&ctrl->completion); > > > > -unlock: > > - mutex_unlock(&vi->cvq_lock); > > return ctrl->status =3D=3D VIRTIO_NET_OK; > > } > > > > @@ -2747,6 +2765,86 @@ static bool virtnet_send_command(struct virtnet_= info *vi, u8 class, u8 cmd, > > return virtnet_send_command_reply(vi, class, cmd, vi->ctrl, out= , NULL); > > } > > > > +static void virtnet_process_dim_cmd(struct virtnet_info *vi, > > + struct virtnet_coal_node *node) > > +{ > > + u16 qnum =3D le16_to_cpu(node->coal_vqs.vqn) / 2; > > + > > + mutex_lock(&vi->rq[qnum].dim_lock); > > + vi->rq[qnum].intr_coal.max_usecs =3D > > + le32_to_cpu(node->coal_vqs.coal.max_usecs); > > + vi->rq[qnum].intr_coal.max_packets =3D > > + le32_to_cpu(node->coal_vqs.coal.max_packets); > > + vi->rq[qnum].dim.state =3D DIM_START_MEASURE; > > + mutex_unlock(&vi->rq[qnum].dim_lock); > > + > > + if (node->is_coal_wait) { > > + mutex_lock(&vi->coal_wait_lock); > > + list_del(&node->list); > > + mutex_unlock(&vi->coal_wait_lock); > > + kfree(node); > > + } else { > > + mutex_lock(&vi->coal_free_lock); > > + list_add(&node->list, &vi->coal_free_list); > > + mutex_unlock(&vi->coal_free_lock); > > + } > > +} > > + > > +static int virtnet_add_dim_command(struct virtnet_info *vi, > > + struct virtnet_coal_node *coal_node) > > +{ > > + struct scatterlist sg; > > + int ret; > > + > > + sg_init_one(&sg, &coal_node->coal_vqs, sizeof(coal_node->coal_v= qs)); > > + ret =3D virtnet_send_command_reply(vi, VIRTIO_NET_CTRL_NOTF_COA= L, > > + VIRTIO_NET_CTRL_NOTF_COAL_VQ_S= ET, > > + &coal_node->ctrl, &sg, NULL); > > + if (!ret) { > > + dev_warn(&vi->dev->dev, > > + "Failed to change coalescing params.\n"); > > + return ret; > > + } > > + > > + virtnet_process_dim_cmd(vi, coal_node); > > + > > + return 0; > > +} > > + > > +static void virtnet_get_cvq_work(struct work_struct *work) > > +{ > > + struct virtnet_info *vi =3D > > + container_of(work, struct virtnet_info, get_cvq); > > + struct virtnet_coal_node *wait_coal; > > + bool valid =3D false; > > + unsigned int tmp; > > + void *res; > > + > > + mutex_lock(&vi->cvq_lock); > > + while ((res =3D virtqueue_get_buf(vi->cvq, &tmp)) !=3D NULL) { > > + complete((struct completion *)res); > > + valid =3D true; > > + } > > + mutex_unlock(&vi->cvq_lock); >=20 > How could we synchronize with the device in this case? >=20 > E.g what happens if the device finishes another buf here? That's a good question. I think we can solve it using the following snippet? diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index e59e12bb7601..5dc3e1244016 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -420,6 +427,12 @@ struct virtnet_info { /* Lock to protect the control VQ */ struct mutex cvq_lock; + /* Atomic to confirm whether the cvq work is scheduled. */ + atomic_t scheduled; + + /* Work struct for acquisition of cvq processing results. */ + struct work_struct get_cvq; + @@ -670,7 +691,9 @@ static void virtnet_cvq_done(struct virtqueue *cvq) { struct virtnet_info *vi =3D cvq->vdev->priv; - complete(&vi->ctrl->completion); + virtqueue_disable_cb(cvq); + if (!atomic_xchg(&vi->scheduled, 1)) + schedule_work(&vi->get_cvq); } +static void virtnet_get_cvq_work(struct work_struct *work) +{ + struct virtnet_info *vi =3D + container_of(work, struct virtnet_info, get_cvq); + struct virtnet_coal_node *wait_coal; + bool valid =3D false; + unsigned int tmp; + void *res; + + mutex_lock(&vi->cvq_lock); + while ((res =3D virtqueue_get_buf(vi->cvq, &tmp)) !=3D NULL) { + complete((struct completion *)res); + valid =3D true; + } + mutex_unlock(&vi->cvq_lock); + + atomic_set(&vi->scheduled, 0); + virtqueue_enable_cb_prepare(vi->cvq); +} >=20 > > + > > + if (!valid) > > + return; > > + > > + while (true) { > > + wait_coal =3D NULL; > > + mutex_lock(&vi->coal_wait_lock); > > + if (!list_empty(&vi->coal_wait_list)) > > + wait_coal =3D list_first_entry(&vi->coal_wait_l= ist, > > + struct virtnet_coa= l_node, > > + list); > > + mutex_unlock(&vi->coal_wait_lock); > > + if (wait_coal) > > + if (virtnet_add_dim_command(vi, wait_coal)) > > + break; > > + else > > + break; > > + } >=20 > This is still an ad-hoc optimization for dim in the general path here. >=20 > Could we have a fn callback so for non dim it's just a completion and > for dim it would be a schedule_work()? >=20 OK, I will try this. And how about this : +static void virtnet_cvq_work_sched(struct virtqueue *cvq) +{ + struct virtnet_info *vi =3D cvq->vdev->priv; + + virtqueue_disable_cb(cvq); + if (!atomic_xchg(&vi->scheduled, 1)) + schedule_work(&vi->get_cvq); +} + static void virtnet_cvq_done(struct virtqueue *cvq) { struct virtnet_info *vi =3D cvq->vdev->priv; + unsigned int tmp; + virtqueue_get_buf(vi->cvq, &tmp); complete(&vi->ctrl->completion); } @@ -5318,7 +5472,11 @@ static int virtnet_find_vqs(struct virtnet_info *vi) /* Parameters for control virtqueue, if any */ if (vi->has_cvq) { - callbacks[total_vqs - 1] =3D virtnet_cvq_done; + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) + callbacks[total_vqs - 1] =3D virtnet_cvq_work_sched; + else + callbacks[total_vqs - 1] =3D virtnet_cvq_done; + names[total_vqs - 1] =3D "control"; } > > +} > > static int virtnet_set_mac_address(struct net_device *dev, void *p) > > { > > struct virtnet_info *vi =3D netdev_priv(dev); > > @@ -4398,35 +4496,73 @@ static int virtnet_send_notf_coal_vq_cmds(struc= t 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 =3D kzalloc(sizeof(*wait_node), GFP_KERNEL); > > + if (!wait_node) { > > + rq->dim.state =3D DIM_START_MEASURE; > > + return; > > + } > > + > > + wait_node->is_coal_wait =3D true; > > + wait_node->coal_vqs.vqn =3D cpu_to_le16(rxq2vq(rq - vi->rq)); > > + wait_node->coal_vqs.coal.max_usecs =3D cpu_to_le32(moder.usec); > > + wait_node->coal_vqs.coal.max_packets =3D 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 =3D container_of(work, struct dim, work); > > struct receive_queue *rq =3D container_of(dim, > > struct receive_queue, dim); > > struct virtnet_info *vi =3D rq->vq->vdev->priv; > > - struct net_device *dev =3D vi->dev; > > + struct virtnet_coal_node *avail_coal; > > struct dim_cq_moder update_moder; > > - int qnum, err; > > > > - qnum =3D rq - vi->rq; > > + update_moder =3D net_dim_get_rx_moderation(dim->mode, dim->prof= ile_ix); > > > > mutex_lock(&rq->dim_lock); > > - if (!rq->dim_enabled) > > - goto out; > > - > > - update_moder =3D net_dim_get_rx_moderation(dim->mode, dim->prof= ile_ix); > > - if (update_moder.usec !=3D rq->intr_coal.max_usecs || > > - update_moder.pkts !=3D rq->intr_coal.max_packets) { > > - err =3D virtnet_send_rx_ctrl_coal_vq_cmd(vi, qnum, > > - update_moder.use= c, > > - update_moder.pkt= s); > > - if (err) > > - pr_debug("%s: Failed to send dim parameters on = rxq%d\n", > > - dev->name, qnum); > > - dim->state =3D DIM_START_MEASURE; > > + if (!rq->dim_enabled || > > + (update_moder.usec =3D=3D rq->intr_coal.max_usecs && > > + update_moder.pkts =3D=3D rq->intr_coal.max_packets)) { > > + rq->dim.state =3D 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; > > + } >=20 > Could we simply sleep instead of using a list here? Do you mean using a semaphore, or a waitqueue? >=20 > > + 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 =3D list_first_entry(&vi->coal_free_list, > > + struct virtnet_coal_node, list); > > + avail_coal->coal_vqs.vqn =3D cpu_to_le16(rxq2vq(rq - vi->rq)); > > + avail_coal->coal_vqs.coal.max_usecs =3D cpu_to_le32(update_mode= r.usec); > > + avail_coal->coal_vqs.coal.max_packets =3D cpu_to_le32(update_mo= der.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) > > @@ -4839,6 +4975,7 @@ static void virtnet_freeze_down(struct virtio_dev= ice *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); > > @@ -5612,6 +5749,45 @@ static const struct xdp_metadata_ops virtnet_xdp= _metadata_ops =3D { > > .xmo_rx_hash =3D 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 =3D min((unsigned int)vi->max_queue_pairs, > > + virtqueue_get_vring_size(vi->cvq) / 3); > > + for (i =3D 0; i < batch_dim_nums; i++) { > > + coal_node =3D 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 =3D -ENOMEM; > > @@ -5797,6 +5973,9 @@ static int virtnet_probe(struct virtio_device *vd= ev) > > 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 =3D 0; > > vi->intr_coal_tx.max_usecs =3D 0; > > @@ -5838,6 +6017,7 @@ static int virtnet_probe(struct virtio_device *vd= ev) > > 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->ctrl->completion); > > enable_rx_mode_work(vi); > > > > @@ -5967,11 +6147,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); >=20 > Do we need to prevent cvq work from being scheduled here? You are right, I'll fix in the next version. Thanks! >=20 > Thanks >=20 > > > > 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 > > >=20