[v2,1/7] vhost-user: strip superfluous whitespace

Commit Message

Laszlo Ersek Aug. 30, 2023, 1:40 p.m. UTC

Cc: "Michael S. Tsirkin" <[e-mail]> (supporter:vhost)
Cc: Eugenio Perez Martin <[e-mail]>
Cc: German Maglione <[e-mail]>
Cc: Liu Jiang <[e-mail]>
Cc: Sergio Lopez Pascual <[e-mail]>
Cc: Stefano Garzarella <[e-mail]>
Signed-off-by: Laszlo Ersek <[e-mail]>
Reviewed-by: Stefano Garzarella <[e-mail]>
---

Notes:
    v2:
    
    - pick up Stefano's R-b

 hw/virtio/vhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Laszlo Ersek Aug. 31, 2023, 7:13 a.m. UTC | #1

On 8/30/23 15:40, Laszlo Ersek wrote:
> Cc: "Michael S. Tsirkin" <[e-mail]> (supporter:vhost)
> Cc: Eugenio Perez Martin <[e-mail]>
> Cc: German Maglione <[e-mail]>
> Cc: Liu Jiang <[e-mail]>
> Cc: Sergio Lopez Pascual <[e-mail]>
> Cc: Stefano Garzarella <[e-mail]>
> Signed-off-by: Laszlo Ersek <[e-mail]>
> Reviewed-by: Stefano Garzarella <[e-mail]>
> ---
> 
> Notes:
>     v2:
>     
>     - pick up Stefano's R-b
> 
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This has been

Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>

under the (identical) v1 posting:

http://mid.mail-archive.com/[e-mail]

Thanks, Phil! (and sorry that I posted v2 too quickly -- I forgot that
sometimes reviewers split a review over multiple days.)

Laszlo

> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 8dcf049d422b..b4b677c1ce66 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -398,7 +398,7 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
>       * operations such as configuring device memory mappings or issuing device
>       * resets, which affect the whole device instead of individual VQs,
>       * vhost-user messages should only be sent once.
> -     * 
> +     *
>       * Devices with multiple vhost_devs are given an associated dev->vq_index
>       * so per_device requests are only sent if vq_index is 0.
>       */
>

Albert Esteve Sept. 6, 2023, 7:12 a.m. UTC | #2

On Thu, Aug 31, 2023 at 9:14 AM Laszlo Ersek <[e-mail]> wrote:

