[PATCH 0/3] vdpa_sim_blk: several fixes for the vDPA block simulator

[PATCH 0/3] vdpa_sim_blk: several fixes for the vDPA block simulator

[Stefano Garzarella]

The first two patches essentially limit the possibility of the guest
doing a DoS to the host.

The third makes the simulator more correct (following what we do in
vdpa_sim_net) by calling vringh_complete_iotlb() in the error path as well.

Stefano Garzarella (3):
  vdpa_sim_blk: use dev_dbg() to print errors
  vdpa_sim_blk: limit the number of request handled per batch
  vdpa_sim_blk: call vringh_complete_iotlb() also in the error path

 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 49 +++++++++++++++++++---------
 1 file changed, 33 insertions(+), 16 deletions(-)

-- 
2.36.1

[PATCH 0/3] vdpa_sim_blk: several fixes for the vDPA block simulator

[Stefano Garzarella]

The first two patches essentially limit the possibility of the guest
doing a DoS to the host.

The third makes the simulator more correct (following what we do in
vdpa_sim_net) by calling vringh_complete_iotlb() in the error path as well.

Stefano Garzarella (3):
  vdpa_sim_blk: use dev_dbg() to print errors
  vdpa_sim_blk: limit the number of request handled per batch
  vdpa_sim_blk: call vringh_complete_iotlb() also in the error path

 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 49 +++++++++++++++++++---------
 1 file changed, 33 insertions(+), 16 deletions(-)

-- 
2.36.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors

[Stefano Garzarella]

Use dev_dbg() instead of dev_err()/dev_warn() to avoid flooding the
host with prints, when the guest driver is misbehaving.
In this way, prints can be dynamically enabled when the vDPA block
simulator is used to validate a driver.

Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index 42d401d43911..a83a5c76f620 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -76,13 +76,13 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 		return false;
 
 	if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
-		dev_err(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
+		dev_dbg(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
 			vq->out_iov.used, vq->in_iov.used);
 		return false;
 	}
 
 	if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
-		dev_err(&vdpasim->vdpa.dev, "request in header too short\n");
+		dev_dbg(&vdpasim->vdpa.dev, "request in header too short\n");
 		return false;
 	}
 
@@ -96,7 +96,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 	bytes = vringh_iov_pull_iotlb(&vq->vring, &vq->out_iov, &hdr,
 				      sizeof(hdr));
 	if (bytes != sizeof(hdr)) {
-		dev_err(&vdpasim->vdpa.dev, "request out header too short\n");
+		dev_dbg(&vdpasim->vdpa.dev, "request out header too short\n");
 		return false;
 	}
 
@@ -110,7 +110,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 	switch (type) {
 	case VIRTIO_BLK_T_IN:
 		if (!vdpasim_blk_check_range(sector, to_push)) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"reading over the capacity - offset: 0x%llx len: 0x%zx\n",
 				offset, to_push);
 			status = VIRTIO_BLK_S_IOERR;
@@ -121,7 +121,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 					      vdpasim->buffer + offset,
 					      to_push);
 		if (bytes < 0) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"vringh_iov_push_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
 				bytes, offset, to_push);
 			status = VIRTIO_BLK_S_IOERR;
@@ -133,7 +133,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 
 	case VIRTIO_BLK_T_OUT:
 		if (!vdpasim_blk_check_range(sector, to_pull)) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"writing over the capacity - offset: 0x%llx len: 0x%zx\n",
 				offset, to_pull);
 			status = VIRTIO_BLK_S_IOERR;
@@ -144,7 +144,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 					      vdpasim->buffer + offset,
 					      to_pull);
 		if (bytes < 0) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"vringh_iov_pull_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
 				bytes, offset, to_pull);
 			status = VIRTIO_BLK_S_IOERR;
@@ -157,7 +157,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 					      vdpasim_blk_id,
 					      VIRTIO_BLK_ID_BYTES);
 		if (bytes < 0) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"vringh_iov_push_iotlb() error: %zd\n", bytes);
 			status = VIRTIO_BLK_S_IOERR;
 			break;
@@ -167,8 +167,8 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 		break;
 
 	default:
-		dev_warn(&vdpasim->vdpa.dev,
-			 "Unsupported request type %d\n", type);
+		dev_dbg(&vdpasim->vdpa.dev,
+			"Unsupported request type %d\n", type);
 		status = VIRTIO_BLK_S_IOERR;
 		break;
 	}
-- 
2.36.1

[PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors

[Stefano Garzarella]

Use dev_dbg() instead of dev_err()/dev_warn() to avoid flooding the
host with prints, when the guest driver is misbehaving.
In this way, prints can be dynamically enabled when the vDPA block
simulator is used to validate a driver.

Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index 42d401d43911..a83a5c76f620 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -76,13 +76,13 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 		return false;
 
 	if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
-		dev_err(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
+		dev_dbg(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
 			vq->out_iov.used, vq->in_iov.used);
 		return false;
 	}
 
 	if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
-		dev_err(&vdpasim->vdpa.dev, "request in header too short\n");
+		dev_dbg(&vdpasim->vdpa.dev, "request in header too short\n");
 		return false;
 	}
 
@@ -96,7 +96,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 	bytes = vringh_iov_pull_iotlb(&vq->vring, &vq->out_iov, &hdr,
 				      sizeof(hdr));
 	if (bytes != sizeof(hdr)) {
-		dev_err(&vdpasim->vdpa.dev, "request out header too short\n");
+		dev_dbg(&vdpasim->vdpa.dev, "request out header too short\n");
 		return false;
 	}
 
@@ -110,7 +110,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 	switch (type) {
 	case VIRTIO_BLK_T_IN:
 		if (!vdpasim_blk_check_range(sector, to_push)) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"reading over the capacity - offset: 0x%llx len: 0x%zx\n",
 				offset, to_push);
 			status = VIRTIO_BLK_S_IOERR;
@@ -121,7 +121,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 					      vdpasim->buffer + offset,
 					      to_push);
 		if (bytes < 0) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"vringh_iov_push_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
 				bytes, offset, to_push);
 			status = VIRTIO_BLK_S_IOERR;
@@ -133,7 +133,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 
 	case VIRTIO_BLK_T_OUT:
 		if (!vdpasim_blk_check_range(sector, to_pull)) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"writing over the capacity - offset: 0x%llx len: 0x%zx\n",
 				offset, to_pull);
 			status = VIRTIO_BLK_S_IOERR;
@@ -144,7 +144,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 					      vdpasim->buffer + offset,
 					      to_pull);
 		if (bytes < 0) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"vringh_iov_pull_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
 				bytes, offset, to_pull);
 			status = VIRTIO_BLK_S_IOERR;
@@ -157,7 +157,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 					      vdpasim_blk_id,
 					      VIRTIO_BLK_ID_BYTES);
 		if (bytes < 0) {
-			dev_err(&vdpasim->vdpa.dev,
+			dev_dbg(&vdpasim->vdpa.dev,
 				"vringh_iov_push_iotlb() error: %zd\n", bytes);
 			status = VIRTIO_BLK_S_IOERR;
 			break;
@@ -167,8 +167,8 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 		break;
 
 	default:
-		dev_warn(&vdpasim->vdpa.dev,
-			 "Unsupported request type %d\n", type);
+		dev_dbg(&vdpasim->vdpa.dev,
+			"Unsupported request type %d\n", type);
 		status = VIRTIO_BLK_S_IOERR;
 		break;
 	}
-- 
2.36.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Stefano Garzarella]

Limit the number of requests (4 per queue as for vdpa_sim_net) handled
in a batch to prevent the worker from using the CPU for too long.

Suggested-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index a83a5c76f620..ac86478845b6 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 static void vdpasim_blk_work(struct work_struct *work)
 {
 	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
+	bool reschedule = false;
 	int i;
 
 	spin_lock(&vdpasim->lock);
@@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
 
 	for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
 		struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
+		bool vq_work = true;
+		int reqs = 0;
 
 		if (!vq->ready)
 			continue;
 
-		while (vdpasim_blk_handle_req(vdpasim, vq)) {
+		while (vq_work) {
+			vq_work = vdpasim_blk_handle_req(vdpasim, vq);
+
 			/* Make sure used is visible before rasing the interrupt. */
 			smp_wmb();
 
@@ -218,10 +223,18 @@ static void vdpasim_blk_work(struct work_struct *work)
 			if (vringh_need_notify_iotlb(&vq->vring) > 0)
 				vringh_notify(&vq->vring);
 			local_bh_enable();
+
+			if (++reqs > 4) {
+				vq_work = false;
+				reschedule = true;
+			}
 		}
 	}
 out:
 	spin_unlock(&vdpasim->lock);
+
+	if (reschedule)
+		schedule_work(&vdpasim->work);
 }
 
 static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
-- 
2.36.1

[PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Stefano Garzarella]

Limit the number of requests (4 per queue as for vdpa_sim_net) handled
in a batch to prevent the worker from using the CPU for too long.

