Re: [Qemu-devel] [PULL 05/21] migration: Introduce ignore-shared capability

[Markus Armbruster]

Juan Quintela <address@hidden> writes:

> Markus Armbruster <address@hidden> wrote:
>> "Dr. David Alan Gilbert (git)" <address@hidden> writes:
>>
>>> From: Yury Kotov <address@hidden>
>>>
>>> We want to use local migration to update QEMU for running guests.
>>> In this case we don't need to migrate shared (file backed) RAM.
>>> So, add a capability to ignore such blocks during live migration.
>>>
>>> Signed-off-by: Yury Kotov <address@hidden>
>>> Message-Id: <address@hidden>
>>> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>>> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
>>> ---
>> [...]
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 1fd7bbea9b..eab87340b2 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -409,13 +409,16 @@
>>>  #           devices (and thus take locks) immediately at the end of 
>>> migration.
>>>  #           (since 3.0)
>>>  #
>>> +# @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 
>>> 4.0)
>>
>> What exactly is considered "shared memory"?
>>
>> Specifically: say you have an ivshmem-plain device.  Is its shared
>> memory migrated?
>>
>> No objection to the pull request; if documentation improvements are
>> necessary, we can do them in a follow-up patch.
>
> They migrate to the same host to update the running qemu.
> What they are trying to do is to share the guest memory so they don't
> have to copy that memory (remember that they are inside the same host).
>
> It is up to you to decide if that is a great idea or "abuse" the
> interface (Anyways the change is small enough).

I'm not passing judgement on the feature, only on the doc comment: it
uses the term "shared memory" without defining it!  It really should,
because "shared memory" is commonly used in ways that do not apply here,
such as "the memory shared with other processes" (this includes shared
objects), "System V shared memory", and possibly more.

Please address this in a follow-up patch.