* [PATCH 0/3] vdpa_sim_blk: several fixes for the vDPA block simulator
@ 2022-06-21 16:08 ` Stefano Garzarella
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-21 16:08 UTC (permalink / raw)
To: virtualization
Cc: Jason Wang, Eugenio Pérez, Michael S. Tsirkin, linux-kernel,
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 0/3] vdpa_sim_blk: several fixes for the vDPA block simulator
@ 2022-06-21 16:08 ` Stefano Garzarella
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-21 16:08 UTC (permalink / raw)
To: virtualization; +Cc: Eugenio Pérez, linux-kernel, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors
2022-06-21 16:08 ` Stefano Garzarella
@ 2022-06-21 16:08 ` Stefano Garzarella
-1 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-21 16:08 UTC (permalink / raw)
To: virtualization
Cc: Jason Wang, Eugenio Pérez, Michael S. Tsirkin, linux-kernel,
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors
@ 2022-06-21 16:08 ` Stefano Garzarella
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-21 16:08 UTC (permalink / raw)
To: virtualization; +Cc: Eugenio Pérez, linux-kernel, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
2022-06-21 16:08 ` Stefano Garzarella
@ 2022-06-21 16:08 ` Stefano Garzarella
-1 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-21 16:08 UTC (permalink / raw)
To: virtualization
Cc: Jason Wang, Eugenio Pérez, Michael S. Tsirkin, linux-kernel,
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
@ 2022-06-21 16:08 ` Stefano Garzarella
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-21 16:08 UTC (permalink / raw)
To: virtualization; +Cc: Eugenio Pérez, linux-kernel, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path
2022-06-21 16:08 ` Stefano Garzarella
@ 2022-06-21 16:12 ` Stefano Garzarella
-1 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-21 16:12 UTC (permalink / raw)
To: virtualization
Cc: Michael S. Tsirkin, linux-kernel, Jason Wang, Eugenio Pérez,
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path
@ 2022-06-21 16:12 ` Stefano Garzarella
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-21 16:12 UTC (permalink / raw)
To: virtualization; +Cc: Eugenio Pérez, linux-kernel, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors
2022-06-21 16:08 ` Stefano Garzarella
@ 2022-06-23 3:40 ` Jason Wang
-1 siblings, 0 replies; 20+ messages in thread
From: Jason Wang @ 2022-06-23 3:40 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Eugenio Pérez, Michael S. Tsirkin, linux-kernel, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors
@ 2022-06-23 3:40 ` Jason Wang
0 siblings, 0 replies; 20+ messages in thread
From: Jason Wang @ 2022-06-23 3:40 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, Eugenio Pérez, Michael S. Tsirkin, linux-kernel
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
2022-06-21 16:08 ` Stefano Garzarella
@ 2022-06-23 3:50 ` Jason Wang
-1 siblings, 0 replies; 20+ messages in thread
From: Jason Wang @ 2022-06-23 3:50 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Eugenio Pérez, Michael S. Tsirkin, linux-kernel, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
@ 2022-06-23 3:50 ` Jason Wang
0 siblings, 0 replies; 20+ messages in thread
From: Jason Wang @ 2022-06-23 3:50 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, Eugenio Pérez, Michael S. Tsirkin, linux-kernel
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path
2022-06-21 16:12 ` Stefano Garzarella
@ 2022-06-23 3:52 ` Jason Wang
-1 siblings, 0 replies; 20+ messages in thread
From: Jason Wang @ 2022-06-23 3:52 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Eugenio Pérez, Michael S. Tsirkin, linux-kernel, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path
@ 2022-06-23 3:52 ` Jason Wang
0 siblings, 0 replies; 20+ messages in thread
From: Jason Wang @ 2022-06-23 3:52 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, Michael S. Tsirkin, linux-kernel, Eugenio Pérez
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
2022-06-23 3:50 ` Jason Wang
@ 2022-06-23 8:58 ` Stefano Garzarella
-1 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-23 8:58 UTC (permalink / raw)
To: Jason Wang
Cc: virtualization, Eugenio Pérez, Michael S. Tsirkin, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
@ 2022-06-23 8:58 ` Stefano Garzarella
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-23 8:58 UTC (permalink / raw)
To: Jason Wang
Cc: Eugenio Pérez, Michael S. Tsirkin, linux-kernel, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
2022-06-23 8:58 ` Stefano Garzarella
@ 2022-06-28 4:01 ` Jason Wang
-1 siblings, 0 replies; 20+ messages in thread
From: Jason Wang @ 2022-06-28 4:01 UTC (permalink / raw)
To: Stefano Garzarella
Cc: virtualization, Eugenio Pérez, Michael S. Tsirkin, linux-kernel
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
@ 2022-06-28 4:01 ` Jason Wang
0 siblings, 0 replies; 20+ messages in thread
From: Jason Wang @ 2022-06-28 4:01 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Eugenio Pérez, Michael S. Tsirkin, linux-kernel, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
2022-06-28 4:01 ` Jason Wang
@ 2022-06-28 7:55 ` Stefano Garzarella
-1 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-28 7:55 UTC (permalink / raw)
To: Jason Wang
Cc: virtualization, Eugenio Pérez, Michael S. Tsirkin, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch
@ 2022-06-28 7:55 ` Stefano Garzarella
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Garzarella @ 2022-06-28 7:55 UTC (permalink / raw)
To: Jason Wang
Cc: Eugenio Pérez, Michael S. Tsirkin, linux-kernel, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-06-28 7:55 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-21 16:08 [PATCH 0/3] vdpa_sim_blk: several fixes for the vDPA block simulator Stefano Garzarella
2022-06-21 16:08 ` Stefano Garzarella
2022-06-21 16:08 ` [PATCH 1/3] vdpa_sim_blk: use dev_dbg() to print errors Stefano Garzarella
2022-06-21 16:08 ` Stefano Garzarella
2022-06-23 3:40 ` Jason Wang
2022-06-23 3:40 ` Jason Wang
2022-06-21 16:08 ` [PATCH 2/3] vdpa_sim_blk: limit the number of request handled per batch Stefano Garzarella
2022-06-21 16:08 ` Stefano Garzarella
2022-06-23 3:50 ` Jason Wang
2022-06-23 3:50 ` Jason Wang
2022-06-23 8:58 ` Stefano Garzarella
2022-06-23 8:58 ` Stefano Garzarella
2022-06-28 4:01 ` Jason Wang
2022-06-28 4:01 ` Jason Wang
2022-06-28 7:55 ` Stefano Garzarella
2022-06-28 7:55 ` Stefano Garzarella
2022-06-21 16:12 ` [PATCH 3/3] vdpa_sim_blk: call vringh_complete_iotlb() also in the error path Stefano Garzarella
2022-06-21 16:12 ` Stefano Garzarella
2022-06-23 3:52 ` Jason Wang
2022-06-23 3:52 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.