qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
@ 2021-06-25  8:52 Denis Plotnikov
  2021-07-02  9:41 ` Denis Plotnikov
  2021-07-07 10:10 ` Michael S. Tsirkin
  0 siblings, 2 replies; 9+ messages in thread
From: Denis Plotnikov @ 2021-06-25  8:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: yc-core, mst

On vhost-user-blk migration, qemu normally sends a number of commands
to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
data logging.
The issue is that qemu doesn't wait for reply from the vhost daemon
for these commands which may result in races between qemu expectation
of logging starting and actual login starting in vhost daemon.
To resolve this issue, this patch makes qemu wait for the commands result
explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
command to make the features setting functions work similary.

Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
---
 hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index ee57abe04526..e47b82adab00 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
         .hdr.size = sizeof(msg.payload.addr),
     };
 
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
+    if (reply_supported) {
+        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
+    }
+
     if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
         return -1;
     }
 
+    if (reply_supported) {
+        return process_message_reply(dev, &msg);
+    }
+
     return 0;
 }
 
@@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
         .hdr.size = sizeof(msg.payload.u64),
     };
 
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
+    if (reply_supported) {
+        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
+    }
+
     if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
         return -1;
     }
 
+    if (reply_supported) {
+        return process_message_reply(dev, &msg);
+    }
+
     return 0;
 }
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
  2021-06-25  8:52 [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies Denis Plotnikov
@ 2021-07-02  9:41 ` Denis Plotnikov
  2021-07-07  9:55   ` [PING] " Denis Plotnikov
  2021-07-07 10:10 ` Michael S. Tsirkin
  1 sibling, 1 reply; 9+ messages in thread
From: Denis Plotnikov @ 2021-07-02  9:41 UTC (permalink / raw)
  To: qemu-devel; +Cc: yc-core, mst

[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]

ping ping!

On 25.06.2021 11:52, Denis Plotnikov wrote:
> On vhost-user-blk migration, qemu normally sends a number of commands
> to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
> Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
> VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
> data logging.
> The issue is that qemu doesn't wait for reply from the vhost daemon
> for these commands which may result in races between qemu expectation
> of logging starting and actual login starting in vhost daemon.
> To resolve this issue, this patch makes qemu wait for the commands result
> explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
> Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
> command to make the features setting functions work similary.
>
> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> ---
>   hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index ee57abe04526..e47b82adab00 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>           .hdr.size = sizeof(msg.payload.addr),
>       };
>   
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +    if (reply_supported) {
> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }
> +
>       if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>           return -1;
>       }
>   
> +    if (reply_supported) {
> +        return process_message_reply(dev, &msg);
> +    }
> +
>       return 0;
>   }
>   
> @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
>           .hdr.size = sizeof(msg.payload.u64),
>       };
>   
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +    if (reply_supported) {
> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }
> +
>       if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>           return -1;
>       }
>   
> +    if (reply_supported) {
> +        return process_message_reply(dev, &msg);
> +    }
> +
>       return 0;
>   }
>   

[-- Attachment #2: Type: text/html, Size: 2822 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PING] [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
  2021-07-02  9:41 ` Denis Plotnikov
@ 2021-07-07  9:55   ` Denis Plotnikov
  0 siblings, 0 replies; 9+ messages in thread
From: Denis Plotnikov @ 2021-07-07  9:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: yc-core, mst

[-- Attachment #1: Type: text/plain, Size: 2555 bytes --]


On 02.07.2021 12:41, Denis Plotnikov wrote:
>
> ping ping!
>
> On 25.06.2021 11:52, Denis Plotnikov wrote:
>> On vhost-user-blk migration, qemu normally sends a number of commands
>> to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
>> Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
>> VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
>> data logging.
>> The issue is that qemu doesn't wait for reply from the vhost daemon
>> for these commands which may result in races between qemu expectation
>> of logging starting and actual login starting in vhost daemon.
>> To resolve this issue, this patch makes qemu wait for the commands result
>> explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
>> Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
>> command to make the features setting functions work similary.
>>
>> Signed-off-by: Denis Plotnikov<den-plotnikov@yandex-team.ru>
>> ---
>>   hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index ee57abe04526..e47b82adab00 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>>           .hdr.size = sizeof(msg.payload.addr),
>>       };
>>   
>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>> +    if (reply_supported) {
>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>> +    }
>> +
>>       if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>>           return -1;
>>       }
>>   
>> +    if (reply_supported) {
>> +        return process_message_reply(dev, &msg);
>> +    }
>> +
>>       return 0;
>>   }
>>   
>> @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
>>           .hdr.size = sizeof(msg.payload.u64),
>>       };
>>   
>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>> +    if (reply_supported) {
>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>> +    }
>> +
>>       if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>>           return -1;
>>       }
>>   
>> +    if (reply_supported) {
>> +        return process_message_reply(dev, &msg);
>> +    }
>> +
>>       return 0;
>>   }
>>   

[-- Attachment #2: Type: text/html, Size: 3172 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
  2021-06-25  8:52 [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies Denis Plotnikov
  2021-07-02  9:41 ` Denis Plotnikov
@ 2021-07-07 10:10 ` Michael S. Tsirkin
  2021-07-07 12:19   ` Denis Plotnikov
  1 sibling, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2021-07-07 10:10 UTC (permalink / raw)
  To: Denis Plotnikov; +Cc: qemu-devel, yc-core

On Fri, Jun 25, 2021 at 11:52:10AM +0300, Denis Plotnikov wrote:
> On vhost-user-blk migration, qemu normally sends a number of commands
> to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
> Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
> VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
> data logging.
> The issue is that qemu doesn't wait for reply from the vhost daemon
> for these commands which may result in races between qemu expectation
> of logging starting and actual login starting in vhost daemon.

Could you be more explicit please? What kind of race have you
observed? Getting a reply slows down the setup considerably and
should not be done lightly.

Thanks!

> To resolve this issue, this patch makes qemu wait for the commands result
> explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
> Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
> command to make the features setting functions work similary.
> 
> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> ---
>  hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index ee57abe04526..e47b82adab00 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>          .hdr.size = sizeof(msg.payload.addr),
>      };
>  
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +    if (reply_supported) {
> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }
> +
>      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>          return -1;
>      }
>  
> +    if (reply_supported) {
> +        return process_message_reply(dev, &msg);
> +    }
> +
>      return 0;
>  }
>  
> @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
>          .hdr.size = sizeof(msg.payload.u64),
>      };
>  
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +    if (reply_supported) {
> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }
> +
>      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>          return -1;
>      }
>  
> +    if (reply_supported) {
> +        return process_message_reply(dev, &msg);
> +    }
> +
>      return 0;
>  }
>  
> -- 
> 2.25.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
  2021-07-07 10:10 ` Michael S. Tsirkin
@ 2021-07-07 12:19   ` Denis Plotnikov
  2021-07-07 14:39     ` Michael S. Tsirkin
  0 siblings, 1 reply; 9+ messages in thread
From: Denis Plotnikov @ 2021-07-07 12:19 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, yc-core


On 07.07.2021 13:10, Michael S. Tsirkin wrote:
> On Fri, Jun 25, 2021 at 11:52:10AM +0300, Denis Plotnikov wrote:
>> On vhost-user-blk migration, qemu normally sends a number of commands
>> to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
>> Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
>> VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
>> data logging.
>> The issue is that qemu doesn't wait for reply from the vhost daemon
>> for these commands which may result in races between qemu expectation
>> of logging starting and actual login starting in vhost daemon.
> Could you be more explicit please? What kind of race have you
> observed? Getting a reply slows down the setup considerably and
> should not be done lightly.

I'm talking about the vhost-user-blk case. On migration setup, we enable 
logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive 
to a vhost-user-blk daemon immediately and the daemon needs some time 
turn the logging on internally. If qemu doesn't wait for reply, after 
sending the command qemu may start migrate memory pages. At this time 
the logging may not be actually turned on in the daemon  but some guest 
pages, which the daemon is about to write to, may be already transferred 
without logging to a destination. Since the logging wasn't turned on, 
those pages won't be transferred again as dirty. So we may end up with 
corrupted data on the destination.

Have I managed to explain the case clearly?

Thanks!

Denis

>
> Thanks!
>
>> To resolve this issue, this patch makes qemu wait for the commands result
>> explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
>> Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
>> command to make the features setting functions work similary.
>>
>> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
>> ---
>>   hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index ee57abe04526..e47b82adab00 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>>           .hdr.size = sizeof(msg.payload.addr),
>>       };
>>   
>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>> +    if (reply_supported) {
>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>> +    }
>> +
>>       if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>>           return -1;
>>       }
>>   
>> +    if (reply_supported) {
>> +        return process_message_reply(dev, &msg);
>> +    }
>> +
>>       return 0;
>>   }
>>   
>> @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
>>           .hdr.size = sizeof(msg.payload.u64),
>>       };
>>   
>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>> +    if (reply_supported) {
>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>> +    }
>> +
>>       if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>>           return -1;
>>       }
>>   
>> +    if (reply_supported) {
>> +        return process_message_reply(dev, &msg);
>> +    }
>> +
>>       return 0;
>>   }
>>   
>> -- 
>> 2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
  2021-07-07 12:19   ` Denis Plotnikov
