[PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[BillXiang]

From: BillXiang <xiangwencheng@dayudpu.com>

Currently, we have added VHOST_USER_SET_LOG_BASE to 
vhost_user_per_device_request in commit 7c211eb078c4 
("vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests"), 
as a result, VHOST_USER_SET_LOG_BASE will be sent only once 
when 'vq_index == 0'.
In this patch we add the check of 'vq_index == 0' before 
vhost_user_read, such that we do not wait for reply for not
sent VHOST_USER_SET_LOG_BASE.

Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
---
 hw/virtio/vhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 00561daa06..fd12992d15 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -460,7 +460,7 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
         return ret;
     }
 
-    if (shmfd) {
+    if (shmfd && (dev->vq_index == 0)) {
         msg.hdr.size = 0;
         ret = vhost_user_read(dev, &msg);
         if (ret < 0) {
-- 
2.30.0

Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[Michael S. Tsirkin]

On Thu, Aug 01, 2024 at 08:45:40PM +0800, BillXiang wrote:
> From: BillXiang <xiangwencheng@dayudpu.com>
> 
> Currently, we have added VHOST_USER_SET_LOG_BASE to 
> vhost_user_per_device_request in commit 7c211eb078c4 
> ("vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests"), 
> as a result, VHOST_USER_SET_LOG_BASE will be sent only once 
> when 'vq_index == 0'.
> In this patch we add the check of 'vq_index == 0' before 
> vhost_user_read, such that we do not wait for reply for not
> sent VHOST_USER_SET_LOG_BASE.
> 
> Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 00561daa06..fd12992d15 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -460,7 +460,7 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
>          return ret;
>      }
>  
> -    if (shmfd) {
> +    if (shmfd && (dev->vq_index == 0)) {
>          msg.hdr.size = 0;
>          ret = vhost_user_read(dev, &msg);
>          if (ret < 0) {



How do things work now after 7c211eb078c4 then?

> -- 
> 2.30.0

Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[BillXiang]

> From: "Michael S. Tsirkin"<mst@redhat.com>
> Date:  Thu, Aug 1, 2024, 22:13
> Subject:  Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE
> To: "BillXiang"<xiangwencheng@dayudpu.com>
> Cc: <qemu-devel@nongnu.org>
> On Thu, Aug 01, 2024 at 08:45:40PM +0800, BillXiang wrote:
> > From: BillXiang <xiangwencheng@dayudpu.com>
> > 
> > Currently, we have added VHOST_USER_SET_LOG_BASE to 
> > vhost_user_per_device_request in commit 7c211eb078c4 
> > ("vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests"), 
> > as a result, VHOST_USER_SET_LOG_BASE will be sent only once 
> > when 'vq_index == 0'.
> > In this patch we add the check of 'vq_index == 0' before 
> > vhost_user_read, such that we do not wait for reply for not
> > sent VHOST_USER_SET_LOG_BASE.
> > 
> > Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>
> > ---
> >  hw/virtio/vhost-user.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 00561daa06..fd12992d15 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -460,7 +460,7 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
> >          return ret;
> >      }
> >  
> > -    if (shmfd) {
> > +    if (shmfd && (dev->vq_index == 0)) {
> >          msg.hdr.size = 0;
> >          ret = vhost_user_read(dev, &msg);
> >          if (ret < 0) {
> 
> 
> 
> How do things work now after 7c211eb078c4 then?

It does not really work after 7c211eb078c4 and it's my mistake. 
I forgot to submit the code to check vq_index in 7c211eb078c4.

> 
> > -- 
> > 2.30.0

Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[Prasad Pandit]

On Thu, 1 Aug 2024 at 20:32, BillXiang <xiangwencheng@dayudpu.com> wrote:
> > From: "Michael S. Tsirkin"<mst@redhat.com>
> > How do things work now after 7c211eb078c4 then?
>
> It does not really work after 7c211eb078c4 and it's my mistake.
> I forgot to submit the code to check vq_index in 7c211eb078c4.
>

* vhost_user_set_log_base() sends set log message only when (vq_index == 0),
    -> https://github.com/qemu/qemu/commit/c98ac64cfb53ccb862a80e818c3a19bdd386e61e
===
    +    /* Send only once with first queue pair */
    +    if (dev->vq_index != 0) {
    +        return 0;
    +    }
===

This should help to keep things working, no?
---
  - Prasad

Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[BillXiang]

> From: "Prasad Pandit"<ppandit@redhat.com>
> Date:  Tue, Aug 27, 2024, 19:06
> Subject:  Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE
> To: "BillXiang"<xiangwencheng@dayudpu.com>
> Cc: "Michael S. Tsirkin"<mst@redhat.com>, <qemu-devel@nongnu.org>
> On Thu, 1 Aug 2024 at 20:32, BillXiang <xiangwencheng@dayudpu.com> wrote:
> > > From: "Michael S. Tsirkin"<mst@redhat.com>
> > > How do things work now after 7c211eb078c4 then?
> >
> > It does not really work after 7c211eb078c4 and it's my mistake.
> > I forgot to submit the code to check vq_index in 7c211eb078c4.
> >
> 
> * vhost_user_set_log_base() sends set log message only when (vq_index == 0),
>     -> https://github.com/qemu/qemu/commit/c98ac64cfb53ccb862a80e818c3a19bdd386e61e
> ===
>     +    /* Send only once with first queue pair */
>     +    if (dev->vq_index != 0) {
>     +        return 0;
>     +    }
> ===
> 
> This should help to keep things working, no?
> ---
>   - Prasad

Yes, that works. But I think it's better to be consistent to use vhost_user_per_device_request for those per-device messages, right?

Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[Prasad Pandit]

On Tue, 27 Aug 2024 at 16:50, BillXiang <xiangwencheng@dayudpu.com> wrote:
> it's better to be consistent to use vhost_user_per_device_request for those per-device messages, right?

* ...consistent to use? Could you please elaborate a little?

Thank you.
---
  - Prasad

Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[BillXiang]

> From: "Prasad Pandit"<ppandit@redhat.com>
> Date:  Tue, Aug 27, 2024, 20:37
> Subject:  Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE
> To: "BillXiang"<xiangwencheng@dayudpu.com>
> Cc: "Michael S. Tsirkin"<mst@redhat.com>, <qemu-devel@nongnu.org>
> On Tue, 27 Aug 2024 at 16:50, BillXiang <xiangwencheng@dayudpu.com> wrote:
> > it's better to be consistent to use vhost_user_per_device_request for those per-device messages, right?
> 
> * ...consistent to use? Could you please elaborate a little?
> 
> Thank you.
> ---
>   - Prasad

That was elaborated in commit b931bfbf0429 (" vhost-user: add multiple queue support "). 
We have added vhost_user_one_time_request() to send those per-device messages only once 
for multi-queue device. Which was then changed to vhost_user_per_device_request() in 
commit 0dcb4172f2ce ("vhost-user: Change one_time to per_device request").
And VHOST_USER_SET_LOG_BASE should be one of those per-device messages that only
be sent once for multi-queue device.

Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[Michael S. Tsirkin]

On Tue, Aug 27, 2024 at 09:00:35PM +0800, BillXiang wrote:
> 
> > From: "Prasad Pandit"<ppandit@redhat.com>
> > Date:  Tue, Aug 27, 2024, 20:37
> > Subject:  Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE
> > To: "BillXiang"<xiangwencheng@dayudpu.com>
> > Cc: "Michael S. Tsirkin"<mst@redhat.com>, <qemu-devel@nongnu.org>
> > On Tue, 27 Aug 2024 at 16:50, BillXiang <xiangwencheng@dayudpu.com> wrote:
> > > it's better to be consistent to use vhost_user_per_device_request for those per-device messages, right?
> > 
> > * ...consistent to use? Could you please elaborate a little?
> > 
> > Thank you.
> > ---
> >   - Prasad
> 
> That was elaborated in commit b931bfbf0429 (" vhost-user: add multiple queue support "). 
> We have added vhost_user_one_time_request() to send those per-device messages only once 
> for multi-queue device. Which was then changed to vhost_user_per_device_request() in 
> commit 0dcb4172f2ce ("vhost-user: Change one_time to per_device request").
> And VHOST_USER_SET_LOG_BASE should be one of those per-device messages that only
> be sent once for multi-queue device.

Bill,
it's important to make it clear, in the commit message, what is the
current behaviour and what is the effect of the patch.
For example: currently qemu hangs waiting for ...., to fix,
.... so we never wait for .... .
At the moment, I'm not really sure if this is a bugfix, or
a cleanup, or what.


-- 
MST

Re: [PATCH v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE

[Michael S. Tsirkin]

On Thu, Aug 01, 2024 at 08:45:40PM +0800, BillXiang wrote:
> From: BillXiang <xiangwencheng@dayudpu.com>
> 
> Currently, we have added VHOST_USER_SET_LOG_BASE to 
> vhost_user_per_device_request in commit 7c211eb078c4 
> ("vhost-user: Skip unnecessary duplicated VHOST_USER_SET_LOG_BASE requests"), 
> as a result, VHOST_USER_SET_LOG_BASE will be sent only once 
> when 'vq_index == 0'.
> In this patch we add the check of 'vq_index == 0' before 
> vhost_user_read, such that we do not wait for reply for not
> sent VHOST_USER_SET_LOG_BASE.
> 
> Signed-off-by: BillXiang <xiangwencheng@dayudpu.com>

If you still want this in, pls rewrite the commit log to make
it clear wat is going on: e.g. "cleanup X which does not do
Y with Z, which does" and repost.


> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 00561daa06..fd12992d15 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -460,7 +460,7 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
>          return ret;
>      }
>  
> -    if (shmfd) {
> +    if (shmfd && (dev->vq_index == 0)) {
>          msg.hdr.size = 0;
>          ret = vhost_user_read(dev, &msg);
>          if (ret < 0) {
> -- 
> 2.30.0