Re: [Qemu-devel] [PATCH 2/2] vhost-user: only seek a reply if needed in set_mem_table

[Michael S. Tsirkin]

On Thu, Sep 08, 2016 at 11:33:34AM +0000, Prerna Saxena wrote:
> Hi Maxime,
> 
> 
> On 08/09/16 2:04 pm, "Maxime Coquelin" <address@hidden> wrote:
> 
> >The goal of this patch is to only request a sync (reply_ack,
> >or get_features) in set_mem_table only when necessary.
> >
> >It should not be necessary the first time we set the table,
> >or when we add a new regions which hadn't been merged with an
> >existing ones.
> 
> 
> I don’t think so. 
> This patch is not helping us solve the issue.
> The hang introduced by original use of get_features() in set_mem_table
> was traced down to use of TCG mode for vhost-user test.

Right but we don't know for sure there are no backends
that do this kind of simplistic thing, and assume that
get features does not happen later. And for most setups
without memory hotplug, they would be right.


> This has now
> been fixed via:
> 
> -----
> commit cdafe929615ec5eca71bcd5a3d12bab5678e5886
> Author: Eduardo Habkost <address@hidden>
> Date:   Fri Sep 2 15:59:43 2016 -0300
> 
> 
>     vhost-user-test: Use libqos instead of pxe-virtio.rom
>     
>     vhost-user-test relies on iPXE just to initialize the virtio-net
>     device, and doesn't do any actual packet tx/rx testing.
>     
>     In addition to that, the test relies on TCG, which is
>     imcompatible with vhost. The test only worked by accident: a bug
>     the memory backend initialization made memory regions not have
>     the DIRTY_MEMORY_CODE bit set in dirty_log_mask.
>     
>     This changes vhost-user-test to initialize the virtio-net device
>     using libqos, and not use TCG nor pxe-virtio.rom.
>     
>     Signed-off-by: Eduardo Habkost <address@hidden>
> 
> -------
> 
> So I think the original hang seems to have been fixed with Patch 1/2 of this 
> series alone.
> 
> Regarding Patch 2/2:
> This patch seems to filter responses from set_mem_table only for certain 
> updates of memory regions. It violates the definition of the REPLY_ACK 
> feature. This feature expects the client to send a response for every call of 
> set_mem_table. And here, qemu exits the set_mem_table() function in some 
> cases without even waiting for the reply that is going to come in.
> 
> As for use of this approach with get_features, we have already debated that 
> on the list before : 
> https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00689.html
> To quote:
> "I do not entirely agree with that. The first set_mem_table command is not 
> much
> different from subsequent set_mem_table calls."
> 
> Regards,
> Prerna
>