@ 2021-07-07 14:39     ` Michael S. Tsirkin
  2021-07-07 14:58       ` Denis Plotnikov
  0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2021-07-07 14:39 UTC (permalink / raw)
  To: Denis Plotnikov; +Cc: qemu-devel, yc-core

On Wed, Jul 07, 2021 at 03:19:20PM +0300, Denis Plotnikov wrote:
> 
> On 07.07.2021 13:10, Michael S. Tsirkin wrote:
> > On Fri, Jun 25, 2021 at 11:52:10AM +0300, Denis Plotnikov wrote:
> > > On vhost-user-blk migration, qemu normally sends a number of commands
> > > to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
> > > Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
> > > VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
> > > data logging.
> > > The issue is that qemu doesn't wait for reply from the vhost daemon
> > > for these commands which may result in races between qemu expectation
> > > of logging starting and actual login starting in vhost daemon.
> > Could you be more explicit please? What kind of race have you
> > observed? Getting a reply slows down the setup considerably and
> > should not be done lightly.
> 
> I'm talking about the vhost-user-blk case. On migration setup, we enable
> logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
> vhost-user-blk daemon immediately and the daemon needs some time turn the
> logging on internally. If qemu doesn't wait for reply, after sending the
> command qemu may start migrate memory pages. At this time the logging may
> not be actually turned on in the daemon  but some guest pages, which the
> daemon is about to write to, may be already transferred without logging to a
> destination. Since the logging wasn't turned on, those pages won't be
> transferred again as dirty. So we may end up with corrupted data on the
> destination.
> 
> Have I managed to explain the case clearly?
> 
> Thanks!
> 
> Denis