Suggested-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index a83a5c76f620..ac86478845b6 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 static void vdpasim_blk_work(struct work_struct *work)
 {
 	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
+	bool reschedule = false;
 	int i;
 
 	spin_lock(&vdpasim->lock);
@@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
 
 	for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
 		struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
+		bool vq_work = true;
+		int reqs = 0;
 
 		if (!vq->ready)
 			continue;
 
-		while (vdpasim_blk_handle_req(vdpasim, vq)) {
+		while (vq_work) {
+			vq_work = vdpasim_blk_handle_req(vdpasim, vq);
+
 			/* Make sure used is visible before rasing the interrupt. */
 			smp_wmb();
 
@@ -218,10 +223,18 @@ static void vdpasim_blk_work(struct work_struct *work)
 			if (vringh_need_notify_iotlb(&vq->vring) > 0)
 				vringh_notify(&vq->vring);
 			local_bh_enable();
+
+			if (++reqs > 4) {
+				vq_work = false;
+				reschedule = true;
+			}
 		}
 	}
 out:
 	spin_unlock(&vdpasim->lock);
+
+	if (reschedule)
+		schedule_work(&vdpasim->work);
 }
 
 static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
-- 
2.36.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path

[Stefano Garzarella]

Call vringh_complete_iotlb() even when we encounter a serious error
that prevents us from writing the status in the "in" header
(e.g. the header length is incorrect, etc.).

The guest is misbehaving, so maybe the ring is in a bad state, but
let's avoid making things worse.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index ac86478845b6..de9cd9843143 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -63,6 +63,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 {
 	size_t pushed = 0, to_pull, to_push;
 	struct virtio_blk_outhdr hdr;
+	bool handled = false;
 	ssize_t bytes;
 	loff_t offset;
 	u64 sector;
@@ -78,12 +79,12 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 	if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
 		dev_dbg(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
 			vq->out_iov.used, vq->in_iov.used);
-		return false;
+		goto err;
 	}
 
 	if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
 		dev_dbg(&vdpasim->vdpa.dev, "request in header too short\n");
-		return false;
+		goto err;
 	}
 
 	/* The last byte is the status and we checked if the last iov has
@@ -97,7 +98,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 				      sizeof(hdr));
 	if (bytes != sizeof(hdr)) {
 		dev_dbg(&vdpasim->vdpa.dev, "request out header too short\n");
-		return false;
+		goto err;
 	}
 
 	to_pull -= bytes;
@@ -182,16 +183,19 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 	/* Last byte is the status */
 	bytes = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov, &status, 1);
 	if (bytes != 1)
-		return false;
+		goto err;
 
 	pushed += bytes;
 
 	/* Make sure data is wrote before advancing index */
 	smp_wmb();
 
+	handled = true;
+
+err:
 	vringh_complete_iotlb(&vq->vring, vq->head, pushed);
 
-	return true;
+	return handled;
 }
 
 static void vdpasim_blk_work(struct work_struct *work)
-- 
2.36.1

[PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path

[Stefano Garzarella]

Call vringh_complete_iotlb() even when we encounter a serious error
that prevents us from writing the status in the "in" header
(e.g. the header length is incorrect, etc.).

The guest is misbehaving, so maybe the ring is in a bad state, but
let's avoid making things worse.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index ac86478845b6..de9cd9843143 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -63,6 +63,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 {
 	size_t pushed = 0, to_pull, to_push;
 	struct virtio_blk_outhdr hdr;
+	bool handled = false;
 	ssize_t bytes;
 	loff_t offset;
 	u64 sector;
@@ -78,12 +79,12 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 	if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
 		dev_dbg(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
 			vq->out_iov.used, vq->in_iov.used);
-		return false;
+		goto err;
 	}
 
 	if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
 		dev_dbg(&vdpasim->vdpa.dev, "request in header too short\n");
-		return false;
+		goto err;
 	}
 
 	/* The last byte is the status and we checked if the last iov has
@@ -97,7 +98,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 				      sizeof(hdr));
 	if (bytes != sizeof(hdr)) {
 		dev_dbg(&vdpasim->vdpa.dev, "request out header too short\n");
-		return false;
+		goto err;
 	}
 
 	to_pull -= bytes;
@@ -182,16 +183,19 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
 	/* Last byte is the status */
 	bytes = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov, &status, 1);
 	if (bytes != 1)
-		return false;
+		goto err;
 
 	pushed += bytes;
 
 	/* Make sure data is wrote before advancing index */
 	smp_wmb();
 
+	handled = true;
+
+err:
 	vringh_complete_iotlb(&vq->vring, vq->head, pushed);
 
-	return true;
+	return handled;
 }
 
 static void vdpasim_blk_work(struct work_struct *work)
-- 
2.36.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors

[Jason Wang]

