Message ID | 20240801124540.38774-1-xiangwencheng@dayudpu.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] vhost-user: Do not wait for reply for not sent VHOST_USER_SET_LOG_BASE | expand |
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
> 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
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
> 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?
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
> 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.
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.
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
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) {