OK so this is just about enabling logging. It would be cleaner to
defer migrating memory until response ... if that is too hard,
at least document why we are doing this please.
And, let's wait for an ack just in that case then - why not?

And what about VHOST_USER_SET_PROTOCOL_FEATURES?


> > 
> > Thanks!
> > 
> > > To resolve this issue, this patch makes qemu wait for the commands result
> > > explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
> > > Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
> > > command to make the features setting functions work similary.
> > > 
> > > Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> > > ---
> > >   hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > index ee57abe04526..e47b82adab00 100644
> > > --- a/hw/virtio/vhost-user.c
> > > +++ b/hw/virtio/vhost-user.c
> > > @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
> > >           .hdr.size = sizeof(msg.payload.addr),
> > >       };
> > > +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> > > +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> > > +    if (reply_supported) {
> > > +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > +    }
> > > +
> > >       if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > >           return -1;
> > >       }
> > > +    if (reply_supported) {
> > > +        return process_message_reply(dev, &msg);
> > > +    }
> > > +
> > >       return 0;
> > >   }
> > > @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
> > >           .hdr.size = sizeof(msg.payload.u64),
> > >       };
> > > +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> > > +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> > > +    if (reply_supported) {
> > > +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > +    }
> > > +
> > >       if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > >           return -1;
> > >       }
> > > +    if (reply_supported) {
> > > +        return process_message_reply(dev, &msg);
> > > +    }
> > > +
> > >       return 0;
> > >   }
> > > -- 
> > > 2.25.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
  2021-07-07 14:39     ` Michael S. Tsirkin
@ 2021-07-07 14:58       ` Denis Plotnikov
  2021-07-07 18:44         ` Michael S. Tsirkin
  0 siblings, 1 reply; 9+ messages in thread
From: Denis Plotnikov @ 2021-07-07 14:58 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, yc-core


On 07.07.2021 17:39, Michael S. Tsirkin wrote:
> On Wed, Jul 07, 2021 at 03:19:20PM +0300, Denis Plotnikov wrote:
>> On 07.07.2021 13:10, Michael S. Tsirkin wrote:
>>> On Fri, Jun 25, 2021 at 11:52:10AM +0300, Denis Plotnikov wrote:
>>>> On vhost-user-blk migration, qemu normally sends a number of commands
>>>> to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
>>>> Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
>>>> VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
>>>> data logging.
>>>> The issue is that qemu doesn't wait for reply from the vhost daemon
>>>> for these commands which may result in races between qemu expectation
>>>> of logging starting and actual login starting in vhost daemon.
>>> Could you be more explicit please? What kind of race have you
>>> observed? Getting a reply slows down the setup considerably and
>>> should not be done lightly.
>> I'm talking about the vhost-user-blk case. On migration setup, we enable
>> logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
>> vhost-user-blk daemon immediately and the daemon needs some time turn the
>> logging on internally. If qemu doesn't wait for reply, after sending the
>> command qemu may start migrate memory pages. At this time the logging may
>> not be actually turned on in the daemon  but some guest pages, which the
>> daemon is about to write to, may be already transferred without logging to a
>> destination. Since the logging wasn't turned on, those pages won't be
>> transferred again as dirty. So we may end up with corrupted data on the
>> destination.
>>
>> Have I managed to explain the case clearly?
>>
>> Thanks!
>>
>> Denis
> OK so this is just about enabling logging. It would be cleaner to
> defer migrating memory until response ... if that is too hard,
> at least document why we are doing this please.
> And, let's wait for an ack just in that case then - why not?
>
> And what about VHOST_USER_SET_PROTOCOL_FEATURES?

The code uses the same path for both VHOST_USER_SET_PROTOCOL_FEATURES 
and VHOST_USER_SET_FEATURES via vhost_user_set_u64(). So, I decided to 
suggest adding reply to both of them, so both feature setting commands 
work similarly as it doesn't contradicts with vhost-user spec.

I'm not sure that it worth doing that, so if you think it's not I'll 
just remove them.


Denis

>
>
>>> Thanks!
>>>
>>>> To resolve this issue, this patch makes qemu wait for the commands result
>>>> explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
>>>> Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
>>>> command to make the features setting functions work similary.
>>>>
>>>> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
>>>> ---
>>>>    hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>>> index ee57abe04526..e47b82adab00 100644
>>>> --- a/hw/virtio/vhost-user.c
>>>> +++ b/hw/virtio/vhost-user.c
>>>> @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>>>>            .hdr.size = sizeof(msg.payload.addr),
>>>>        };
>>>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>>>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>>>> +    if (reply_supported) {
>>>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>>>> +    }
>>>> +
>>>>        if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>>>>            return -1;
>>>>        }
>>>> +    if (reply_supported) {
>>>> +        return process_message_reply(dev, &msg);
>>>> +    }
>>>> +
>>>>        return 0;
>>>>    }
>>>> @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
>>>>            .hdr.size = sizeof(msg.payload.u64),
>>>>        };
>>>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>>>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>>>> +    if (reply_supported) {
>>>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>>>> +    }
>>>> +
>>>>        if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>>>>            return -1;
>>>>        }
>>>> +    if (reply_supported) {
>>>> +        return process_message_reply(dev, &msg);
>>>> +    }
>>>> +
>>>>        return 0;
>>>>    }
>>>> -- 
>>>> 2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
  2021-07-07 14:58       ` Denis Plotnikov
@ 2021-07-07 18:44         ` Michael S. Tsirkin
  2021-07-08  7:23           ` Denis Plotnikov
  0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2021-07-07 18:44 UTC (permalink / raw)
  To: Denis Plotnikov; +Cc: qemu-devel, yc-core

On Wed, Jul 07, 2021 at 05:58:50PM +0300, Denis Plotnikov wrote:
> 
> On 07.07.2021 17:39, Michael S. Tsirkin wrote:
> > On Wed, Jul 07, 2021 at 03:19:20PM +0300, Denis Plotnikov wrote:
> > > On 07.07.2021 13:10, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 25, 2021 at 11:52:10AM +0300, Denis Plotnikov wrote:
> > > > > On vhost-user-blk migration, qemu normally sends a number of commands
> > > > > to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
> > > > > Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
> > > > > VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
> > > > > data logging.
> > > > > The issue is that qemu doesn't wait for reply from the vhost daemon
> > > > > for these commands which may result in races between qemu expectation
> > > > > of logging starting and actual login starting in vhost daemon.
> > > > Could you be more explicit please? What kind of race have you
> > > > observed? Getting a reply slows down the setup considerably and
> > > > should not be done lightly.
> > > I'm talking about the vhost-user-blk case. On migration setup, we enable
> > > logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
> > > vhost-user-blk daemon immediately and the daemon needs some time turn the
> > > logging on internally. If qemu doesn't wait for reply, after sending the
> > > command qemu may start migrate memory pages. At this time the logging may
> > > not be actually turned on in the daemon  but some guest pages, which the
> > > daemon is about to write to, may be already transferred without logging to a
> > > destination. Since the logging wasn't turned on, those pages won't be
> > > transferred again as dirty. So we may end up with corrupted data on the
> > > destination.
> > > 
> > > Have I managed to explain the case clearly?
> > > 
> > > Thanks!
> > > 
> > > Denis
> > OK so this is just about enabling logging. It would be cleaner to
> > defer migrating memory until response ... if that is too hard,
> > at least document why we are doing this please.
> > And, let's wait for an ack just in that case then - why not?
> > 
> > And what about VHOST_USER_SET_PROTOCOL_FEATURES?
> 
> The code uses the same path for both VHOST_USER_SET_PROTOCOL_FEATURES and
> VHOST_USER_SET_FEATURES via vhost_user_set_u64(). So, I decided to suggest
> adding reply to both of them, so both feature setting commands work
> similarly as it doesn't contradicts with vhost-user spec.
> 
> I'm not sure that it worth doing that, so if you think it's not I'll just
> remove them.
> 
> 
> Denis


