[PATCH net-next 0/3] virtio_net: enable the irq for ctrlq

[PATCH net-next 0/3] virtio_net: enable the irq for ctrlq

[Heng Qi]

Ctrlq in polling mode may cause the virtual machine to hang and
occupy additional CPU resources. Enabling the irq for ctrlq
alleviates this problem and allows commands to be requested
concurrently.

This patch set is on top of
https://lore.kernel.org/all/20240423035746.699466-1-danielj@nvidia.com/ ,
so patchwork may cause warnings.

Heng Qi (3):
  virtio_net: enable irq for the control vq
  virtio_net: fix possible dim status unrecoverable
  virtio_net: improve dim command request efficiency

 drivers/net/virtio_net.c | 268 +++++++++++++++++++++++++++++++++++----
 1 file changed, 240 insertions(+), 28 deletions(-)

-- 
2.32.0.3.g01195cf9f

[PATCH net-next 1/3] virtio_net: enable irq for the control vq

[Heng Qi]

Control vq polling request results consume more CPU.
Especially when dim issues more control requests to the device,
it's beneficial to the guest to enable control vq's irq.

Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a4d3c76654a4..79a1b30c173c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -287,6 +287,12 @@ struct virtnet_info {
 	bool has_cvq;
 	struct mutex cvq_lock;
 
+	/* Wait for the device to complete the 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;
 
@@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
 	return false;
 }
 
+static void virtnet_cvq_done(struct virtqueue *cvq)
+{
+	struct virtnet_info *vi = cvq->vdev->priv;
+
+	schedule_work(&vi->get_cvq);
+}
+
 static void skb_xmit_done(struct virtqueue *vq)
 {
 	struct virtnet_info *vi = vq->vdev->priv;
@@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 	return !oom;
 }
 
+static void virtnet_get_cvq_work(struct work_struct *work)
+{
+	struct virtnet_info *vi =
+		container_of(work, struct virtnet_info, get_cvq);
+	unsigned int tmp;
+	void *res;
+
+	mutex_lock(&vi->cvq_lock);
+	res = virtqueue_get_buf(vi->cvq, &tmp);
+	if (res)
+		complete(&vi->completion);
+	mutex_unlock(&vi->cvq_lock);
+}
+
 static void skb_recv_done(struct virtqueue *rvq)
 {
 	struct virtnet_info *vi = rvq->vdev->priv;
@@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *out)
 {
 	struct scatterlist *sgs[4], hdr, stat;
-	unsigned out_num = 0, tmp;
+	unsigned out_num = 0;
 	int ret;
 
 	/* Caller should know better */
@@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 		return vi->ctrl->status == VIRTIO_NET_OK;
 	}
 
-	/* Spin for a response, the kick causes an ioport write, trapping
-	 * into the hypervisor, so the request should be handled immediately.
-	 */
-	while (!virtqueue_get_buf(vi->cvq, &tmp) &&
-	       !virtqueue_is_broken(vi->cvq)) {
-		cond_resched();
-		cpu_relax();
-	}
-
 	mutex_unlock(&vi->cvq_lock);
+
+	wait_for_completion(&vi->completion);
+
 	return vi->ctrl->status == VIRTIO_NET_OK;
 }
 
@@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 
 	/* Parameters for control virtqueue, if any */
 	if (vi->has_cvq) {
-		callbacks[total_vqs - 1] = NULL;
+		callbacks[total_vqs - 1] = virtnet_cvq_done;
 		names[total_vqs - 1] = "control";
 	}
 
@@ -4952,6 +4973,8 @@ 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);
 
 	/* serialize netdev register + virtio_device_ready() with ndo_open() */
-- 
2.32.0.3.g01195cf9f

[PATCH net-next 2/3] virtio_net: fix possible dim status unrecoverable

[Heng Qi]

When the dim worker is scheduled, if it no longer needs to issue
commands, dim may not be able to return to the working state later.

For example, the following single queue scenario:
  1. The dim worker of rxq0 is scheduled, and the dim status is
     changed to DIM_APPLY_NEW_PROFILE;
  2. dim is disabled or parameters have not been modified;
  3. virtnet_rx_dim_work exits directly;

Then, even if net_dim is invoked again, it cannot work because the
state is not restored to DIM_START_MEASURE.

Fixes: 6208799553a8 ("virtio-net: support rx netdim")
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 79a1b30c173c..8f05bcf1d37d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3650,9 +3650,9 @@ static void virtnet_rx_dim_work(struct work_struct *work)
 		if (err)
 			pr_debug("%s: Failed to send dim parameters on rxq%d\n",
 				 dev->name, qnum);
