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

[Halil Pasic]

On 10/14/2016 07:18 PM, Jianjun Duan wrote:
>>>>> +/*
>>>>> >>>> + * Offsets of layout of a tail queue head.
>>>>> >>>> + */
>>>>> >>>> +#define QTAILQ_FIRST_OFFSET 0
>>>>> >>>> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
>>>>> >>>> +
>>>>> >>>> +/*
>>>>> >>>> + * Offsets of layout of a tail queue element.
>>>>> >>>> + */
>>>>> >>>> +#define QTAILQ_NEXT_OFFSET 0
>>>>> >>>> +#define QTAILQ_PREV_OFFSET (sizeof(void *))
>>>>> >>>> +
>>>>> >>>> +/*
>>>>> >>>> + * Tail queue tranversal using pointer arithmetic.
>>>>> >>>> + */
>>>>> >>>> +#define QTAILQ_RAW_FOREACH(elm, head, entry)                        
>>>>> >>>>            \
>>>>> >>>> +        for ((elm) = *((void **) ((char *) (head) + 
>>>>> >>>> QTAILQ_FIRST_OFFSET));     \
>>>>> >>>> +             (elm);                                                 
>>>>> >>>>            \
>>>>> >>>> +             (elm) =                                                
>>>>> >>>>            \
>>>>> >>>> +                 *((void **) ((char *) (elm) + (entry) + 
>>>>> >>>> QTAILQ_NEXT_OFFSET)))
>>>>> >>>> +/*
>>>>> >>>> + * Tail queue insertion using pointer arithmetic.
>>>>> >>>> + */
>>>>> >>>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do {               
>>>>> >>>>            \
>>>>> >>>> +        *((void **) ((char *) (elm) + (entry) + 
>>>>> >>>> QTAILQ_NEXT_OFFSET)) = NULL;   \
>>>>> >>>> +        *((void **) ((char *) (elm) + (entry) + 
>>>>> >>>> QTAILQ_PREV_OFFSET)) =         \
>>>>> >>>> +            *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET));    
>>>>> >>>>            \
>>>>> >>>> +        **((void ***)((char *) (head) + QTAILQ_LAST_OFFSET)) = 
>>>>> >>>> (elm);          \
>>>>> >>>> +        *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) =       
>>>>> >>>>            \
>>>>> >>>> +            (void *) ((char *) (elm) + (entry) + 
>>>>> >>>> QTAILQ_NEXT_OFFSET);          \
>>>>> >>>> +} while (/*CONSTCOND*/0)
>>>> >>>
>>>> >>> I wonder if there's a simpler way to do this; I'm not sure this works, 
>>>> >>> but something like:
>>>> >>>
>>>> >>> struct QTAILQDummy {
>>>> >>>     char dummy;
>>>> >>> };
>>>> >>>
>>>> >>> QTAILQ_HEAD(QTAILQRawHead, struct QTAILQDummy)
>>>> >>> typedef QTAILQ_ENTRY(struct QTAILQDummy) QTAILQRawEntry;
>>>> >>>
>>>> >>> #define QTAILQ_RAW_FOREACH(elm, head, entry)                           
>>>> >>>         \
>>>> >>>         for ((elm) = ((struct QTAILQRawHead *)head)->tqh_first)        
>>>> >>>         \
>>>> >>>              (elm);                                                    
>>>> >>>         \
>>>> >>>              (elm) =                                                   
>>>> >>>         \
>>>> >>>              (elm) = ((QTAILQRawEntry *)((char *) (elm) + 
>>>> >>> (entry)))->tqh_next
>>>> >>>
>>>> >>> and then I think elm gets declared as a struct QTAILQDummy.
>>>> >>> But it does avoid those 
>>>> >>> FIRST_OFFSET/LAST_OFFSET/NEXT_OFFSET/PREV_OFFSET calculations.
>>>> >>>
>>>> >>> Would that work?
>>>> >>>
>>> >> It is intended for QTAILQ of any type. So type is not available.
>> > 
>> > I think it might be possible to do it generally.
>> > 
> If we have type, then we can use what is there already, and don't need a
> pointer arithmetic based approach. Inside put/get, we only get type
> layout info from vmsd, which is all about size and offset. This macro
> is used inside put/get, so I am not sure how we can directly use type
> here.
> 

Dave's approach seems perfectly sane to me. 

Jianjun have you actually tried to make it work before writing this?
Your argument does not work, because what you need from vmsd for
QTAILQ_RAW_FOREACH is only .start which corresponds to the entry
parameter of the macro. Dave still does the pointer arithmetic to
get a  pointer (char*) to the anonymous struct holding tqe_next
and tqe_prev. Now since no arithmetic is done wit tqe_next
and tqe_prev, only dereferencing, their pointer type does not matter
all that much so we can do the and follow the pointer. Same goes
for the head.

Actually the QTAILQDummy is not necessary in my opinion since we can
probably (did not try it out myself) do:

Q_TAILQ_HEAD(QTAILQRawHead, void,)
typedef Q_TAILQ_ENTRY(void,) QTAILQRawEntry;

Cheers,
Halil