[Qemu-devel] Should memory hotplug work with vhost-user backends?

[Qemu-devel] Should memory hotplug work with vhost-user backends?

[Raphael Norwitz]

For background I am trying to work around a ram slot limit imposed by the vhost-user protocol. We are having trouble reconciling the comment here: https://github.com/qemu/qemu/blob/master/hw/virtio/vhost-user.c#L333  that “For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE., we just need to send it once the first time” and the high level implementation of memory hot-add, which calls set_mem_table every time a VM hot adds memory.

A few questions:
1.
What exactly is the check `if (vhost_user_one_time_request(msg->hdr.request) && dev->vq_index != 0)` for? In the message for commit b931bfbf042983f311b3b09894d8030b2755a638, which introduced the check, I see it says “non-vring specific messages[, which should] be sent only once” and gives VHOST_USER_SET_MEM_TABLE as an example one such message. The `vhost_user_one_time_request()` call clearly checks whether this type of message is the kind of message is supposed to be sent once of which VHOST_USER_SET_MEM_TABLE is one. Why, then, does this commit add the check if `dev->vq_index != 0`? It seems like there is a latent assumption that after the first call dev->vq_index should be set to some value greater than one, however for many cases such as vhost-user-scsi devices we can see this is clearly not the case https://github.com/qemu/qemu/blob/master/hw/scsi/vhost-user-scsi.c#L95. Is this check then ‘broken’ for such devices?

2.
If this check is indeed broken for such devices, and set_mem_table call is only supposed to be run once for such devices, is the ability to call it multiple times technically a bug for devices such as vhost-user-scsci devices? If so, this would imply that the existing ability to hot add memory to vhost-user-scsi devices is by extension technically a bug/unintended behavior. Is this the case?

Thanks,
Raphael

Re: [Qemu-devel] Should memory hotplug work with vhost-user backends?

[Stefan Hajnoczi]

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

On Tue, Jul 02, 2019 at 10:08:54PM +0000, Raphael Norwitz wrote:
> For background I am trying to work around a ram slot limit imposed by the vhost-user protocol. We are having trouble reconciling the comment here: https://github.com/qemu/qemu/blob/master/hw/virtio/vhost-user.c#L333  that “For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE., we just need to send it once the first time” and the high level implementation of memory hot-add, which calls set_mem_table every time a VM hot adds memory.
> 
> A few questions:
> 1.
> What exactly is the check `if (vhost_user_one_time_request(msg->hdr.request) && dev->vq_index != 0)` for? In the message for commit b931bfbf042983f311b3b09894d8030b2755a638, which introduced the check, I see it says “non-vring specific messages[, which should] be sent only once” and gives VHOST_USER_SET_MEM_TABLE as an example one such message. The `vhost_user_one_time_request()` call clearly checks whether this type of message is the kind of message is supposed to be sent once of which VHOST_USER_SET_MEM_TABLE is one. Why, then, does this commit add the check if `dev->vq_index != 0`? It seems like there is a latent assumption that after the first call dev->vq_index should be set to some value greater than one, however for many cases such as vhost-user-scsi devices we can see this is clearly not the case https://github.com/qemu/qemu/blob/master/hw/scsi/vhost-user-scsi.c#L95. Is this check then ‘broken’ for such devices?
> 
> 2.
> If this check is indeed broken for such devices, and set_mem_table call is only supposed to be run once for such devices, is the ability to call it multiple times technically a bug for devices such as vhost-user-scsci devices? If so, this would imply that the existing ability to hot add memory to vhost-user-scsi devices is by extension technically a bug/unintended behavior. Is this the case?

Hi Raphael,
David Gilbert and I recently came to the conclusion that memory hotplug
is not safe in vhost-user device backends built using libvhost-user.

It's likely that memory hotplug hasn't been fully thought through at the
protocol specification and QEMU vhost-user master implementation levels
either.