-		dim->state = DIM_START_MEASURE;
 	}
 out:
+	dim->state = DIM_START_MEASURE;
 	mutex_unlock(&rq->dim_lock);
 }
 
-- 
2.32.0.3.g01195cf9f

[PATCH net-next 3/3] virtio_net: improve dim command request efficiency

[Heng Qi]

Currently, ctrlq processes commands in a synchronous manner,
which increases the delay of dim commands when configuring
multi-queue VMs, which in turn causes the CPU utilization to
increase and interferes with the performance of dim.

Therefore we asynchronously process ctlq's dim commands.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 229 +++++++++++++++++++++++++++++++++++----
 1 file changed, 209 insertions(+), 20 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 8f05bcf1d37d..6cd72c0119e6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -138,6 +138,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_wait;
+	struct list_head list;
+};
+
 /* The dma information of pages allocated at a time. */
 struct virtnet_rq_dma {
 	dma_addr_t addr;
@@ -337,6 +345,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;
 
@@ -2049,17 +2065,108 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 	return !oom;
 }
 
+static void __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 = VIRTIO_NET_OK;
+	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;
+	}
+
+	virtqueue_kick(vi->cvq);
+}
+
+static void virtnet_add_dim_command(struct virtnet_info *vi,
+				    struct virtnet_coal_node *ctrl)
+{
+	mutex_lock(&vi->cvq_lock);
+	__virtnet_add_dim_command(vi, ctrl);
+	mutex_unlock(&vi->cvq_lock);
+}
+
+static void virtnet_process_dim_cmd(struct virtnet_info *vi, void *res)
+{
+	struct virtnet_coal_node *node;
+	u16 qnum;
+
+	node = (struct virtnet_coal_node *)res;
+	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_wait) {
+		mutex_lock(&vi->coal_free_lock);
+		list_add(&node->list, &vi->coal_free_list);
+		mutex_unlock(&vi->coal_free_lock);
+	} else {
+		kfree(node);
+	}
+}
+
 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);
-	res = virtqueue_get_buf(vi->cvq, &tmp);
-	if (res)
-		complete(&vi->completion);
+	while ((res = virtqueue_get_buf(vi->cvq, &tmp)) != NULL) {
+		if (res == ((void *)vi))
+			complete(&vi->completion);
+		else
+			virtnet_process_dim_cmd(vi, res);
+
+		valid = true;
+	}
+
+	if (!valid) {
+		mutex_unlock(&vi->cvq_lock);
+		return;
+	}
+
+	mutex_lock(&vi->coal_wait_lock);
+	while (!list_empty(&vi->coal_wait_list)) {
+		if (vi->cvq->num_free < 3)
+			goto out_get_cvq;
+
+		wait_coal = list_first_entry(&vi->coal_wait_list,
+					     struct virtnet_coal_node, list);
+		list_del(&wait_coal->list);
+		__virtnet_add_dim_command(vi, wait_coal);
+	}
+
+out_get_cvq:
+	mutex_unlock(&vi->coal_wait_lock);
 	mutex_unlock(&vi->cvq_lock);
 }
 
@@ -3625,35 +3732,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_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);
+	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:
-	dim->state = DIM_START_MEASURE;
 	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)
@@ -4748,6 +4893,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;
@@ -4932,6 +5116,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;
@@ -5081,6 +5268,8 @@ static void virtnet_remove(struct virtio_device *vdev)
 
 	net_failover_destroy(vi->failover);
 
+	virtnet_del_coal_free_list(vi);
+
 	remove_vq_common(vi);
 
 	free_netdev(vi->dev);
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net-next 2/3] virtio_net: fix possible dim status unrecoverable

[Jakub Kicinski]

On Thu, 25 Apr 2024 20:58:54 +0800 Heng Qi wrote:
> When the dim worker is scheduled, if it no longer needs to issue
> commands, dim may not be able to return to the working state later.
> 
> For example, the following single queue scenario:
>   1. The dim worker of rxq0 is scheduled, and the dim status is
>      changed to DIM_APPLY_NEW_PROFILE;
>   2. dim is disabled or parameters have not been modified;
>   3. virtnet_rx_dim_work exits directly;
> 
> Then, even if net_dim is invoked again, it cannot work because the
> state is not restored to DIM_START_MEASURE.
> 
> Fixes: 6208799553a8 ("virtio-net: support rx netdim")
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>

