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

[Jianjun Duan]

On 10/21/2016 11:51 AM, Dr. David Alan Gilbert wrote:
> * Jianjun Duan (address@hidden) wrote:
>>
>>
>> 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.
> 
> If you have a new version with either that or Paolo's suggestion,
> it would be good; I'd like to see this set go in because I've
> now got a small pile of patches built on top of it using
> my WITH_TMP macro.
> 

I am working on it.

Thanks,
Jianjun
> Dave
> 
>>
>> Thanks,
>> Jianjun
>>> Cheers,
>>> Halil
>>>
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>