| Message ID | cda939fc-7da7-89a3-dfcc-8c81f214bce1@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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. The current design is to append the qtailq from source to the corresponding one on target. It works well for the task in hard such as migrating ccs_list and pending_events for DRC objects. I suspect in most cases the qtailqs on target are empty. 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. Thanks, Jianjun > Halil > > -----------------------8<------------------------------------- > > From 4323c308c5f56ed86eb0a5bb6027bca4617ecc8c Mon Sep 17 00:00:00 2001 > From: Halil Pasic <pasic@linux.vnet.ibm.com> > Date: Thu, 3 Nov 2016 13:07:05 +0100 > Subject: [PATCH] tests/test-vmstate.c: add test empty qtailq > > Add test for empty qtailq. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > -- > > For fixup. > --- > tests/test-vmstate.c | 28 ++++++++++++++++++++++++++++ > 1 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c > index a992408..789f07a 100644 > --- a/tests/test-vmstate.c > +++ b/tests/test-vmstate.c > @@ -126,6 +126,14 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj, > return ret; > } > > +static void load_vmstate_one_obj(const VMStateDescription *vmsd, void *obj, > + int version_id) > +{ > + QEMUFile *fload = open_test_file(false); > + > + SUCCESS(vmstate_load_state(fload, vmsd, obj, version_id)); > + qemu_fclose(fload); > +} > > static int load_vmstate(const VMStateDescription *desc, > void *obj, void *obj_clone, > @@ -633,6 +641,25 @@ static void test_load_q(void) > qemu_fclose(fload); > } > > + > +static void test_sl_empty_q(void) > +{ > + TestQtailq obj_q = { > + .i16 = -512, > + .i32 = 70000, > + }; > + TestQtailq tgt = {.q = {.tqh_first = (void *)1}}; > + > + QTAILQ_INIT(&obj_q.q); > + > + save_vmstate(&vmstate_q, &obj_q); > + load_vmstate_one_obj(&vmstate_q, &tgt, 1); > + g_assert_cmpint(tgt.i16, ==, obj_q.i16); > + g_assert_cmpint(tgt.i32, ==, obj_q.i32); > + g_assert_cmpint(QTAILQ_EMPTY(&(tgt.q)),!=, false); > + > +} > + > int main(int argc, char **argv) > { > temp_fd = mkstemp(temp_file); > @@ -649,6 +676,7 @@ int main(int argc, char **argv) > g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip); > g_test_add_func("/vmstate/qtailq/save/saveq", test_save_q); > g_test_add_func("/vmstate/qtailq/load/loadq", test_load_q); > + g_test_add_func("/vmstate/qtailq/empty", test_sl_empty_q); > g_test_run(); > > close(temp_fd); >
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. > 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. > 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? > 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. > 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. Sorry if I'm bothering you with nonsense. Greetings, Halil > > Thanks, > Jianjun >
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. 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. Thanks, Jianjun > Sorry if I'm bothering you with nonsense. > > Greetings, > Halil > >> >> Thanks, >> Jianjun >> >
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
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c index a992408..789f07a 100644 --- a/tests/test-vmstate.c +++ b/tests/test-vmstate.c @@ -126,6 +126,14 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj, return ret; } +static void load_vmstate_one_obj(const VMStateDescription *vmsd, void *obj, + int version_id) +{ + QEMUFile *fload = open_test_file(false); + + SUCCESS(vmstate_load_state(fload, vmsd, obj, version_id)); + qemu_fclose(fload); +} static int load_vmstate(const VMStateDescription *desc, void *obj, void *obj_clone, @@ -633,6 +641,25 @@ static void test_load_q(void) qemu_fclose(fload); } + +static void test_sl_empty_q(void) +{ + TestQtailq obj_q = { + .i16 = -512, + .i32 = 70000, + }; + TestQtailq tgt = {.q = {.tqh_first = (void *)1}}; + + QTAILQ_INIT(&obj_q.q); + + save_vmstate(&vmstate_q, &obj_q); + load_vmstate_one_obj(&vmstate_q, &tgt, 1); + g_assert_cmpint(tgt.i16, ==, obj_q.i16); + g_assert_cmpint(tgt.i32, ==, obj_q.i32); + g_assert_cmpint(QTAILQ_EMPTY(&(tgt.q)),!=, false); + +} + int main(int argc, char **argv) { temp_fd = mkstemp(temp_file); @@ -649,6 +676,7 @@ int main(int argc, char **argv) g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip); g_test_add_func("/vmstate/qtailq/save/saveq", test_save_q); g_test_add_func("/vmstate/qtailq/load/loadq", test_load_q); + g_test_add_func("/vmstate/qtailq/empty", test_sl_empty_q); g_test_run(); close(temp_fd);