Re: [PATCH v4 04/10] Transmit vhost-user memory regions individually

[Raphael Norwitz]

On Thu, Jun 4, 2020 at 10:45 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, May 21, 2020 at 7:00 AM Raphael Norwitz <raphael.norwitz@nutanix.com> 
> wrote:
>>
>> With this change, when the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS
>> protocol feature has been negotiated, Qemu no longer sends the backend
>> all the memory regions in a single message. Rather, when the memory
>> tables are set or updated, a series of VHOST_USER_ADD_MEM_REG and
>> VHOST_USER_REM_MEM_REG messages are sent to transmit the regions to map
>> and/or unmap instead of sending send all the regions in one fixed size
>> VHOST_USER_SET_MEM_TABLE message.
>>
>> The vhost_user struct maintains a shadow state of the VM’s memory
>> regions. When the memory tables are modified, the
>> vhost_user_set_mem_table() function compares the new device memory state
>> to the shadow state and only sends regions which need to be unmapped or
>> mapped in. The regions which must be unmapped are sent first, followed
>> by the new regions to be mapped in. After all the messages have been
>> sent, the shadow state is set to the current virtual device state.
>>
>> Existing backends which do not support
>> VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS are unaffected.
>>
>> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
>> Signed-off-by: Swapnil Ingle <swapnil.ingle@nutanix.com>
>> Signed-off-by: Peter Turschmid <peter.turschm@nutanix.com>
>> Suggested-by: Mike Cui <cui@nutanix.com>
> >
> >
> > The change is a bit tricky, why not add more pre-condition/post-condition 
> > checks? This could really help in case we missed some OOB conditions.

Here are a few conditions I could think of:

1. I can ensure that new regions won't overrun the shadow table - just
putting an assert(u->num_shadow_regions + add_idx - rm_idx <
VHOST_MEMORY_MAX_NREGIONS); at the end of scrub_shadow_regions()

2. I could add a precondition asserting that none of the regions in
the shadow table are overlapping.

3. i could add a post condition check that all remove regions are in
the shadow table but not the device memory state and that all add
regions are in the device memory state but not in the shadow table.

Can you think of any others?

(1) is simple but (2) and (3) are more involved and will introduce
overhead. I'm not sure how valuable they are, but if you want I can
tack them on to the end of the series.

>
> > I would also use longer names: rem->remove, reg->registry, etc.. I think 
> > they improve readability.

Sure - happy to change the variable names.

>
> > Nonetheless, it looks ok and apparently 4 people already looked at it, so 
> > for now:
> > Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Also did you have any comments on the rest of the libvhost-user
patches before I ship a V5?


>
>> ---
>>  docs/interop/vhost-user.rst |  33 ++-
>>  hw/virtio/vhost-user.c      | 510 
>> +++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 469 insertions(+), 74 deletions(-)