We didn't investigate deeper for the time being, but I'm not surprised
that you've found inconsistencies.  The ability to hotplug memory is a
valuable feature.  It will be necessary to get it working sooner or
later.

Are you going to work on it?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] Should memory hotplug work with vhost-user backends?

[Raphael Norwitz]

On Wed, Jul 03, 2019 at 11:04:31AM +0100, Stefan Hajnoczi wrote:
> On Tue, Jul 02, 2019 at 10:08:54PM +0000, Raphael Norwitz wrote:
> > For background I am trying to work around a ram slot limit imposed by the vhost-user protocol. We are having trouble reconciling the comment here: https://github.com/qemu/qemu/blob/master/hw/virtio/vhost-user.c#L333  that “For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE., we just need to send it once the first time” and the high level implementation of memory hot-add, which calls set_mem_table every time a VM hot adds memory.
> > 
> > A few questions:
> > 1.
> > What exactly is the check `if (vhost_user_one_time_request(msg->hdr.request) && dev->vq_index != 0)` for? In the message for commit b931bfbf042983f311b3b09894d8030b2755a638, which introduced the check, I see it says “non-vring specific messages[, which should] be sent only once” and gives VHOST_USER_SET_MEM_TABLE as an example one such message. The `vhost_user_one_time_request()` call clearly checks whether this type of message is the kind of message is supposed to be sent once of which VHOST_USER_SET_MEM_TABLE is one. Why, then, does this commit add the check if `dev->vq_index != 0`? It seems like there is a latent assumption that after the first call dev->vq_index should be set to some value greater than one, however for many cases such as vhost-user-scsi devices we can see this is clearly not the case https://github.com/qemu/qemu/blob/master/hw/scsi/vhost-user-scsi.c#L95. Is this check then ‘broken’ for such devices?
> > 
> > 2.
> > If this check is indeed broken for such devices, and set_mem_table call is only supposed to be run once for such devices, is the ability to call it multiple times technically a bug for devices such as vhost-user-scsci devices? If so, this would imply that the existing ability to hot add memory to vhost-user-scsi devices is by extension technically a bug/unintended behavior. Is this the case?
> 
> Hi Raphael,
> David Gilbert and I recently came to the conclusion that memory hotplug
> is not safe in vhost-user device backends built using libvhost-user.

Hi David, Sefan,

Just want to follow up here. Sorry - I know this was a while ago.

I am looking to add postcopy migration support for my patch set lifting
the vhost-user max ram slots limitation
(https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg06641.html)
and it seems the most convienient way to do this would be to add support
for my new protocol feature in libvhost-user and then test with
vhost-user-bridge.

I've briefly looked through the libvhost-user code and the hot-add path
looks safe enough to me (or at least no more broken than the regular
vhost-user memory hot-add path).

Can you elaborate a little more about why memory hot-add is unsafe with
vhost-user device backends built with libvhost-user, as opposed to those
using the raw vhost-user protocol semantics?


Thanks,
Raphael

> 
> It's likely that memory hotplug hasn't been fully thought through at the
> protocol specification and QEMU vhost-user master implementation levels
> either.
> 
> We didn't investigate deeper for the time being, but I'm not surprised
> that you've found inconsistencies.  The ability to hotplug memory is a
> valuable feature.  It will be necessary to get it working sooner or
> later.
> 
> Are you going to work on it?
> 
> Stefan

Re: [Qemu-devel] Should memory hotplug work with vhost-user backends?

[Stefan Hajnoczi]

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

