| Message ID | 88bb646d-39aa-e439-4b30-2c38777a4b56@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 10/31/2016 06:10 AM, Halil Pasic wrote: > > > On 10/28/2016 09:46 PM, Jianjun Duan wrote: >> >> >> On 10/28/2016 12:06 PM, Dr. David Alan Gilbert wrote: >>> * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: >>>> Currently we cannot directly transfer a QTAILQ instance because of the >>>> limitation in the migration code. Here we introduce an approach to >>>> transfer such structures. We created VMStateInfo vmstate_info_qtailq >>>> for QTAILQ. Similar VMStateInfo can be created for other data structures >>>> such as list. >>>> >>>> This approach will be used to transfer pending_events and ccs_list in spapr >>>> state. >>>> >>>> We also create some macros in qemu/queue.h to access a QTAILQ using pointer >>>> arithmetic. This ensures that we do not depend on the implementation >>>> details about QTAILQ in the migration code. >>>> >>>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> >>>> --- >>>> include/migration/vmstate.h | 20 ++++++++++++++ >>>> include/qemu/queue.h | 61 +++++++++++++++++++++++++++++++++++++++++ >>>> migration/trace-events | 4 +++ >>>> migration/vmstate.c | 67 +++++++++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 152 insertions(+) >>>> >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >>>> index d0e37b5..318a6f1 100644 >>>> --- a/include/migration/vmstate.h >>>> +++ b/include/migration/vmstate.h >>>> @@ -251,6 +251,7 @@ extern const VMStateInfo vmstate_info_timer; >>>> extern const VMStateInfo vmstate_info_buffer; >>>> extern const VMStateInfo vmstate_info_unused_buffer; >>>> extern const VMStateInfo vmstate_info_bitmap; >>>> +extern const VMStateInfo vmstate_info_qtailq; >>>> >>>> #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0) >>>> #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0) >>>> @@ -662,6 +663,25 @@ extern const VMStateInfo vmstate_info_bitmap; >>>> .offset = offsetof(_state, _field), \ >>>> } >>>> >>>> +/* For QTAILQ that need customized handling. >>>> + * Target QTAILQ needs be properly initialized. >>>> + * _type: type of QTAILQ element >>>> + * _next: name of QTAILQ entry field in QTAILQ element >>>> + * _vmsd: VMSD for QTAILQ element >>>> + * size: size of QTAILQ element >>>> + * start: offset of QTAILQ entry in QTAILQ element >>>> + */ >>>> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \ >>>> +{ \ >>>> + .name = (stringify(_field)), \ >>>> + .version_id = (_version), \ >>>> + .vmsd = &(_vmsd), \ >>>> + .size = sizeof(_type), \ >>>> + .info = &vmstate_info_qtailq, \ >>>> + .offset = offsetof(_state, _field), \ >>>> + .start = offsetof(_type, _next), \ >>>> +} >>>> + >>>> /* _f : field name >>>> _f_n : num of elements field_name >>>> _n : num of elements >>>> diff --git a/include/qemu/queue.h b/include/qemu/queue.h >>>> index 342073f..16af127 100644 >>>> --- a/include/qemu/queue.h >>>> +++ b/include/qemu/queue.h >>>> @@ -438,4 +438,65 @@ struct { \ >>>> #define QTAILQ_PREV(elm, headname, field) \ >>>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) >>>> >>>> +#define field_at_offset(base, offset, type) \ >>>> + ((type) (((char *) (base)) + (offset))) >>>> + >>>> +typedef struct DUMB_Q_ENTRY DUMB_Q_ENTRY; >>>> +typedef struct DUMB_Q DUMB_Q; >>>> + >>>> +struct DUMB_Q_ENTRY { >>>> + QTAILQ_ENTRY(DUMB_Q_ENTRY) next; >>>> +}; >>>> + >>>> +struct DUMB_Q { >>>> + QTAILQ_HEAD(DUMB_Q_HEAD, DUMB_Q_ENTRY) head; >>>> +}; >>> >>> OK, good we've got rid of the hard coded offsets; thanks! >>> >>>> +#define dumb_q ((DUMB_Q *) 0) >>>> +#define dumb_qh ((typeof(dumb_q->head) *) 0) >>>> +#define dumb_qe ((DUMB_Q_ENTRY *) 0) >>> >>> Note that 'dumb' and 'dummy' are completely different words; >>> this is a dummy not a dumb. >>> >> OK. >>>> +/* >>>> + * Offsets of layout of a tail queue head. >>>> + */ >>>> +#define QTAILQ_FIRST_OFFSET ((size_t) &(QTAILQ_FIRST(dumb_qh))) >>> >>> Isn't that just offsetof(dumb_qh, tqh_first) ? >> Yes. But I don't want to depend on the inside of the type if it is >> possible. QTAILQ_FIRST is a defined access macro. >> >>> >>>> +#define QTAILQ_LAST_OFFSET (offsetof(typeof(dumb_q->head), tqh_last)) >>> >>> Similarly isnt't that just offsetof(DUMB_Q_HEAD, tqh_last) ? >>> >> Similarly, DUMB_Q_HEAD may not be a type name in the future. >> >> Thanks, >> Jianjun >>>> +/* >>>> + * Raw access of elements of a tail queue >>>> + */ >>>> +#define QTAILQ_RAW_FIRST(head) \ >>>> + (*field_at_offset(head, QTAILQ_FIRST_OFFSET, void **)) >>>> +#define QTAILQ_RAW_LAST(head) \ >>>> + (*field_at_offset(head, QTAILQ_LAST_OFFSET, void ***)) >>>> + >>>> +/* >>>> + * Offsets of layout of a tail queue element. >>>> + */ >>>> +#define QTAILQ_NEXT_OFFSET ((size_t) (&QTAILQ_NEXT(dumb_qe, next))) >>>> +#define QTAILQ_PREV_OFFSET (offsetof(typeof(dumb_qe->next), tqe_prev)) >>> >>> Similar comments to the pair above. >>> >>> Dave >>> P.S. I'm out next week, so please someone else jump in. >>> > [..] > > I think this got overly complicated. Here is a little patch on > top of your stuff which gets rid of 15 lines and IMHO simplifies > things quite a bit. What do you think? > > It is based on/inspired by Dave's proposal with the dummy stuff. > > Did not address the typos though. It is unlikely the definition of QTAILQ will change, so hard-coded value probably was the most simple. Now that we want to address the potential changes, I think my code will deal with future changes better. It uses the proper q head and entry definition, and uses the provided interface whenever possible. I will fix the typo though. Thanks, Jianjun > > Cheers, > Halil > > ----------------- 8< ---------------------------- > From: Halil Pasic <pasic@linux.vnet.ibm.com> > Date: Mon, 31 Oct 2016 13:53:31 +0100 > Subject: [PATCH] fixup: simplify QTAILQ raw access macros > > Intended to be fixed up to [PATCH v9 2/3] migration: migrate QTAILQ. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > include/qemu/queue.h | 33 +++++++++------------------------ > 1 files changed, 9 insertions(+), 24 deletions(-) > > diff --git a/include/qemu/queue.h b/include/qemu/queue.h > index 16af127..d67cb4e 100644 > --- a/include/qemu/queue.h > +++ b/include/qemu/queue.h > @@ -441,33 +441,17 @@ struct { \ > #define field_at_offset(base, offset, type) \ > ((type) (((char *) (base)) + (offset))) > > -typedef struct DUMB_Q_ENTRY DUMB_Q_ENTRY; > -typedef struct DUMB_Q DUMB_Q; > - > -struct DUMB_Q_ENTRY { > - QTAILQ_ENTRY(DUMB_Q_ENTRY) next; > -}; > - > -struct DUMB_Q { > - QTAILQ_HEAD(DUMB_Q_HEAD, DUMB_Q_ENTRY) head; > -}; > - > -#define dumb_q ((DUMB_Q *) 0) > -#define dumb_qh ((typeof(dumb_q->head) *) 0) > -#define dumb_qe ((DUMB_Q_ENTRY *) 0) > - > /* > - * Offsets of layout of a tail queue head. > + * Raw access helpers > */ > -#define QTAILQ_FIRST_OFFSET ((size_t) &(QTAILQ_FIRST(dumb_qh))) > -#define QTAILQ_LAST_OFFSET (offsetof(typeof(dumb_q->head), tqh_last)) > +typedef Q_TAILQ_HEAD(QTAILQRawHead, void,) QTAILQRawHead; > +typedef Q_TAILQ_ENTRY(void,) QTAILQRawEntry; > + > /* > * Raw access of elements of a tail queue > */ > -#define QTAILQ_RAW_FIRST(head) \ > - (*field_at_offset(head, QTAILQ_FIRST_OFFSET, void **)) > -#define QTAILQ_RAW_LAST(head) \ > - (*field_at_offset(head, QTAILQ_LAST_OFFSET, void ***)) > +#define QTAILQ_RAW_FIRST(head) (((QTAILQRawHead *)(head))->tqh_first) > +#define QTAILQ_RAW_LAST(head) (((QTAILQRawHead *)(head))->tqh_last) > > /* > * Offsets of layout of a tail queue element. > @@ -479,9 +463,10 @@ struct DUMB_Q { > * Raw access of elements of a tail entry > */ > #define QTAILQ_RAW_NEXT(elm, entry) \ > - (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) > + ((field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_next)) > #define QTAILQ_RAW_PREV(elm, entry) \ > - (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) > + (field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_prev) > + > /* > * Tail queue tranversal using pointer arithmetic. > */ >
On 10/31/2016 06:10 PM, Jianjun Duan wrote: >> I think this got overly complicated. Here is a little patch on >> > top of your stuff which gets rid of 15 lines and IMHO simplifies >> > things quite a bit. What do you think? >> > >> > It is based on/inspired by Dave's proposal with the dummy stuff. >> > >> > Did not address the typos though. > It is unlikely the definition of QTAILQ will change, so hard-coded > value probably was the most simple. Now that we want to address the > potential changes, I think my code will deal with future changes better. Please elaborate in what way does your version deal better with future changes? IMHO the version with my patch applied covers all the corners your original code covers but it is without any doubt more concise and in my opinion also more straight-forward. Halil
On 10/31/2016 10:21 AM, Halil Pasic wrote: > > > On 10/31/2016 06:10 PM, Jianjun Duan wrote: >>> I think this got overly complicated. Here is a little patch on >>>> top of your stuff which gets rid of 15 lines and IMHO simplifies >>>> things quite a bit. What do you think? >>>> >>>> It is based on/inspired by Dave's proposal with the dummy stuff. >>>> >>>> Did not address the typos though. >> It is unlikely the definition of QTAILQ will change, so hard-coded >> value probably was the most simple. Now that we want to address the >> potential changes, I think my code will deal with future changes better. > > Please elaborate in what way does your version deal better with future > changes? IMHO the version with my patch applied covers all the corners > your original code covers but it is without any doubt more concise and > in my opinion also more straight-forward. I don't use the internals of head and entry structures if there are access macro around. Also I didn't use pointer type cast. I don't think pointer cast is more straightforward. Thanks, Jianjun > > Halil >
On 10/31/2016 06:32 PM, Jianjun Duan wrote: >>>> I think this got overly complicated. Here is a little patch on >>>>> >>>> top of your stuff which gets rid of 15 lines and IMHO simplifies >>>>> >>>> things quite a bit. What do you think? >>>>> >>>> >>>>> >>>> It is based on/inspired by Dave's proposal with the dummy stuff. >>>>> >>>> >>>>> >>>> Did not address the typos though. >>> >> It is unlikely the definition of QTAILQ will change, so hard-coded >>> >> value probably was the most simple. Now that we want to address the >>> >> potential changes, I think my code will deal with future changes better. >> > >> > Please elaborate in what way does your version deal better with future >> > changes? IMHO the version with my patch applied covers all the corners >> > your original code covers but it is without any doubt more concise and >> > in my opinion also more straight-forward. > I don't use the internals of head and entry structures if there are > access macro around. Also I didn't use pointer type cast. I don't think > pointer cast is more straightforward. Internals is quite relative since the stuff is part of the header and there is no indication that it's not part of the API. About pointer type casts I do not understand what you mean by that: my understanding is that we both do casts to a pointer type. And I do think it is more straightforward than defining a macro for the offset for each link-pointer and then basically play the field_at_offset game twice: once to pin point the struct holding all the links, and once again to pinpoint the individual links. As you see I do not believe you that your version is more robust. If you want to convince me _give me a remotely reasonable patch_ which beaks my version but not yours. The straight-forwardness is probably a matter of taste, and that's why I used IMHO. I'm not sure if I understood you correctly, do you think that the current version of the code is more readable that what I have proposed? If you do, I doubt there is a rational way to settle this taste dispute. If the majority of the people says your approach is more straight-forward then I apologize. Cheers, Halil
On 10/31/2016 11:01 AM, Halil Pasic wrote: > > > On 10/31/2016 06:32 PM, Jianjun Duan wrote: >>>>> I think this got overly complicated. Here is a little patch on >>>>>>>>>> top of your stuff which gets rid of 15 lines and IMHO simplifies >>>>>>>>>> things quite a bit. What do you think? >>>>>>>>>> >>>>>>>>>> It is based on/inspired by Dave's proposal with the dummy stuff. >>>>>>>>>> >>>>>>>>>> Did not address the typos though. >>>>>> It is unlikely the definition of QTAILQ will change, so hard-coded >>>>>> value probably was the most simple. Now that we want to address the >>>>>> potential changes, I think my code will deal with future changes better. >>>> >>>> Please elaborate in what way does your version deal better with future >>>> changes? IMHO the version with my patch applied covers all the corners >>>> your original code covers but it is without any doubt more concise and >>>> in my opinion also more straight-forward. >> I don't use the internals of head and entry structures if there are >> access macro around. Also I didn't use pointer type cast. I don't think >> pointer cast is more straightforward. > > > Internals is quite relative since the stuff is part of the header > and there is no indication that it's not part of the API. About > pointer type casts I do not understand what you mean by that: > my understanding is that we both do casts to a pointer type. And > I do think it is more straightforward than defining a macro for the > offset for each link-pointer and then basically play the field_at_offset > game twice: once to pin point the struct holding all the links, and > once again to pinpoint the individual links. > It is more concise but not more straightforward for me. > As you see I do not believe you that your version is more robust. > If you want to convince me _give me a remotely reasonable patch_ which > beaks my version but not yours. > > The straight-forwardness is probably a matter of taste, and that's > why I used IMHO. I'm not sure if I understood you correctly, do > you think that the current version of the code is more readable > that what I have proposed? If you do, I doubt there is a rational > way to settle this taste dispute. If the majority of the people > says your approach is more straight-forward then I apologize. > I agree it is a matter of taste. With current code both versions will not break. But if somebody changes the name of the var you directly accessed, your code will break. Of course I don't expect somebody to just do that. It is indeed in the header file and we expect people to change all at once if any change is done. I used this argument for hard encoding and it is not taken. Thanks, Jianjun > Cheers, > Halil >
On 10/31/2016 07:25 PM, Jianjun Duan wrote: > > > On 10/31/2016 11:01 AM, Halil Pasic wrote: >> >> >> On 10/31/2016 06:32 PM, Jianjun Duan wrote: >>>>>> I think this got overly complicated. Here is a little patch on >>>>>>>>>>> top of your stuff which gets rid of 15 lines and IMHO simplifies >>>>>>>>>>> things quite a bit. What do you think? >>>>>>>>>>> >>>>>>>>>>> It is based on/inspired by Dave's proposal with the dummy stuff. >>>>>>>>>>> >>>>>>>>>>> Did not address the typos though. >>>>>>> It is unlikely the definition of QTAILQ will change, so hard-coded >>>>>>> value probably was the most simple. Now that we want to address the >>>>>>> potential changes, I think my code will deal with future changes better. >>>>> >>>>> Please elaborate in what way does your version deal better with future >>>>> changes? IMHO the version with my patch applied covers all the corners >>>>> your original code covers but it is without any doubt more concise and >>>>> in my opinion also more straight-forward. >>> I don't use the internals of head and entry structures if there are >>> access macro around. Also I didn't use pointer type cast. I don't think >>> pointer cast is more straightforward. >> >> >> Internals is quite relative since the stuff is part of the header >> and there is no indication that it's not part of the API. About >> pointer type casts I do not understand what you mean by that: >> my understanding is that we both do casts to a pointer type. And >> I do think it is more straightforward than defining a macro for the >> offset for each link-pointer and then basically play the field_at_offset >> game twice: once to pin point the struct holding all the links, and >> once again to pinpoint the individual links. >> > It is more concise but not more straightforward for me. > >> As you see I do not believe you that your version is more robust. >> If you want to convince me _give me a remotely reasonable patch_ which >> beaks my version but not yours. >> >> The straight-forwardness is probably a matter of taste, and that's >> why I used IMHO. I'm not sure if I understood you correctly, do >> you think that the current version of the code is more readable >> that what I have proposed? If you do, I doubt there is a rational >> way to settle this taste dispute. If the majority of the people >> says your approach is more straight-forward then I apologize. >> > > I agree it is a matter of taste. With current code both versions will > not break. But if somebody changes the name of the var you directly > accessed, your code will break. Of course I don't expect somebody to > just do that. > > It is indeed in the header file and we expect people to change all at > once if any change is done. I used this argument for hard encoding and > it is not taken. There is a huge difference. If somebody changes the name of tqe_next for instance it will break compile-time and it will also break the normal (non-raw) macros. Same goes for the case if someone wanted to remove the Q_TAILQ_HEAD macro for example (or am I missing something). In your case there was no compile time error but a possible run-time memory corruption instead. If you see no difference between these I really do not know how to do constructive discussion. Cheers, Halil > > Thanks, > Jianjun > > Cheers, >> Halil >>
On 31/10/2016 14:10, Halil Pasic wrote: > I think this got overly complicated. I agree. :) > Here is a little patch on > top of your stuff which gets rid of 15 lines and IMHO simplifies > things quite a bit. What do you think? > > It is based on/inspired by Dave's proposal with the dummy stuff. > > Did not address the typos though. I still prefer my field_at_offset proposal, but I'm obviously biased. Squashing your changes on top of 2/3 is fine too. But this change makes little sense: > */ > #define QTAILQ_RAW_NEXT(elm, entry) \ > - (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) > + ((field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_next)) > #define QTAILQ_RAW_PREV(elm, entry) \ > - (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) > + (field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_prev) > + field_at_offset seems to be out of place. Paolo
On 11/01/2016 04:02 PM, Paolo Bonzini wrote: > > > On 31/10/2016 14:10, Halil Pasic wrote: >> I think this got overly complicated. > > I agree. :) > >> Here is a little patch on >> top of your stuff which gets rid of 15 lines and IMHO simplifies >> things quite a bit. What do you think? >> >> It is based on/inspired by Dave's proposal with the dummy stuff. >> >> Did not address the typos though. > > I still prefer my field_at_offset proposal, but I'm obviously biased. :) have searched it *field_at_offset(void **, elm, (entry) + QTAILQ_NEXT_OFFSET) = NULL; *field_at_offset(void ***, elm, (entry) + QTAILQ_PREV_OFFSET) = *field_at_offset(void ***, head, QTAILQ_LAST_OFFSET); **field_at_offset(void ***, head, QTAILQ_LAST_OFFSET) = (elm); *field_at_offset(void ***, head, QTAILQ_LAST_OFFSET) = field_at_offset(void **, elm, (entry) + QTAILQ_NEXT_OFFSET); That was for the insert of course. A lot of * for my taste and we still have the QTAILQ_NEXT_OFFSET and QTAILQ_PREV_OFFSET macros which are supposed to capture what QTAILQRawEntry does (namely where is the next and the prev pointer within the entry -- not the element). Obviously I'm biased too ;) > Squashing your changes on top of 2/3 is fine too. But this change makes > little sense: > >> */ >> #define QTAILQ_RAW_NEXT(elm, entry) \ >> - (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) >> + ((field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_next)) >> #define QTAILQ_RAW_PREV(elm, entry) \ >> - (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) >> + (field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_prev) >> + > > field_at_offset seems to be out of place. Me scratching head. This is the place where we pinpoint the qtailq entry which is at offset entry within the element and reinterpret the pointer as a pointer to QTAILQRawEntry. In the end we end up with a void pointer to the next or prev element. I think #define QTAILQ_RAW_NEXT(elm, entry_offset) \ ((field_at_offset(elm, entry_offset, QTAILQRawEntry *)->tqe_next) would be more readable though. Could you explain whats wrong with that? Thank you very much for commenting! Halil > > Paolo >
diff --git a/include/qemu/queue.h b/include/qemu/queue.h index 16af127..d67cb4e 100644 --- a/include/qemu/queue.h +++ b/include/qemu/queue.h @@ -441,33 +441,17 @@ struct { \ #define field_at_offset(base, offset, type) \ ((type) (((char *) (base)) + (offset))) -typedef struct DUMB_Q_ENTRY DUMB_Q_ENTRY; -typedef struct DUMB_Q DUMB_Q; - -struct DUMB_Q_ENTRY { - QTAILQ_ENTRY(DUMB_Q_ENTRY) next; -}; - -struct DUMB_Q { - QTAILQ_HEAD(DUMB_Q_HEAD, DUMB_Q_ENTRY) head; -}; - -#define dumb_q ((DUMB_Q *) 0) -#define dumb_qh ((typeof(dumb_q->head) *) 0) -#define dumb_qe ((DUMB_Q_ENTRY *) 0) - /* - * Offsets of layout of a tail queue head. + * Raw access helpers */ -#define QTAILQ_FIRST_OFFSET ((size_t) &(QTAILQ_FIRST(dumb_qh))) -#define QTAILQ_LAST_OFFSET (offsetof(typeof(dumb_q->head), tqh_last)) +typedef Q_TAILQ_HEAD(QTAILQRawHead, void,) QTAILQRawHead; +typedef Q_TAILQ_ENTRY(void,) QTAILQRawEntry; + /* * Raw access of elements of a tail queue */ -#define QTAILQ_RAW_FIRST(head) \ - (*field_at_offset(head, QTAILQ_FIRST_OFFSET, void **)) -#define QTAILQ_RAW_LAST(head) \ - (*field_at_offset(head, QTAILQ_LAST_OFFSET, void ***)) +#define QTAILQ_RAW_FIRST(head) (((QTAILQRawHead *)(head))->tqh_first) +#define QTAILQ_RAW_LAST(head) (((QTAILQRawHead *)(head))->tqh_last) /* * Offsets of layout of a tail queue element. @@ -479,9 +463,10 @@ struct DUMB_Q { * Raw access of elements of a tail entry */ #define QTAILQ_RAW_NEXT(elm, entry) \ - (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) + ((field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_next)) #define QTAILQ_RAW_PREV(elm, entry) \ - (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) + (field_at_offset(elm, entry, QTAILQRawEntry *)->tqe_prev) + /* * Tail queue tranversal using pointer arithmetic. */