Re: [Qemu-devel] [Qemu-ppc] [QEMU PATCH v9 2/3] migration: migrate QTAILQ

[Jianjun Duan]

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 (address@hidden) 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 <address@hidden>
>>>> ---
>>>>  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 <address@hidden>
> 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 <address@hidden>
> ---
>  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.
>   */
>