On Thu, Apr 09, 2020 at 08:21:16PM -0400, Raphael Norwitz wrote:
> On Wed, Jul 03, 2019 at 11:04:31AM +0100, Stefan Hajnoczi wrote:
> > On Tue, Jul 02, 2019 at 10:08:54PM +0000, Raphael Norwitz wrote:
> > > For background I am trying to work around a ram slot limit imposed by the vhost-user protocol. We are having trouble reconciling the comment here: https://github.com/qemu/qemu/blob/master/hw/virtio/vhost-user.c#L333  that “For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE., we just need to send it once the first time” and the high level implementation of memory hot-add, which calls set_mem_table every time a VM hot adds memory.
> > > 
> > > A few questions:
> > > 1.
> > > What exactly is the check `if (vhost_user_one_time_request(msg->hdr.request) && dev->vq_index != 0)` for? In the message for commit b931bfbf042983f311b3b09894d8030b2755a638, which introduced the check, I see it says “non-vring specific messages[, which should] be sent only once” and gives VHOST_USER_SET_MEM_TABLE as an example one such message. The `vhost_user_one_time_request()` call clearly checks whether this type of message is the kind of message is supposed to be sent once of which VHOST_USER_SET_MEM_TABLE is one. Why, then, does this commit add the check if `dev->vq_index != 0`? It seems like there is a latent assumption that after the first call dev->vq_index should be set to some value greater than one, however for many cases such as vhost-user-scsi devices we can see this is clearly not the case https://github.com/qemu/qemu/blob/master/hw/scsi/vhost-user-scsi.c#L95. Is this check then ‘broken’ for such devices?
> > > 
> > > 2.
> > > If this check is indeed broken for such devices, and set_mem_table call is only supposed to be run once for such devices, is the ability to call it multiple times technically a bug for devices such as vhost-user-scsci devices? If so, this would imply that the existing ability to hot add memory to vhost-user-scsi devices is by extension technically a bug/unintended behavior. Is this the case?
> > 
> > Hi Raphael,
> > David Gilbert and I recently came to the conclusion that memory hotplug
> > is not safe in vhost-user device backends built using libvhost-user.
> 
> Hi David, Sefan,
> 
> Just want to follow up here. Sorry - I know this was a while ago.
> 
> I am looking to add postcopy migration support for my patch set lifting
> the vhost-user max ram slots limitation
> (https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg06641.html)
> and it seems the most convienient way to do this would be to add support
> for my new protocol feature in libvhost-user and then test with
> vhost-user-bridge.
> 
> I've briefly looked through the libvhost-user code and the hot-add path
> looks safe enough to me (or at least no more broken than the regular
> vhost-user memory hot-add path).
> 
> Can you elaborate a little more about why memory hot-add is unsafe with
> vhost-user device backends built with libvhost-user, as opposed to those
> using the raw vhost-user protocol semantics?

The libvhost-user problem is that the library is mostly designed for a
single-threaded event loop that handles all virtqueue and vhost-user
protocol activity.

As soon as virtqueues are handled by dedicated threads there are race
conditions between the virtqueue threads and the vhost-user protocol
thread.

A virtqueue thread may or may not have an up-to-date view of memory
translation.  This can result in it missing memory that is currently
being hotplugged and continuing to access memory that has been removed.

Dave might have additional comments.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] Should memory hotplug work with vhost-user backends?

[Michael S. Tsirkin]

On Tue, Jul 02, 2019 at 10:08:54PM +0000, Raphael Norwitz wrote:
> For background I am trying to work around a ram slot limit imposed by the
> vhost-user protocol. We are having trouble reconciling the comment here: https:
> //github.com/qemu/qemu/blob/master/hw/virtio/vhost-user.c#L333  that “For
> non-vring specific requests, like VHOST_USER_SET_MEM_TABLE., we just need to
> send it once the first time” and the high level implementation of memory
> hot-add, which calls set_mem_table every time a VM hot adds memory.

IIUC the comment refers to multiple virtqueue. It is trying to say that
we do not need to send VHOST_USER_SET_MEM_TABLE for each
virtqueue.



>  
> 
> A few questions:
> 
> 1.
> 
> What exactly is the check `if (vhost_user_one_time_request(msg->hdr.request) &&
> dev->vq_index != 0)` for?

Some backends register multiple dev instances per backend: one for each
virtqueue.  This check avoids sending VHOST_USER_SET_MEM_TABLE more than
once in this case.


> In the message for commit
> b931bfbf042983f311b3b09894d8030b2755a638, which introduced the check, I see it
> says “non-vring specific messages[, which should] be sent only once” and gives
> VHOST_USER_SET_MEM_TABLE as an example one such message. The
> `vhost_user_one_time_request()` call clearly checks whether this type of
> message is the kind of message is supposed to be sent once of which
> VHOST_USER_SET_MEM_TABLE is one. Why, then, does this commit add the check if
> `dev->vq_index != 0`? It seems like there is a latent assumption that after the
> first call dev->vq_index should be set to some value greater than one, however
> for many cases such as vhost-user-scsi devices we can see this is clearly not
> the case https://github.com/qemu/qemu/blob/master/hw/scsi/vhost-user-scsi.c#
> L95. Is this check then ‘broken’ for such devices?

I think vhost-scsi has a single instance per backend, that is
why vq_index is 0.

>  
> 
> 2.
> 
> If this check is indeed broken for such devices, and set_mem_table call is only
> supposed to be run once for such devices, is the ability to call it multiple
> times technically a bug for devices such as vhost-user-scsci devices? If so,
> this would imply that the existing ability to hot add memory to vhost-user-scsi
> devices is by extension technically a bug/unintended behavior. Is this the
> case?
> 
>  
> 
> Thanks,
> 
> Raphael

I don't think that is the case. It's possible that memory hotplug has
bugs with vhost-user, but I don't think it's anything fundamental.

-- 
MST

Re: [Qemu-devel] Should memory hotplug work with vhost-user backends?

[Raphael Norwitz]

On Jul 3, 2019, at 11:57 AM, Michael S. Tsirkin <mst@redhat.com<mailto:mst@redhat.com>> wrote:

On Tue, Jul 02, 2019 at 10:08:54PM +0000, Raphael Norwitz wrote:
For background I am trying to work around a ram slot limit imposed by the
vhost-user protocol. We are having trouble reconciling the comment here: https:
//github.com/qemu/qemu/blob/master/hw/virtio/vhost-user.c#L333<http://github.com/qemu/qemu/blob/master/hw/virtio/vhost-user.c#L333>  that “For
non-vring specific requests, like VHOST_USER_SET_MEM_TABLE., we just need to
send it once the first time” and the high level implementation of memory
hot-add, which calls set_mem_table every time a VM hot adds memory.

IIUC the comment refers to multiple virtqueue. It is trying to say that
we do not need to send VHOST_USER_SET_MEM_TABLE for each
virtqueue.





A few questions:

1.

What exactly is the check `if (vhost_user_one_time_request(msg->hdr.request) &&
dev->vq_index != 0)` for?

Some backends register multiple dev instances per backend: one for each
virtqueue.  This check avoids sending VHOST_USER_SET_MEM_TABLE more than
once in this case.


In the message for commit
b931bfbf042983f311b3b09894d8030b2755a638, which introduced the check, I see it
says “non-vring specific messages[, which should] be sent only once” and gives
VHOST_USER_SET_MEM_TABLE as an example one such message. The
`vhost_user_one_time_request()` call clearly checks whether this type of
message is the kind of message is supposed to be sent once of which
VHOST_USER_SET_MEM_TABLE is one. Why, then, does this commit add the check if
`dev->vq_index != 0`? It seems like there is a latent assumption that after the
first call dev->vq_index should be set to some value greater than one, however
for many cases such as vhost-user-scsi devices we can see this is clearly not
the case https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_qemu_qemu_blob_master_hw_scsi_vhost-2Duser-2Dscsi.c-23&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=In4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk&m=JQ2hn-RhTfyhZ0Fxq5mzERaoVzb7cT8vcI7Xv20J3yk&s=aIeMP5ifKxF3no26c_M5x1rnVH0fhoFU2iRG9BPRNBk&e=
L95. Is this check then ‘broken’ for such devices?

I think vhost-scsi has a single instance per backend, that is
why vq_index is 0.



2.

