[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [Qemu-devel] [QEMU PATCH v10 3/3] tests/migration: Add te

From: Jianjun Duan
Subject: Re: [Qemu-ppc] [Qemu-devel] [QEMU PATCH v10 3/3] tests/migration: Add test for QTAILQ migration
Date: Thu, 3 Nov 2016 11:40:06 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0

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 <address@hidden> wrote:
>>>>>>>> Add a test for QTAILQ migration to tests/test-vmstate.c.
>>>>>>>> Signed-off-by: Jianjun Duan <address@hidden>
>>>>>> Reviewed-by: Juan Quintela <address@hidden>
>>>> 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.

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.

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

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

> Sorry if I'm bothering you with nonsense.
> Greetings,
> Halil
>> Thanks,
>> Jianjun

reply via email to

[Prev in Thread] Current Thread [Next in Thread]