I'm inclined to say let's not add to the latency of setting up the
device unnecessarily.

> > 
> > 
> > > > Thanks!
> > > > 
> > > > > To resolve this issue, this patch makes qemu wait for the commands result
> > > > > explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
> > > > > Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
> > > > > command to make the features setting functions work similary.
> > > > > 
> > > > > Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> > > > > ---
> > > > >    hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
> > > > >    1 file changed, 20 insertions(+)
> > > > > 
> > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > index ee57abe04526..e47b82adab00 100644
> > > > > --- a/hw/virtio/vhost-user.c
> > > > > +++ b/hw/virtio/vhost-user.c
> > > > > @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
> > > > >            .hdr.size = sizeof(msg.payload.addr),
> > > > >        };
> > > > > +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> > > > > +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> > > > > +    if (reply_supported) {
> > > > > +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > > > +    }
> > > > > +
> > > > >        if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > > > >            return -1;
> > > > >        }
> > > > > +    if (reply_supported) {
> > > > > +        return process_message_reply(dev, &msg);
> > > > > +    }
> > > > > +
> > > > >        return 0;
> > > > >    }
> > > > > @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
> > > > >            .hdr.size = sizeof(msg.payload.u64),
> > > > >        };
> > > > > +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> > > > > +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> > > > > +    if (reply_supported) {
> > > > > +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > > > +    }
> > > > > +
> > > > >        if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > > > >            return -1;
> > > > >        }
> > > > > +    if (reply_supported) {
> > > > > +        return process_message_reply(dev, &msg);
> > > > > +    }
> > > > > +
> > > > >        return 0;
> > > > >    }
> > > > > -- 
> > > > > 2.25.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies
  2021-07-07 18:44         ` Michael S. Tsirkin
@ 2021-07-08  7:23           ` Denis Plotnikov
  0 siblings, 0 replies; 9+ messages in thread
From: Denis Plotnikov @ 2021-07-08  7:23 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, yc-core