If this check is indeed broken for such devices, and set_mem_table call is only
supposed to be run once for such devices, is the ability to call it multiple
times technically a bug for devices such as vhost-user-scsci devices? If so,
this would imply that the existing ability to hot add memory to vhost-user-scsi
devices is by extension technically a bug/unintended behavior. Is this the
case?



Thanks,

Raphael

I don't think that is the case. It's possible that memory hotplug has
bugs with vhost-user, but I don't think it's anything fundamental.

--
MST

Thanks for the responses Stefan and MST, I’ll be looking into it and posting an RFC when I'm ready.

Re: [Qemu-devel] Should memory hotplug work with vhost-user backends?

[Raphael Norwitz]

> > I've briefly looked through the libvhost-user code and the hot-add path
> > looks safe enough to me (or at least no more broken than the regular
> > vhost-user memory hot-add path).
> >
> > Can you elaborate a little more about why memory hot-add is unsafe with
> > vhost-user device backends built with libvhost-user, as opposed to those
> > using the raw vhost-user protocol semantics?
>
> The libvhost-user problem is that the library is mostly designed for a
> single-threaded event loop that handles all virtqueue and vhost-user
> protocol activity.
>
> As soon as virtqueues are handled by dedicated threads there are race
> conditions between the virtqueue threads and the vhost-user protocol
> thread.
>
> A virtqueue thread may or may not have an up-to-date view of memory
> translation.  This can result in it missing memory that is currently
> being hotplugged and continuing to access memory that has been removed.
>

I agree - this is definitely seems like a problem if memory is being removed,
but I don’t see how a virtqueue thread may have an outdated view of memory
in the hot-add case.

libvhost-user now supports the REPLY_ACK feature, so that on hot-add qemu
will wait for a response from the backend, confirming the new memory was
successfully mapped in, before returning from vhost_user_set_mem_table(). If
the new memory is mapped in by the backend before the ram is exposed to the
guest, how could a virtqueue thread receive operations on missing memory?

> Dave might have additional comments.
>
> Stefan

Re: [Qemu-devel] Should memory hotplug work with vhost-user backends?

[Dr. David Alan Gilbert]

* Raphael Norwitz (raphael.s.norwitz@gmail.com) wrote:
> > > I've briefly looked through the libvhost-user code and the hot-add path
> > > looks safe enough to me (or at least no more broken than the regular
> > > vhost-user memory hot-add path).
> > >
> > > Can you elaborate a little more about why memory hot-add is unsafe with
> > > vhost-user device backends built with libvhost-user, as opposed to those
> > > using the raw vhost-user protocol semantics?
> >
> > The libvhost-user problem is that the library is mostly designed for a
> > single-threaded event loop that handles all virtqueue and vhost-user
> > protocol activity.
> >
> > As soon as virtqueues are handled by dedicated threads there are race
> > conditions between the virtqueue threads and the vhost-user protocol
> > thread.
> >
> > A virtqueue thread may or may not have an up-to-date view of memory
> > translation.  This can result in it missing memory that is currently
> > being hotplugged and continuing to access memory that has been removed.
> >
> 
> I agree - this is definitely seems like a problem if memory is being removed,
> but I don’t see how a virtqueue thread may have an outdated view of memory
> in the hot-add case.

In the vhost-user client it gets a bit tricky because the setmemtable
command doesn't give differences - it gives a whole mapping table; so
the way libvhost-user does it is to perform a whole new set of mappings.
While the qemu and kernel side doesn't see any change, the mappings do
change in the client.  This probably needs a protocol improvement.

> libvhost-user now supports the REPLY_ACK feature, so that on hot-add qemu
> will wait for a response from the backend, confirming the new memory was
> successfully mapped in, before returning from vhost_user_set_mem_table(). If
> the new memory is mapped in by the backend before the ram is exposed to the
> guest, how could a virtqueue thread receive operations on missing memory?

Yeh, I think use of that new memory isn't that bad.

Dave

> > Dave might have additional comments.
> >
> > Stefan
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK