Re: [Qemu-devel] [QEMU PATCH v10 3/3] tests/migration: Add test for QTAILQ migration

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 11/03/2016 07:40 PM, Jianjun Duan wrote:
> 
> 
> On 11/03/2016 10:17 AM, Halil Pasic wrote:
>>
>>
>> On 11/03/2016 05:47 PM, Jianjun Duan wrote:
>>>
>>> On 11/03/2016 05:22 AM, Halil Pasic wrote:
>>>>>
>>>>>
>>>>> On 11/02/2016 11:47 AM, Juan Quintela wrote:
>>>>>>> Jianjun Duan <duanj@linux.vnet.ibm.com> wrote:
>>>>>>>>> Add a test for QTAILQ migration to tests/test-vmstate.c.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
>>>>>>>
>>>>>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>>>>>
>>>>>
>>>>> Empty QTAILQ seems to be broken. Have written a small
>>>>> test to prove my point. It May even make sense to have such
>>>>> a test in the test-suite (some prettyfication might be
>>>>> necessary though).
>>>>>
>>> It is working as intended.
>>>
>>
>> My train of thought was that the object holding the queue might
>> be dynamically allocated by the migration code or otherwise
>> uninitialized. I was unaware these scenarios are prohibited.
>>
>>
> This is a valid point. To get this covered vmstate_load_state needs to
> be revised so that at any moment of recursion we know if the field is in
> a dynamic created structure. If yes the structures which need
> initialization such as QTAILQ can be initialized.
> 

Or you just zero out the head in VMStateInfo.get right away and to not care
what was there. Of course this is only if loading to non-empty
lists is invalid. This is why I cared describing why I think
it is (invalid).

> I would leave this until the need is there. In current device migration
> code I imagine such scenarios would be rare if they should appear at
> all. Because all the devices (even the hotplugged ones) are already
> initialized on target. So a QTAILQ in such context should already be
> initialized. Otherwise it should be fixed.
> 

I agree that this type of a solution is an overkill for something
nobody needs at the moment.

>>> The current design is to append the qtailq from source to the
>>> corresponding one on target. 
>>
>> I do not see this documented. I'm used to vmstate_load overwriting
>> values and following pointers, so IMHO it is not obvious that
>> qtailq load does append.
>>
> 
> I will document this.
> 

I'm fine with this option too but I think I would slightly prefer
the solution described above. Maybe some of the more experienced
guys (think Paolo, Juan, Dave) will come up with some guidance.

>>> It works well for the task in hard
>>> such as migrating ccs_list and pending_events for DRC objects.
>>>
>>
>> Because target head is always properly initialized to empty queue?
>>
> They may not be empty. But they should be initialized.
>>> I suspect in most cases the qtailqs on target are empty. 
>>
>> If I think about migration having no queues populated with
>> elements on a target site sounds very reasonable since IFAIU
>> the target should not do any work which would populate these
>> data structures.
>>
> See above.
>>
> 
>>
>>> If not,
>>> appending to them is a good choice. Clearing them is tricky since
>>> each queue probably require a specialized routine to clean. If they
>>> are not empty there are must be good reasons for that.
>>
>> Have you some code or a scenario in mind where this is legit? I
>> mean creating a mix of the state(?) we found at the target and
>> the state captured at the source does not sound right. I would
>> argue that the target should not have any state which is subject
>> to migration.
>>
>> You are right a non-empty queue is trouble, and frankly I never
>> considered it as a valid scenario.
>>
> It may not be a mix of state. It really depends on how the overall state
> of the devices is designed. If there are dependence between different
> elements of the state, then difference in these elements may break some
> consistency. One example is that migrating the length of the qtailq but
> appending the content to a non-empty qtailq. In such a case the length
> should not be migrated. It should be calculated on target instead.
> 
> It should be treated case by case.
> 

I did not get your argument here, but I think we can both live
without me understanding this.

Halil