On 07.07.2021 21:44, Michael S. Tsirkin wrote:
> On Wed, Jul 07, 2021 at 05:58:50PM +0300, Denis Plotnikov wrote:
>> On 07.07.2021 17:39, Michael S. Tsirkin wrote:
>>> On Wed, Jul 07, 2021 at 03:19:20PM +0300, Denis Plotnikov wrote:
>>>> On 07.07.2021 13:10, Michael S. Tsirkin wrote:
>>>>> On Fri, Jun 25, 2021 at 11:52:10AM +0300, Denis Plotnikov wrote:
>>>>>> On vhost-user-blk migration, qemu normally sends a number of commands
>>>>>> to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
>>>>>> Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
>>>>>> VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
>>>>>> data logging.
>>>>>> The issue is that qemu doesn't wait for reply from the vhost daemon
>>>>>> for these commands which may result in races between qemu expectation
>>>>>> of logging starting and actual login starting in vhost daemon.
>>>>> Could you be more explicit please? What kind of race have you
>>>>> observed? Getting a reply slows down the setup considerably and
>>>>> should not be done lightly.
>>>> I'm talking about the vhost-user-blk case. On migration setup, we enable
>>>> logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
>>>> vhost-user-blk daemon immediately and the daemon needs some time turn the
>>>> logging on internally. If qemu doesn't wait for reply, after sending the
>>>> command qemu may start migrate memory pages. At this time the logging may
>>>> not be actually turned on in the daemon  but some guest pages, which the
>>>> daemon is about to write to, may be already transferred without logging to a
>>>> destination. Since the logging wasn't turned on, those pages won't be
>>>> transferred again as dirty. So we may end up with corrupted data on the
>>>> destination.
>>>>
>>>> Have I managed to explain the case clearly?
>>>>
>>>> Thanks!
>>>>
>>>> Denis
>>> OK so this is just about enabling logging. It would be cleaner to
>>> defer migrating memory until response ... if that is too hard,
>>> at least document why we are doing this please.
>>> And, let's wait for an ack just in that case then - why not?
>>>
>>> And what about VHOST_USER_SET_PROTOCOL_FEATURES?
>> The code uses the same path for both VHOST_USER_SET_PROTOCOL_FEATURES and
>> VHOST_USER_SET_FEATURES via vhost_user_set_u64(). So, I decided to suggest
>> adding reply to both of them, so both feature setting commands work
>> similarly as it doesn't contradicts with vhost-user spec.
>>
>> I'm not sure that it worth doing that, so if you think it's not I'll just
>> remove them.
>>
>>
>> Denis
>
> I'm inclined to say let's not add to the latency of setting up the
> device unnecessarily.

ok

I'll remove reply for VHOST_USER_SET_FEATURES and amend the commit 
message in v2

Thanks!

Denis

>>>
>>>>> Thanks!
>>>>>
>>>>>> To resolve this issue, this patch makes qemu wait for the commands result
>>>>>> explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
>>>>>> Also, this patch adds the reply waiting for VHOST_USER_SET_PROTOCOL_FEATURES
>>>>>> command to make the features setting functions work similary.
>>>>>>
>>>>>> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
>>>>>> ---
>>>>>>     hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
>>>>>>     1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>>>>> index ee57abe04526..e47b82adab00 100644
>>>>>> --- a/hw/virtio/vhost-user.c
>>>>>> +++ b/hw/virtio/vhost-user.c
>>>>>> @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>>>>>>             .hdr.size = sizeof(msg.payload.addr),
>>>>>>         };
>>>>>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>>>>>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>>>>>> +    if (reply_supported) {
>>>>>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>>>>>> +    }
>>>>>> +
>>>>>>         if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>>>>>>             return -1;
>>>>>>         }
>>>>>> +    if (reply_supported) {
>>>>>> +        return process_message_reply(dev, &msg);
>>>>>> +    }
>>>>>> +
>>>>>>         return 0;
>>>>>>     }
>>>>>> @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
>>>>>>             .hdr.size = sizeof(msg.payload.u64),
>>>>>>         };
>>>>>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>>>>>> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
>>>>>> +    if (reply_supported) {
>>>>>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>>>>>> +    }
>>>>>> +
>>>>>>         if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>>>>>>             return -1;
>>>>>>         }
>>>>>> +    if (reply_supported) {
>>>>>> +        return process_message_reply(dev, &msg);
>>>>>> +    }
>>>>>> +
>>>>>>         return 0;
>>>>>>     }
>>>>>> -- 
>>>>>> 2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-08  7:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25  8:52 [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies Denis Plotnikov
2021-07-02  9:41 ` Denis Plotnikov
2021-07-07  9:55   ` [PING] " Denis Plotnikov
2021-07-07 10:10 ` Michael S. Tsirkin
2021-07-07 12:19   ` Denis Plotnikov
2021-07-07 14:39     ` Michael S. Tsirkin
2021-07-07 14:58       ` Denis Plotnikov
2021-07-07 18:44         ` Michael S. Tsirkin
2021-07-08  7:23           ` Denis Plotnikov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).