This sounds like a legitimate bug fix so it needs to be sent separately
to the net tree (subject tagged with [PATCH net]) and then you'll have
to wait until the following Thursday for the net tree to get merged
into net-next. At which point you can send the improvements.
(Without the wait there would be a conflict between the trees).

Right now the series does not apply to net-next anyway.
-- 
pw-bot: cr

Re: [PATCH net-next 2/3] virtio_net: fix possible dim status unrecoverable

[Heng Qi]

在 2024/4/26 上午10:21, Jakub Kicinski 写道:
> On Thu, 25 Apr 2024 20:58:54 +0800 Heng Qi wrote:
>> When the dim worker is scheduled, if it no longer needs to issue
>> commands, dim may not be able to return to the working state later.
>>
>> For example, the following single queue scenario:
>>    1. The dim worker of rxq0 is scheduled, and the dim status is
>>       changed to DIM_APPLY_NEW_PROFILE;
>>    2. dim is disabled or parameters have not been modified;
>>    3. virtnet_rx_dim_work exits directly;
>>
>> Then, even if net_dim is invoked again, it cannot work because the
>> state is not restored to DIM_START_MEASURE.
>>
>> Fixes: 6208799553a8 ("virtio-net: support rx netdim")
>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> This sounds like a legitimate bug fix so it needs to be sent separately
> to the net tree (subject tagged with [PATCH net]) and then you'll have
> to wait until the following Thursday for the net tree to get merged
> into net-next. At which point you can send the improvements.
> (Without the wait there would be a conflict between the trees).


Yes, you are right, since this set is on top of the one DanJ is working on

(which I mentioned in the commit log and patchwork will warn about),

I merged the fix patch into this series. I'll wait for his set to be merged and push it again.

Thanks a lot!


>
> Right now the series does not apply to net-next anyway.

Re: [PATCH net-next 1/3] virtio_net: enable irq for the control vq

[Jason Wang]

On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
> Control vq polling request results consume more CPU.
> Especially when dim issues more control requests to the device,
> it's beneficial to the guest to enable control vq's irq.
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a4d3c76654a4..79a1b30c173c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -287,6 +287,12 @@ struct virtnet_info {
>         bool has_cvq;
>         struct mutex cvq_lock;
>
> +       /* Wait for the device to complete the 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;
>
> @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
>         return false;
>  }
>
> +static void virtnet_cvq_done(struct virtqueue *cvq)
> +{
> +       struct virtnet_info *vi = cvq->vdev->priv;
> +
> +       schedule_work(&vi->get_cvq);
> +}
> +
>  static void skb_xmit_done(struct virtqueue *vq)
>  {
>         struct virtnet_info *vi = vq->vdev->priv;
> @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>         return !oom;
>  }
>
> +static void virtnet_get_cvq_work(struct work_struct *work)
> +{
> +       struct virtnet_info *vi =
> +               container_of(work, struct virtnet_info, get_cvq);
> +       unsigned int tmp;
> +       void *res;
> +
> +       mutex_lock(&vi->cvq_lock);
> +       res = virtqueue_get_buf(vi->cvq, &tmp);
> +       if (res)
> +               complete(&vi->completion);
> +       mutex_unlock(&vi->cvq_lock);
> +}
> +
>  static void skb_recv_done(struct virtqueue *rvq)
>  {
>         struct virtnet_info *vi = rvq->vdev->priv;
> @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>                                  struct scatterlist *out)
>  {
>         struct scatterlist *sgs[4], hdr, stat;
> -       unsigned out_num = 0, tmp;
> +       unsigned out_num = 0;
>         int ret;
>
>         /* Caller should know better */
> @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>                 return vi->ctrl->status == VIRTIO_NET_OK;
>         }
>
> -       /* Spin for a response, the kick causes an ioport write, trapping
> -        * into the hypervisor, so the request should be handled immediately.
> -        */
> -       while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> -              !virtqueue_is_broken(vi->cvq)) {
> -               cond_resched();
> -               cpu_relax();
> -       }
> -
>         mutex_unlock(&vi->cvq_lock);
> +
> +       wait_for_completion(&vi->completion);
> +

A question here, can multiple cvq requests be submitted to the device?
If yes, what happens if the device completes them out of order?

Thanks

>         return vi->ctrl->status == VIRTIO_NET_OK;
>  }
>
> @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>
>         /* Parameters for control virtqueue, if any */
>         if (vi->has_cvq) {
> -               callbacks[total_vqs - 1] = NULL;
> +               callbacks[total_vqs - 1] = virtnet_cvq_done;
>                 names[total_vqs - 1] = "control";
>         }
>
> @@ -4952,6 +4973,8 @@ 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);
>
>         /* serialize netdev register + virtio_device_ready() with ndo_open() */
> --
> 2.32.0.3.g01195cf9f
>

