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 (du...@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 <du...@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 <pa...@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 <pa...@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.
> */
>