> On 8/30/23 15:40, Laszlo Ersek wrote:
> > Cc: "Michael S. Tsirkin" <[e-mail]> (supporter:vhost)
> > Cc: Eugenio Perez Martin <[e-mail]>
> > Cc: German Maglione <[e-mail]>
> > Cc: Liu Jiang <[e-mail]>
> > Cc: Sergio Lopez Pascual <[e-mail]>
> > Cc: Stefano Garzarella <[e-mail]>
> > Signed-off-by: Laszlo Ersek <[e-mail]>
> > Reviewed-by: Stefano Garzarella <[e-mail]>
> > ---
> >
> > Notes:
> >     v2:
> >
> >     - pick up Stefano's R-b
> >
> >  hw/virtio/vhost-user.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> This has been
>
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>
> under the (identical) v1 posting:
>
> http://mid.mail-archive.com/[e-mail]
>
> Thanks, Phil! (and sorry that I posted v2 too quickly -- I forgot that
> sometimes reviewers split a review over multiple days.)
>
> Laszlo
>
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 8dcf049d422b..b4b677c1ce66 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -398,7 +398,7 @@ static int vhost_user_write(struct vhost_dev *dev,
> VhostUserMsg *msg,
> >       * operations such as configuring device memory mappings or issuing
> device
> >       * resets, which affect the whole device instead of individual VQs,
> >       * vhost-user messages should only be sent once.
> > -     *
> > +     *
> >       * Devices with multiple vhost_devs are given an associated
> dev->vq_index
> >       * so per_device requests are only sent if vq_index is 0.
> >       */
> >
>
>
Thanks for the series!
I had a timeout problem with a virtio device I am developing, and I was not
sure yet what was going on.
Your description of the problem seemed to fit mine, in my case the driver
sent a command through the data plane
right after the feature negotiation that reached the backend too soon.
Adding delays alleviated the issue, so it
already hinted me to a race condition.

I tested using this patch series and according to my experiments, it really
lowers the chances to get the deadlock.
Sadly, I do still get the issue sometimes, though (not frequently)...
However, I think probably the solution comes not
from the Qemu side, but from the rust-vmm vhost-user-backend crate. I am
looking for that solution on my side.

But that does not invalidate this patch, which I think is a necessary
improvement, and in my tests it really
helps a lot with the described issue. Therefore:

Tested-by: Albert Esteve <[e-mail]>

BR,
Albert

Laszlo Ersek Sept. 6, 2023, 9:09 a.m. UTC | #3

On 9/6/23 09:12, Albert Esteve wrote:
> 
> 
> On Thu, Aug 31, 2023 at 9:14 AM Laszlo Ersek <[e-mail]
> <mailto:[e-mail]>> wrote:
> 
>     On 8/30/23 15:40, Laszlo Ersek wrote:
>     > Cc: "Michael S. Tsirkin" <[e-mail] <mailto:[e-mail]>>
>     (supporter:vhost)
>     > Cc: Eugenio Perez Martin <[e-mail]
>     <mailto:[e-mail]>>
>     > Cc: German Maglione <[e-mail]
>     <mailto:[e-mail]>>
>     > Cc: Liu Jiang <[e-mail]
>     <mailto:[e-mail]>>
>     > Cc: Sergio Lopez Pascual <[e-mail] <mailto:[e-mail]>>
>     > Cc: Stefano Garzarella <[e-mail]
>     <mailto:[e-mail]>>
>     > Signed-off-by: Laszlo Ersek <[e-mail]
>     <mailto:[e-mail]>>
>     > Reviewed-by: Stefano Garzarella <[e-mail]
>     <mailto:[e-mail]>>
>     > ---
>     >
>     > Notes:
>     >     v2:
>     >     
>     >     - pick up Stefano's R-b
>     >
>     >  hw/virtio/vhost-user.c | 2 +-
>     >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>     This has been
> 
>     Reviewed-by: Philippe Mathieu-Daudé <[e-mail]
>     <mailto:[e-mail]>>
> 
>     under the (identical) v1 posting:
> 
>     http://mid.mail-archive.com/[e-mail] <http://mid.mail-archive.com/[e-mail]>
> 
>     Thanks, Phil! (and sorry that I posted v2 too quickly -- I forgot that
>     sometimes reviewers split a review over multiple days.)
> 
>     Laszlo
> 
>     >
>     > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>     > index 8dcf049d422b..b4b677c1ce66 100644
>     > --- a/hw/virtio/vhost-user.c
>     > +++ b/hw/virtio/vhost-user.c
>     > @@ -398,7 +398,7 @@ static int vhost_user_write(struct vhost_dev
>     *dev, VhostUserMsg *msg,
>     >       * operations such as configuring device memory mappings or
>     issuing device
>     >       * resets, which affect the whole device instead of
>     individual VQs,
>     >       * vhost-user messages should only be sent once.
>     > -     *
>     > +     *
>     >       * Devices with multiple vhost_devs are given an associated
>     dev->vq_index
>     >       * so per_device requests are only sent if vq_index is 0.
>     >       */
>     >
> 
> 
> Thanks for the series!
> I had a timeout problem with a virtio device I am developing, and I was
> not sure yet what was going on.
> Your description of the problem seemed to fit mine, in my case the
> driver sent a command through the data plane
> right after the feature negotiation that reached the backend too soon.
> Adding delays alleviated the issue, so it
> already hinted me to a race condition.
> 
> I tested using this patch series and according to my experiments, it
> really lowers the chances to get the deadlock.
> Sadly, I do still get the issue sometimes, though (not frequently)...
> However, I think probably the solution comes not
> from the Qemu side, but from the rust-vmm vhost-user-backend crate. I am
> looking for that solution on my side.
> 
> But that does not invalidate this patch, which I think is a necessary
> improvement, and in my tests it really
> helps a lot with the described issue. Therefore:
> 
> Tested-by: Albert Esteve <[e-mail] <mailto:[e-mail]>>

Thank you for the testing and the feedback!

My problem with relegating the fix to rust-vmm/vhost -- i.e. with
calling the hang a bug inside rust-vmm/vhost -- is that rust-vmm/vhost
is *unfixable* as long as the vhost-user specification says what it
says. As soon as the guest is permitted to issue a data plane operation,
without forcing it to await completion of an earlier control plane
operation, the cat is out of the bag. Those operations travel through
independent channels, and the vhost-user spec currently requires the
backend to listen to both channels at the same time. There's no way to
restore the original order after both operations have been submitted
effectively in parallel from the guest side.

The guest cannot limit itself, the virtio operations it needs to do (on
the control plane) are described in the virtio spec, in "driver
requirements" sections, and most of those steps are inherently
treated/specified as synchronous. The guest already thinks those steps
are synchronous.

So it really is a spec problem. I see the big problem in the switch from
vhost-net to the generic vhost-user protocol. My understanding from the
virtio-networking series in the RH Developer Blog is that vhost-net was
entirely ioctl() based, and consequently totally synchronous. That was
lost when the protocol was rebased to unix domain sockets, without
upholding the explicit request-response pattern in all operations.

I'm worried we can't get this unstuck until Michael answers Stefan's
question, concerning the spec.

Laszlo

Laszlo Ersek Oct. 2, 2023, 7:48 p.m. UTC | #4

On 9/6/23 09:12, Albert Esteve wrote:
> 
> 
> On Thu, Aug 31, 2023 at 9:14 AM Laszlo Ersek <[e-mail]
> <mailto:[e-mail]>> wrote:
> 
>     On 8/30/23 15:40, Laszlo Ersek wrote:
>     > Cc: "Michael S. Tsirkin" <[e-mail] <mailto:[e-mail]>>
>     (supporter:vhost)
>     > Cc: Eugenio Perez Martin <[e-mail]
>     <mailto:[e-mail]>>
>     > Cc: German Maglione <[e-mail]
>     <mailto:[e-mail]>>
>     > Cc: Liu Jiang <[e-mail]
>     <mailto:[e-mail]>>
>     > Cc: Sergio Lopez Pascual <[e-mail] <mailto:[e-mail]>>
>     > Cc: Stefano Garzarella <[e-mail]
>     <mailto:[e-mail]>>
>     > Signed-off-by: Laszlo Ersek <[e-mail]
>     <mailto:[e-mail]>>
>     > Reviewed-by: Stefano Garzarella <[e-mail]
>     <mailto:[e-mail]>>
>     > ---
>     >
>     > Notes:
>     >     v2:
>     >     
>     >     - pick up Stefano's R-b
>     >
>     >  hw/virtio/vhost-user.c | 2 +-
>     >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>     This has been
> 
>     Reviewed-by: Philippe Mathieu-Daudé <[e-mail]
>     <mailto:[e-mail]>>
> 
>     under the (identical) v1 posting:
> 
>     http://mid.mail-archive.com/[e-mail] <http://mid.mail-archive.com/[e-mail]>
> 
>     Thanks, Phil! (and sorry that I posted v2 too quickly -- I forgot that
>     sometimes reviewers split a review over multiple days.)
> 
>     Laszlo
> 
>     >
>     > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>     > index 8dcf049d422b..b4b677c1ce66 100644
>     > --- a/hw/virtio/vhost-user.c
>     > +++ b/hw/virtio/vhost-user.c
>     > @@ -398,7 +398,7 @@ static int vhost_user_write(struct vhost_dev
>     *dev, VhostUserMsg *msg,
>     >       * operations such as configuring device memory mappings or
>     issuing device
>     >       * resets, which affect the whole device instead of
>     individual VQs,
>     >       * vhost-user messages should only be sent once.
>     > -     *
>     > +     *
>     >       * Devices with multiple vhost_devs are given an associated
>     dev->vq_index
>     >       * so per_device requests are only sent if vq_index is 0.
>     >       */
>     >
> 
> 
> Thanks for the series!
> I had a timeout problem with a virtio device I am developing, and I was
> not sure yet what was going on.
> Your description of the problem seemed to fit mine, in my case the
> driver sent a command through the data plane
> right after the feature negotiation that reached the backend too soon.
> Adding delays alleviated the issue, so it
> already hinted me to a race condition.
> 
> I tested using this patch series and according to my experiments, it
> really lowers the chances to get the deadlock.
> Sadly, I do still get the issue sometimes, though (not frequently)...
> However, I think probably the solution comes not
> from the Qemu side, but from the rust-vmm vhost-user-backend crate. I am
> looking for that solution on my side.
> 
> But that does not invalidate this patch, which I think is a necessary
> improvement, and in my tests it really
> helps a lot with the described issue. Therefore:
> 
> Tested-by: Albert Esteve <[e-mail] <mailto:[e-mail]>>

Thanks again -- I'm picking this up for the whole series.

Laszlo

> 
> BR,
> Albert

Patch

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8dcf049d422b..b4b677c1ce66 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -398,7 +398,7 @@  static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
      * operations such as configuring device memory mappings or issuing device
      * resets, which affect the whole device instead of individual VQs,
      * vhost-user messages should only be sent once.
-     * 
+     *
      * Devices with multiple vhost_devs are given an associated dev->vq_index
      * so per_device requests are only sent if vq_index is 0.
      */