Re: [PATCH net-next 1/3] virtio_net: enable irq for the control vq

[Heng Qi]

On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> > Control vq polling request results consume more CPU.
> > Especially when dim issues more control requests to the device,
> > it's beneficial to the guest to enable control vq's irq.
> >
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++----------
> >  1 file changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index a4d3c76654a4..79a1b30c173c 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -287,6 +287,12 @@ struct virtnet_info {
> >         bool has_cvq;
> >         struct mutex cvq_lock;
> >
> > +       /* Wait for the device to complete the 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;
> >
> > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
> >         return false;
> >  }
> >
> > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > +{
> > +       struct virtnet_info *vi = cvq->vdev->priv;
> > +
> > +       schedule_work(&vi->get_cvq);
> > +}
> > +
> >  static void skb_xmit_done(struct virtqueue *vq)
> >  {
> >         struct virtnet_info *vi = vq->vdev->priv;
> > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> >         return !oom;
> >  }
> >
> > +static void virtnet_get_cvq_work(struct work_struct *work)
> > +{
> > +       struct virtnet_info *vi =
> > +               container_of(work, struct virtnet_info, get_cvq);
> > +       unsigned int tmp;
> > +       void *res;
> > +
> > +       mutex_lock(&vi->cvq_lock);
> > +       res = virtqueue_get_buf(vi->cvq, &tmp);
> > +       if (res)
> > +               complete(&vi->completion);
> > +       mutex_unlock(&vi->cvq_lock);
> > +}
> > +
> >  static void skb_recv_done(struct virtqueue *rvq)
> >  {
> >         struct virtnet_info *vi = rvq->vdev->priv;
> > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> >                                  struct scatterlist *out)
> >  {
> >         struct scatterlist *sgs[4], hdr, stat;
> > -       unsigned out_num = 0, tmp;
> > +       unsigned out_num = 0;
> >         int ret;
> >
> >         /* Caller should know better */
> > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> >                 return vi->ctrl->status == VIRTIO_NET_OK;
> >         }
> >
> > -       /* Spin for a response, the kick causes an ioport write, trapping
> > -        * into the hypervisor, so the request should be handled immediately.
> > -        */
> > -       while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > -              !virtqueue_is_broken(vi->cvq)) {
> > -               cond_resched();
> > -               cpu_relax();
> > -       }
> > -
> >         mutex_unlock(&vi->cvq_lock);
> > +
> > +       wait_for_completion(&vi->completion);
> > +
> 
> A question here, can multiple cvq requests be submitted to the device?
> If yes, what happens if the device completes them out of order?

For user commands (such as ethtool cmds), multiple cvq requests is not allowed.
because it holds the netlink lock when waiting for the response.

For multiple dim commands and a user command allowed to be sent simultaneously
, the corresponding command-specific information(desc_state) will be used to
distinguish different responses.

Thanks.

> 
> Thanks
> 
> >         return vi->ctrl->status == VIRTIO_NET_OK;
> >  }
> >
> > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >
> >         /* Parameters for control virtqueue, if any */
> >         if (vi->has_cvq) {
> > -               callbacks[total_vqs - 1] = NULL;
> > +               callbacks[total_vqs - 1] = virtnet_cvq_done;
> >                 names[total_vqs - 1] = "control";
> >         }
> >
> > @@ -4952,6 +4973,8 @@ 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);
> >
> >         /* serialize netdev register + virtio_device_ready() with ndo_open() */
> > --
> > 2.32.0.3.g01195cf9f
> >
> 

Re: [PATCH net-next 1/3] virtio_net: enable irq for the control vq

[Jason Wang]