On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Use dev_dbg() instead of dev_err()/dev_warn() to avoid flooding the
> host with prints, when the guest driver is misbehaving.
> In this way, prints can be dynamically enabled when the vDPA block
> simulator is used to validate a driver.
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index 42d401d43911..a83a5c76f620 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -76,13 +76,13 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                 return false;
>
>         if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
> -               dev_err(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
> +               dev_dbg(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
>                         vq->out_iov.used, vq->in_iov.used);
>                 return false;
>         }
>
>         if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
> -               dev_err(&vdpasim->vdpa.dev, "request in header too short\n");
> +               dev_dbg(&vdpasim->vdpa.dev, "request in header too short\n");
>                 return false;
>         }
>
> @@ -96,7 +96,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>         bytes = vringh_iov_pull_iotlb(&vq->vring, &vq->out_iov, &hdr,
>                                       sizeof(hdr));
>         if (bytes != sizeof(hdr)) {
> -               dev_err(&vdpasim->vdpa.dev, "request out header too short\n");
> +               dev_dbg(&vdpasim->vdpa.dev, "request out header too short\n");
>                 return false;
>         }
>
> @@ -110,7 +110,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>         switch (type) {
>         case VIRTIO_BLK_T_IN:
>                 if (!vdpasim_blk_check_range(sector, to_push)) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "reading over the capacity - offset: 0x%llx len: 0x%zx\n",
>                                 offset, to_push);
>                         status = VIRTIO_BLK_S_IOERR;
> @@ -121,7 +121,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                                               vdpasim->buffer + offset,
>                                               to_push);
>                 if (bytes < 0) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "vringh_iov_push_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
>                                 bytes, offset, to_push);
>                         status = VIRTIO_BLK_S_IOERR;
> @@ -133,7 +133,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>
>         case VIRTIO_BLK_T_OUT:
>                 if (!vdpasim_blk_check_range(sector, to_pull)) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "writing over the capacity - offset: 0x%llx len: 0x%zx\n",
>                                 offset, to_pull);
>                         status = VIRTIO_BLK_S_IOERR;
> @@ -144,7 +144,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                                               vdpasim->buffer + offset,
>                                               to_pull);
>                 if (bytes < 0) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "vringh_iov_pull_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
>                                 bytes, offset, to_pull);
>                         status = VIRTIO_BLK_S_IOERR;
> @@ -157,7 +157,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                                               vdpasim_blk_id,
>                                               VIRTIO_BLK_ID_BYTES);
>                 if (bytes < 0) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "vringh_iov_push_iotlb() error: %zd\n", bytes);
>                         status = VIRTIO_BLK_S_IOERR;
>                         break;
> @@ -167,8 +167,8 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                 break;
>
>         default:
> -               dev_warn(&vdpasim->vdpa.dev,
> -                        "Unsupported request type %d\n", type);
> +               dev_dbg(&vdpasim->vdpa.dev,
> +                       "Unsupported request type %d\n", type);
>                 status = VIRTIO_BLK_S_IOERR;
>                 break;
>         }
> --
> 2.36.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors

[Jason Wang]

