[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: Halil Pasic
Subject: Re: [Qemu-ppc] [Qemu-devel] [QEMU PATCH v10 3/3] tests/migration: Add test for QTAILQ migration
Date: Thu, 3 Nov 2016 19:51:06 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0

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

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.


reply via email to

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