On Tue, May 7, 2024 at 12:03 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
> On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > >
> > > Control vq polling request results consume more CPU.
> > > Especially when dim issues more control requests to the device,
> > > it's beneficial to the guest to enable control vq's irq.
> > >
> > > Suggested-by: Jason Wang <jasowang@redhat.com>
> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > ---
> > >  drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++----------
> > >  1 file changed, 34 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index a4d3c76654a4..79a1b30c173c 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -287,6 +287,12 @@ struct virtnet_info {
> > >         bool has_cvq;
> > >         struct mutex cvq_lock;
> > >
> > > +       /* Wait for the device to complete the 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;
> > >
> > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
> > >         return false;
> > >  }
> > >
> > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > +{
> > > +       struct virtnet_info *vi = cvq->vdev->priv;
> > > +
> > > +       schedule_work(&vi->get_cvq);
> > > +}
> > > +
> > >  static void skb_xmit_done(struct virtqueue *vq)
> > >  {
> > >         struct virtnet_info *vi = vq->vdev->priv;
> > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > >         return !oom;
> > >  }
> > >
> > > +static void virtnet_get_cvq_work(struct work_struct *work)
> > > +{
> > > +       struct virtnet_info *vi =
> > > +               container_of(work, struct virtnet_info, get_cvq);
> > > +       unsigned int tmp;
> > > +       void *res;
> > > +
> > > +       mutex_lock(&vi->cvq_lock);
> > > +       res = virtqueue_get_buf(vi->cvq, &tmp);
> > > +       if (res)
> > > +               complete(&vi->completion);
> > > +       mutex_unlock(&vi->cvq_lock);
> > > +}
> > > +
> > >  static void skb_recv_done(struct virtqueue *rvq)
> > >  {
> > >         struct virtnet_info *vi = rvq->vdev->priv;
> > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > >                                  struct scatterlist *out)
> > >  {
> > >         struct scatterlist *sgs[4], hdr, stat;
> > > -       unsigned out_num = 0, tmp;
> > > +       unsigned out_num = 0;
> > >         int ret;
> > >
> > >         /* Caller should know better */
> > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > >                 return vi->ctrl->status == VIRTIO_NET_OK;
> > >         }
> > >
> > > -       /* Spin for a response, the kick causes an ioport write, trapping
> > > -        * into the hypervisor, so the request should be handled immediately.
> > > -        */
> > > -       while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > -              !virtqueue_is_broken(vi->cvq)) {
> > > -               cond_resched();
> > > -               cpu_relax();
> > > -       }
> > > -
> > >         mutex_unlock(&vi->cvq_lock);
> > > +
> > > +       wait_for_completion(&vi->completion);
> > > +
> >
> > A question here, can multiple cvq requests be submitted to the device?
> > If yes, what happens if the device completes them out of order?
>
> For user commands (such as ethtool cmds), multiple cvq requests is not allowed.
> because it holds the netlink lock when waiting for the response.
>
> For multiple dim commands and a user command allowed to be sent simultaneously
> , the corresponding command-specific information(desc_state) will be used to
> distinguish different responses.

Just to make sure we are on the same page. I meant at least we are
still use the global completion which seems to be problematic.

wait_for_completion(&vi->completion);

Thanks


>
> Thanks.
>
> >
> > Thanks
> >
> > >         return vi->ctrl->status == VIRTIO_NET_OK;
> > >  }
> > >
> > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > >
> > >         /* Parameters for control virtqueue, if any */
> > >         if (vi->has_cvq) {
> > > -               callbacks[total_vqs - 1] = NULL;
> > > +               callbacks[total_vqs - 1] = virtnet_cvq_done;
> > >                 names[total_vqs - 1] = "control";
> > >         }
> > >
> > > @@ -4952,6 +4973,8 @@ 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);
> > >
> > >         /* serialize netdev register + virtio_device_ready() with ndo_open() */
> > > --
> > > 2.32.0.3.g01195cf9f
> > >
> >
>

Re: [PATCH net-next 1/3] virtio_net: enable irq for the control vq

[Heng Qi]