On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Use dev_dbg() instead of dev_err()/dev_warn() to avoid flooding the
> host with prints, when the guest driver is misbehaving.
> In this way, prints can be dynamically enabled when the vDPA block
> simulator is used to validate a driver.
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index 42d401d43911..a83a5c76f620 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -76,13 +76,13 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                 return false;
>
>         if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
> -               dev_err(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
> +               dev_dbg(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
>                         vq->out_iov.used, vq->in_iov.used);
>                 return false;
>         }
>
>         if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
> -               dev_err(&vdpasim->vdpa.dev, "request in header too short\n");
> +               dev_dbg(&vdpasim->vdpa.dev, "request in header too short\n");
>                 return false;
>         }
>
> @@ -96,7 +96,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>         bytes = vringh_iov_pull_iotlb(&vq->vring, &vq->out_iov, &hdr,
>                                       sizeof(hdr));
>         if (bytes != sizeof(hdr)) {
> -               dev_err(&vdpasim->vdpa.dev, "request out header too short\n");
> +               dev_dbg(&vdpasim->vdpa.dev, "request out header too short\n");
>                 return false;
>         }
>
> @@ -110,7 +110,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>         switch (type) {
>         case VIRTIO_BLK_T_IN:
>                 if (!vdpasim_blk_check_range(sector, to_push)) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "reading over the capacity - offset: 0x%llx len: 0x%zx\n",
>                                 offset, to_push);
>                         status = VIRTIO_BLK_S_IOERR;
> @@ -121,7 +121,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                                               vdpasim->buffer + offset,
>                                               to_push);
>                 if (bytes < 0) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "vringh_iov_push_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
>                                 bytes, offset, to_push);
>                         status = VIRTIO_BLK_S_IOERR;
> @@ -133,7 +133,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>
>         case VIRTIO_BLK_T_OUT:
>                 if (!vdpasim_blk_check_range(sector, to_pull)) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "writing over the capacity - offset: 0x%llx len: 0x%zx\n",
>                                 offset, to_pull);
>                         status = VIRTIO_BLK_S_IOERR;
> @@ -144,7 +144,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                                               vdpasim->buffer + offset,
>                                               to_pull);
>                 if (bytes < 0) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "vringh_iov_pull_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
>                                 bytes, offset, to_pull);
>                         status = VIRTIO_BLK_S_IOERR;
> @@ -157,7 +157,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                                               vdpasim_blk_id,
>                                               VIRTIO_BLK_ID_BYTES);
>                 if (bytes < 0) {
> -                       dev_err(&vdpasim->vdpa.dev,
> +                       dev_dbg(&vdpasim->vdpa.dev,
>                                 "vringh_iov_push_iotlb() error: %zd\n", bytes);
>                         status = VIRTIO_BLK_S_IOERR;
>                         break;
> @@ -167,8 +167,8 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                 break;
>
>         default:
> -               dev_warn(&vdpasim->vdpa.dev,
> -                        "Unsupported request type %d\n", type);
> +               dev_dbg(&vdpasim->vdpa.dev,
> +                       "Unsupported request type %d\n", type);
>                 status = VIRTIO_BLK_S_IOERR;
>                 break;
>         }
> --
> 2.36.1
>

Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Jason Wang]

On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Limit the number of requests (4 per queue as for vdpa_sim_net) handled
> in a batch to prevent the worker from using the CPU for too long.
>
> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index a83a5c76f620..ac86478845b6 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>  static void vdpasim_blk_work(struct work_struct *work)
>  {
>         struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> +       bool reschedule = false;
>         int i;
>
>         spin_lock(&vdpasim->lock);
> @@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
>
>         for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
>                 struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> +               bool vq_work = true;
> +               int reqs = 0;
>
>                 if (!vq->ready)
>                         continue;
>
> -               while (vdpasim_blk_handle_req(vdpasim, vq)) {
> +               while (vq_work) {
> +                       vq_work = vdpasim_blk_handle_req(vdpasim, vq);
> +

Is it better to check and exit the loop early here?

Thanks

>                         /* Make sure used is visible before rasing the interrupt. */
>                         smp_wmb();
>
> @@ -218,10 +223,18 @@ static void vdpasim_blk_work(struct work_struct *work)
>                         if (vringh_need_notify_iotlb(&vq->vring) > 0)
>                                 vringh_notify(&vq->vring);
>                         local_bh_enable();
> +
> +                       if (++reqs > 4) {
> +                               vq_work = false;
> +                               reschedule = true;
> +                       }
>                 }
>         }
>  out:
>         spin_unlock(&vdpasim->lock);
> +
> +       if (reschedule)
> +               schedule_work(&vdpasim->work);
>  }
>
>  static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
> --
> 2.36.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Jason Wang]

On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Limit the number of requests (4 per queue as for vdpa_sim_net) handled
> in a batch to prevent the worker from using the CPU for too long.
>
> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index a83a5c76f620..ac86478845b6 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>  static void vdpasim_blk_work(struct work_struct *work)
>  {
>         struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> +       bool reschedule = false;
>         int i;
>
>         spin_lock(&vdpasim->lock);
> @@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
>
>         for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
>                 struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> +               bool vq_work = true;
> +               int reqs = 0;
>
>                 if (!vq->ready)
>                         continue;
>
> -               while (vdpasim_blk_handle_req(vdpasim, vq)) {
> +               while (vq_work) {
> +                       vq_work = vdpasim_blk_handle_req(vdpasim, vq);
> +

Is it better to check and exit the loop early here?

Thanks

>                         /* Make sure used is visible before rasing the interrupt. */
>                         smp_wmb();
>
> @@ -218,10 +223,18 @@ static void vdpasim_blk_work(struct work_struct *work)
>                         if (vringh_need_notify_iotlb(&vq->vring) > 0)
>                                 vringh_notify(&vq->vring);
>                         local_bh_enable();
> +
> +                       if (++reqs > 4) {
> +                               vq_work = false;
> +                               reschedule = true;
> +                       }
>                 }
>         }
>  out:
>         spin_unlock(&vdpasim->lock);
> +
> +       if (reschedule)
> +               schedule_work(&vdpasim->work);
>  }
>
>  static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
> --
> 2.36.1
>

