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

[Halil Pasic]

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.

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.
  */
-- 
1.7.1