On Tue, 7 May 2024 14:24:16 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, May 7, 2024 at 12:03 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> > On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > >
> > > > Control vq polling request results consume more CPU.
> > > > Especially when dim issues more control requests to the device,
> > > > it's beneficial to the guest to enable control vq's irq.
> > > >
> > > > Suggested-by: Jason Wang <jasowang@redhat.com>
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > ---
> > > >  drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++----------
> > > >  1 file changed, 34 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index a4d3c76654a4..79a1b30c173c 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -287,6 +287,12 @@ struct virtnet_info {
> > > >         bool has_cvq;
> > > >         struct mutex cvq_lock;
> > > >
> > > > +       /* Wait for the device to complete the 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;
> > > >
> > > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
> > > >         return false;
> > > >  }
> > > >
> > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > +{
> > > > +       struct virtnet_info *vi = cvq->vdev->priv;
> > > > +
> > > > +       schedule_work(&vi->get_cvq);
> > > > +}
> > > > +
> > > >  static void skb_xmit_done(struct virtqueue *vq)
> > > >  {
> > > >         struct virtnet_info *vi = vq->vdev->priv;
> > > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > >         return !oom;
> > > >  }
> > > >
> > > > +static void virtnet_get_cvq_work(struct work_struct *work)
> > > > +{
> > > > +       struct virtnet_info *vi =
> > > > +               container_of(work, struct virtnet_info, get_cvq);
> > > > +       unsigned int tmp;
> > > > +       void *res;
> > > > +
> > > > +       mutex_lock(&vi->cvq_lock);
> > > > +       res = virtqueue_get_buf(vi->cvq, &tmp);
> > > > +       if (res)
> > > > +               complete(&vi->completion);
> > > > +       mutex_unlock(&vi->cvq_lock);
> > > > +}
> > > > +
> > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > >  {
> > > >         struct virtnet_info *vi = rvq->vdev->priv;
> > > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > >                                  struct scatterlist *out)
> > > >  {
> > > >         struct scatterlist *sgs[4], hdr, stat;
> > > > -       unsigned out_num = 0, tmp;
> > > > +       unsigned out_num = 0;
> > > >         int ret;
> > > >
> > > >         /* Caller should know better */
> > > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > >                 return vi->ctrl->status == VIRTIO_NET_OK;
> > > >         }
> > > >
> > > > -       /* Spin for a response, the kick causes an ioport write, trapping
> > > > -        * into the hypervisor, so the request should be handled immediately.
> > > > -        */
> > > > -       while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > -              !virtqueue_is_broken(vi->cvq)) {
> > > > -               cond_resched();
> > > > -               cpu_relax();
> > > > -       }
> > > > -
> > > >         mutex_unlock(&vi->cvq_lock);
> > > > +
> > > > +       wait_for_completion(&vi->completion);
> > > > +
> > >
> > > A question here, can multiple cvq requests be submitted to the device?
> > > If yes, what happens if the device completes them out of order?
> >
> > For user commands (such as ethtool cmds), multiple cvq requests is not allowed.
> > because it holds the netlink lock when waiting for the response.
> >
> > For multiple dim commands and a user command allowed to be sent simultaneously
> > , the corresponding command-specific information(desc_state) will be used to
> > distinguish different responses.
> 
> Just to make sure we are on the same page. I meant at least we are
> still use the global completion which seems to be problematic.
> 
> wait_for_completion(&vi->completion);


This completion is only used by the ethtool command, so it is the global one.

dim commands use specific coal_free_list and coal_wait_list to complete
multiple command issuance (please see patch 3).

Thanks.

> 
> Thanks
> 
> 
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > > >         return vi->ctrl->status == VIRTIO_NET_OK;
> > > >  }
> > > >
> > > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > >
> > > >         /* Parameters for control virtqueue, if any */
> > > >         if (vi->has_cvq) {
> > > > -               callbacks[total_vqs - 1] = NULL;
> > > > +               callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > >                 names[total_vqs - 1] = "control";
> > > >         }
> > > >
> > > > @@ -4952,6 +4973,8 @@ 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);
> > > >
> > > >         /* serialize netdev register + virtio_device_ready() with ndo_open() */
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > > >
> > >
> >
> 

Re: [PATCH net-next 1/3] virtio_net: enable irq for the control vq

[Jason Wang]