Re: [PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path

[Jason Wang]

On Wed, Jun 22, 2022 at 12:13 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Call vringh_complete_iotlb() even when we encounter a serious error
> that prevents us from writing the status in the "in" header
> (e.g. the header length is incorrect, etc.).
>
> The guest is misbehaving, so maybe the ring is in a bad state, but
> let's avoid making things worse.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index ac86478845b6..de9cd9843143 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -63,6 +63,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>  {
>         size_t pushed = 0, to_pull, to_push;
>         struct virtio_blk_outhdr hdr;
> +       bool handled = false;
>         ssize_t bytes;
>         loff_t offset;
>         u64 sector;
> @@ -78,12 +79,12 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>         if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
>                 dev_dbg(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
>                         vq->out_iov.used, vq->in_iov.used);
> -               return false;
> +               goto err;
>         }
>
>         if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
>                 dev_dbg(&vdpasim->vdpa.dev, "request in header too short\n");
> -               return false;
> +               goto err;
>         }
>
>         /* The last byte is the status and we checked if the last iov has
> @@ -97,7 +98,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                                       sizeof(hdr));
>         if (bytes != sizeof(hdr)) {
>                 dev_dbg(&vdpasim->vdpa.dev, "request out header too short\n");
> -               return false;
> +               goto err;
>         }
>
>         to_pull -= bytes;
> @@ -182,16 +183,19 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>         /* Last byte is the status */
>         bytes = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov, &status, 1);
>         if (bytes != 1)
> -               return false;
> +               goto err;
>
>         pushed += bytes;
>
>         /* Make sure data is wrote before advancing index */
>         smp_wmb();
>
> +       handled = true;
> +
> +err:
>         vringh_complete_iotlb(&vq->vring, vq->head, pushed);
>
> -       return true;
> +       return handled;
>  }
>
>  static void vdpasim_blk_work(struct work_struct *work)
> --
> 2.36.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path

[Jason Wang]

On Wed, Jun 22, 2022 at 12:13 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Call vringh_complete_iotlb() even when we encounter a serious error
> that prevents us from writing the status in the "in" header
> (e.g. the header length is incorrect, etc.).
>
> The guest is misbehaving, so maybe the ring is in a bad state, but
> let's avoid making things worse.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index ac86478845b6..de9cd9843143 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -63,6 +63,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>  {
>         size_t pushed = 0, to_pull, to_push;
>         struct virtio_blk_outhdr hdr;
> +       bool handled = false;
>         ssize_t bytes;
>         loff_t offset;
>         u64 sector;
> @@ -78,12 +79,12 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>         if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
>                 dev_dbg(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
>                         vq->out_iov.used, vq->in_iov.used);
> -               return false;
> +               goto err;
>         }
>
>         if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
>                 dev_dbg(&vdpasim->vdpa.dev, "request in header too short\n");
> -               return false;
> +               goto err;
>         }
>
>         /* The last byte is the status and we checked if the last iov has
> @@ -97,7 +98,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>                                       sizeof(hdr));
>         if (bytes != sizeof(hdr)) {
>                 dev_dbg(&vdpasim->vdpa.dev, "request out header too short\n");
> -               return false;
> +               goto err;
>         }
>
>         to_pull -= bytes;
> @@ -182,16 +183,19 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>         /* Last byte is the status */
>         bytes = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov, &status, 1);
>         if (bytes != 1)
> -               return false;
> +               goto err;
>
>         pushed += bytes;
>
>         /* Make sure data is wrote before advancing index */
>         smp_wmb();
>
> +       handled = true;
> +
> +err:
>         vringh_complete_iotlb(&vq->vring, vq->head, pushed);
>
> -       return true;
> +       return handled;
>  }
>
>  static void vdpasim_blk_work(struct work_struct *work)
> --
> 2.36.1
>

Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Stefano Garzarella]

