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

[Jianjun Duan]

On 10/15/2016 05:48 AM, Halil Pasic wrote:
> 
> 
> 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;
> 

Now I see. I thought Dave was using QTAILQDummy as an example.

Thanks,
Jianjun
> Cheers,
> Halil
>