On Tue, May 7, 2024 at 2:33 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
> On Tue, 7 May 2024 14:24:16 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Tue, May 7, 2024 at 12:03 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > >
> > > On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > >
> > > > > Control vq polling request results consume more CPU.
> > > > > Especially when dim issues more control requests to the device,
> > > > > it's beneficial to the guest to enable control vq's irq.
> > > > >
> > > > > Suggested-by: Jason Wang <jasowang@redhat.com>
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > ---
> > > > >  drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++----------
> > > > >  1 file changed, 34 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index a4d3c76654a4..79a1b30c173c 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -287,6 +287,12 @@ struct virtnet_info {
> > > > >         bool has_cvq;
> > > > >         struct mutex cvq_lock;
> > > > >
> > > > > +       /* Wait for the device to complete the 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;
> > > > >
> > > > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
> > > > >         return false;
> > > > >  }
> > > > >
> > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > +{
> > > > > +       struct virtnet_info *vi = cvq->vdev->priv;
> > > > > +
> > > > > +       schedule_work(&vi->get_cvq);
> > > > > +}
> > > > > +
> > > > >  static void skb_xmit_done(struct virtqueue *vq)
> > > > >  {
> > > > >         struct virtnet_info *vi = vq->vdev->priv;
> > > > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > >         return !oom;
> > > > >  }
> > > > >
> > > > > +static void virtnet_get_cvq_work(struct work_struct *work)
> > > > > +{
> > > > > +       struct virtnet_info *vi =
> > > > > +               container_of(work, struct virtnet_info, get_cvq);
> > > > > +       unsigned int tmp;
> > > > > +       void *res;
> > > > > +
> > > > > +       mutex_lock(&vi->cvq_lock);
> > > > > +       res = virtqueue_get_buf(vi->cvq, &tmp);
> > > > > +       if (res)
> > > > > +               complete(&vi->completion);
> > > > > +       mutex_unlock(&vi->cvq_lock);
> > > > > +}
> > > > > +
> > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > >  {
> > > > >         struct virtnet_info *vi = rvq->vdev->priv;
> > > > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > >                                  struct scatterlist *out)
> > > > >  {
> > > > >         struct scatterlist *sgs[4], hdr, stat;
> > > > > -       unsigned out_num = 0, tmp;
> > > > > +       unsigned out_num = 0;
> > > > >         int ret;
> > > > >
> > > > >         /* Caller should know better */
> > > > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > >                 return vi->ctrl->status == VIRTIO_NET_OK;
> > > > >         }
> > > > >
> > > > > -       /* Spin for a response, the kick causes an ioport write, trapping
> > > > > -        * into the hypervisor, so the request should be handled immediately.
> > > > > -        */
> > > > > -       while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > -              !virtqueue_is_broken(vi->cvq)) {
> > > > > -               cond_resched();
> > > > > -               cpu_relax();
> > > > > -       }
> > > > > -
> > > > >         mutex_unlock(&vi->cvq_lock);
> > > > > +
> > > > > +       wait_for_completion(&vi->completion);
> > > > > +
> > > >
> > > > A question here, can multiple cvq requests be submitted to the device?
> > > > If yes, what happens if the device completes them out of order?
> > >
> > > For user commands (such as ethtool cmds), multiple cvq requests is not allowed.
> > > because it holds the netlink lock when waiting for the response.
> > >
> > > For multiple dim commands and a user command allowed to be sent simultaneously
> > > , the corresponding command-specific information(desc_state) will be used to
> > > distinguish different responses.
> >
> > Just to make sure we are on the same page. I meant at least we are
> > still use the global completion which seems to be problematic.
> >
> > wait_for_completion(&vi->completion);
>
>
> This completion is only used by the ethtool command, so it is the global one.
>
> dim commands use specific coal_free_list and coal_wait_list to complete
> multiple command issuance (please see patch 3).

If I was not wrong, at least for this patch the global completion will
be used by both dim and rtnl. If yes, it's not good to introduce a bug
in patch 1 and fix it in patch 3.

Thanks

>
> Thanks.
>
> >
> > Thanks
> >
> >
> > >
> > > Thanks.
> > >
> > > >
> > > > Thanks
> > > >
> > > > >         return vi->ctrl->status == VIRTIO_NET_OK;
> > > > >  }
> > > > >
> > > > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > >
> > > > >         /* Parameters for control virtqueue, if any */
> > > > >         if (vi->has_cvq) {
> > > > > -               callbacks[total_vqs - 1] = NULL;
> > > > > +               callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > >                 names[total_vqs - 1] = "control";
> > > > >         }
> > > > >
> > > > > @@ -4952,6 +4973,8 @@ 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);
> > > > >
> > > > >         /* serialize netdev register + virtio_device_ready() with ndo_open() */
> > > > > --
> > > > > 2.32.0.3.g01195cf9f
> > > > >
> > > >
> > >
> >
>

Re: [PATCH net-next 1/3] virtio_net: enable irq for the control vq

[Heng Qi]