On Thu, Jun 23, 2022 at 11:50:22AM +0800, Jason Wang wrote:
>On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> Limit the number of requests (4 per queue as for vdpa_sim_net) handled
>> in a batch to prevent the worker from using the CPU for too long.
>>
>> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> index a83a5c76f620..ac86478845b6 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> @@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>>  static void vdpasim_blk_work(struct work_struct *work)
>>  {
>>         struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
>> +       bool reschedule = false;
>>         int i;
>>
>>         spin_lock(&vdpasim->lock);
>> @@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
>>
>>         for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
>>                 struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
>> +               bool vq_work = true;
>> +               int reqs = 0;
>>
>>                 if (!vq->ready)
>>                         continue;
>>
>> -               while (vdpasim_blk_handle_req(vdpasim, vq)) {
>> +               while (vq_work) {
>> +                       vq_work = vdpasim_blk_handle_req(vdpasim, vq);
>> +
>
>Is it better to check and exit the loop early here?

Maybe, but I'm not sure.

In vdpa_sim_net we call vringh_complete_iotlb() and send notification 
also in the error path, so I thought was better to send notification 
also when vdpasim_blk_handle_req() return false, since we will update 
the used.idx.

However, I don't think it's a common path, so if you think it's better 
to exit the loop early, I can do it.

Thanks,
Stefano

Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Stefano Garzarella]

On Thu, Jun 23, 2022 at 11:50:22AM +0800, Jason Wang wrote:
>On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> Limit the number of requests (4 per queue as for vdpa_sim_net) handled
>> in a batch to prevent the worker from using the CPU for too long.
>>
>> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> index a83a5c76f620..ac86478845b6 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> @@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>>  static void vdpasim_blk_work(struct work_struct *work)
>>  {
>>         struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
>> +       bool reschedule = false;
>>         int i;
>>
>>         spin_lock(&vdpasim->lock);
>> @@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
>>
>>         for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
>>                 struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
>> +               bool vq_work = true;
>> +               int reqs = 0;
>>
>>                 if (!vq->ready)
>>                         continue;
>>
>> -               while (vdpasim_blk_handle_req(vdpasim, vq)) {
>> +               while (vq_work) {
>> +                       vq_work = vdpasim_blk_handle_req(vdpasim, vq);
>> +
>
>Is it better to check and exit the loop early here?

Maybe, but I'm not sure.

In vdpa_sim_net we call vringh_complete_iotlb() and send notification 
also in the error path, so I thought was better to send notification 
also when vdpasim_blk_handle_req() return false, since we will update 
the used.idx.

However, I don't think it's a common path, so if you think it's better 
to exit the loop early, I can do it.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Jason Wang]

On Thu, Jun 23, 2022 at 4:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Jun 23, 2022 at 11:50:22AM +0800, Jason Wang wrote:
> >On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> Limit the number of requests (4 per queue as for vdpa_sim_net) handled
> >> in a batch to prevent the worker from using the CPU for too long.
> >>
> >> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> ---
> >>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
> >>  1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> index a83a5c76f620..ac86478845b6 100644
> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> @@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
> >>  static void vdpasim_blk_work(struct work_struct *work)
> >>  {
> >>         struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> >> +       bool reschedule = false;
> >>         int i;
> >>
> >>         spin_lock(&vdpasim->lock);
> >> @@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
> >>
> >>         for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> >>                 struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> >> +               bool vq_work = true;
> >> +               int reqs = 0;
> >>
> >>                 if (!vq->ready)
> >>                         continue;
> >>
> >> -               while (vdpasim_blk_handle_req(vdpasim, vq)) {
> >> +               while (vq_work) {
> >> +                       vq_work = vdpasim_blk_handle_req(vdpasim, vq);
> >> +
> >
> >Is it better to check and exit the loop early here?
>
> Maybe, but I'm not sure.
>
> In vdpa_sim_net we call vringh_complete_iotlb() and send notification
> also in the error path,

Looks not?

                read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->in_iov, &ctrl,
                                             sizeof(ctrl));
                if (read != sizeof(ctrl))
                        break;

We break the loop.

Thanks

> so I thought was better to send notification
> also when vdpasim_blk_handle_req() return false, since we will update
> the used.idx.
>
> However, I don't think it's a common path, so if you think it's better
> to exit the loop early, I can do it.
>
> Thanks,
> Stefano
>

Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Jason Wang]

On Thu, Jun 23, 2022 at 4:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Jun 23, 2022 at 11:50:22AM +0800, Jason Wang wrote:
> >On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> Limit the number of requests (4 per queue as for vdpa_sim_net) handled
> >> in a batch to prevent the worker from using the CPU for too long.
> >>
> >> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> ---
> >>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
> >>  1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> index a83a5c76f620..ac86478845b6 100644
> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> @@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
> >>  static void vdpasim_blk_work(struct work_struct *work)
> >>  {
> >>         struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> >> +       bool reschedule = false;
> >>         int i;
> >>
> >>         spin_lock(&vdpasim->lock);
> >> @@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
> >>
> >>         for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> >>                 struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> >> +               bool vq_work = true;
> >> +               int reqs = 0;
> >>
> >>                 if (!vq->ready)
> >>                         continue;
> >>
> >> -               while (vdpasim_blk_handle_req(vdpasim, vq)) {
> >> +               while (vq_work) {
> >> +                       vq_work = vdpasim_blk_handle_req(vdpasim, vq);
> >> +
> >
> >Is it better to check and exit the loop early here?
>
> Maybe, but I'm not sure.
>
> In vdpa_sim_net we call vringh_complete_iotlb() and send notification
> also in the error path,

Looks not?

                read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->in_iov, &ctrl,
                                             sizeof(ctrl));
                if (read != sizeof(ctrl))
                        break;

We break the loop.

Thanks

> so I thought was better to send notification
> also when vdpasim_blk_handle_req() return false, since we will update
> the used.idx.
>
> However, I don't think it's a common path, so if you think it's better
> to exit the loop early, I can do it.
>
> Thanks,
> Stefano
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Stefano Garzarella]

On Tue, Jun 28, 2022 at 6:01 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jun 23, 2022 at 4:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Thu, Jun 23, 2022 at 11:50:22AM +0800, Jason Wang wrote:
> > >On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >>
> > >> Limit the number of requests (4 per queue as for vdpa_sim_net) handled
> > >> in a batch to prevent the worker from using the CPU for too long.
> > >>
> > >> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > >> ---
> > >>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
> > >>  1 file changed, 14 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > >> index a83a5c76f620..ac86478845b6 100644
> > >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > >> @@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
> > >>  static void vdpasim_blk_work(struct work_struct *work)
> > >>  {
> > >>         struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> > >> +       bool reschedule = false;
> > >>         int i;
> > >>
> > >>         spin_lock(&vdpasim->lock);
> > >> @@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
> > >>
> > >>         for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> > >>                 struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> > >> +               bool vq_work = true;
> > >> +               int reqs = 0;
> > >>
> > >>                 if (!vq->ready)
> > >>                         continue;
> > >>
> > >> -               while (vdpasim_blk_handle_req(vdpasim, vq)) {
> > >> +               while (vq_work) {
> > >> +                       vq_work = vdpasim_blk_handle_req(vdpasim, vq);
> > >> +
> > >
> > >Is it better to check and exit the loop early here?
> >
> > Maybe, but I'm not sure.
> >
> > In vdpa_sim_net we call vringh_complete_iotlb() and send notification
> > also in the error path,
>
> Looks not?
>
>                 read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->in_iov, &ctrl,
>                                              sizeof(ctrl));
>                 if (read != sizeof(ctrl))
>                         break;
>
> We break the loop.

I was looking at vdpasim_net_work(), but I was confused since it
handles 2 queues.

I'll break the loop as it was before.

Thanks,
Stefano

Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch

[Stefano Garzarella]

On Tue, Jun 28, 2022 at 6:01 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jun 23, 2022 at 4:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Thu, Jun 23, 2022 at 11:50:22AM +0800, Jason Wang wrote:
> > >On Wed, Jun 22, 2022 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >>
> > >> Limit the number of requests (4 per queue as for vdpa_sim_net) handled
> > >> in a batch to prevent the worker from using the CPU for too long.
> > >>
> > >> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > >> ---
> > >>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 15 ++++++++++++++-
> > >>  1 file changed, 14 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > >> index a83a5c76f620..ac86478845b6 100644
> > >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > >> @@ -197,6 +197,7 @@ static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
> > >>  static void vdpasim_blk_work(struct work_struct *work)
> > >>  {
> > >>         struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> > >> +       bool reschedule = false;
> > >>         int i;
> > >>
> > >>         spin_lock(&vdpasim->lock);
> > >> @@ -206,11 +207,15 @@ static void vdpasim_blk_work(struct work_struct *work)
> > >>
> > >>         for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> > >>                 struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> > >> +               bool vq_work = true;
> > >> +               int reqs = 0;
> > >>
> > >>                 if (!vq->ready)
> > >>                         continue;
> > >>
> > >> -               while (vdpasim_blk_handle_req(vdpasim, vq)) {
> > >> +               while (vq_work) {
> > >> +                       vq_work = vdpasim_blk_handle_req(vdpasim, vq);
> > >> +
> > >
> > >Is it better to check and exit the loop early here?
> >
> > Maybe, but I'm not sure.
> >
> > In vdpa_sim_net we call vringh_complete_iotlb() and send notification
> > also in the error path,
>
> Looks not?
>
>                 read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->in_iov, &ctrl,
>                                              sizeof(ctrl));
>                 if (read != sizeof(ctrl))
>                         break;
>
> We break the loop.

I was looking at vdpasim_net_work(), but I was confused since it
handles 2 queues.

I'll break the loop as it was before.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization