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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature