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

[Prerna Saxena]

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. 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