On Wed, 8 May 2024 10:19:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, May 7, 2024 at 2:33 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> > On Tue, 7 May 2024 14:24:16 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Tue, May 7, 2024 at 12:03 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > >
> > > > On Tue, 7 May 2024 11:15:22 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Thu, Apr 25, 2024 at 8:59 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > > >
> > > > > > Control vq polling request results consume more CPU.
> > > > > > Especially when dim issues more control requests to the device,
> > > > > > it's beneficial to the guest to enable control vq's irq.
> > > > > >
> > > > > > Suggested-by: Jason Wang <jasowang@redhat.com>
> > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > ---
> > > > > >  drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++++----------
> > > > > >  1 file changed, 34 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > index a4d3c76654a4..79a1b30c173c 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -287,6 +287,12 @@ struct virtnet_info {
> > > > > >         bool has_cvq;
> > > > > >         struct mutex cvq_lock;
> > > > > >
> > > > > > +       /* Wait for the device to complete the 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;
> > > > > >
> > > > > > @@ -520,6 +526,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
> > > > > >         return false;
> > > > > >  }
> > > > > >
> > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > > +{
> > > > > > +       struct virtnet_info *vi = cvq->vdev->priv;
> > > > > > +
> > > > > > +       schedule_work(&vi->get_cvq);
> > > > > > +}
> > > > > > +
> > > > > >  static void skb_xmit_done(struct virtqueue *vq)
> > > > > >  {
> > > > > >         struct virtnet_info *vi = vq->vdev->priv;
> > > > > > @@ -2036,6 +2049,20 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > >         return !oom;
> > > > > >  }
> > > > > >
> > > > > > +static void virtnet_get_cvq_work(struct work_struct *work)
> > > > > > +{
> > > > > > +       struct virtnet_info *vi =
> > > > > > +               container_of(work, struct virtnet_info, get_cvq);
> > > > > > +       unsigned int tmp;
> > > > > > +       void *res;
> > > > > > +
> > > > > > +       mutex_lock(&vi->cvq_lock);
> > > > > > +       res = virtqueue_get_buf(vi->cvq, &tmp);
> > > > > > +       if (res)
> > > > > > +               complete(&vi->completion);
> > > > > > +       mutex_unlock(&vi->cvq_lock);
> > > > > > +}
> > > > > > +
> > > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > > >  {
> > > > > >         struct virtnet_info *vi = rvq->vdev->priv;
> > > > > > @@ -2531,7 +2558,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > >                                  struct scatterlist *out)
> > > > > >  {
> > > > > >         struct scatterlist *sgs[4], hdr, stat;
> > > > > > -       unsigned out_num = 0, tmp;
> > > > > > +       unsigned out_num = 0;
> > > > > >         int ret;
> > > > > >
> > > > > >         /* Caller should know better */
> > > > > > @@ -2566,16 +2593,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > >                 return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > >         }
> > > > > >
> > > > > > -       /* Spin for a response, the kick causes an ioport write, trapping
> > > > > > -        * into the hypervisor, so the request should be handled immediately.
> > > > > > -        */
> > > > > > -       while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > -              !virtqueue_is_broken(vi->cvq)) {
> > > > > > -               cond_resched();
> > > > > > -               cpu_relax();
> > > > > > -       }
> > > > > > -
> > > > > >         mutex_unlock(&vi->cvq_lock);
> > > > > > +
> > > > > > +       wait_for_completion(&vi->completion);
> > > > > > +
> > > > >
> > > > > A question here, can multiple cvq requests be submitted to the device?
> > > > > If yes, what happens if the device completes them out of order?
> > > >
> > > > For user commands (such as ethtool cmds), multiple cvq requests is not allowed.
> > > > because it holds the netlink lock when waiting for the response.
> > > >
> > > > For multiple dim commands and a user command allowed to be sent simultaneously
> > > > , the corresponding command-specific information(desc_state) will be used to
> > > > distinguish different responses.
> > >
> > > Just to make sure we are on the same page. I meant at least we are
> > > still use the global completion which seems to be problematic.
> > >
> > > wait_for_completion(&vi->completion);
> >
> >
> > This completion is only used by the ethtool command, so it is the global one.
> >
> > dim commands use specific coal_free_list and coal_wait_list to complete
> > multiple command issuance (please see patch 3).
> 
> If I was not wrong, at least for this patch the global completion will
> be used by both dim and rtnl. If yes, it's not good to introduce a bug
> in patch 1 and fix it in patch 3.

Ok, patches 1 and 3 will be refactored.

Thanks.

> 
> Thanks
> 
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >         return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > >  }
> > > > > >
> > > > > > @@ -4433,7 +4454,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > > >
> > > > > >         /* Parameters for control virtqueue, if any */
> > > > > >         if (vi->has_cvq) {
> > > > > > -               callbacks[total_vqs - 1] = NULL;
> > > > > > +               callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > > >                 names[total_vqs - 1] = "control";
> > > > > >         }
> > > > > >
> > > > > > @@ -4952,6 +4973,8 @@ 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);
> > > > > >
> > > > > >         /* serialize netdev register + virtio_device_ready() with ndo_open() */
> > > > > > --
> > > > > > 2.32.0.3.g01195cf9f
> > > > > >
> > > > >
> > > >
> > >
> >
>