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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Jianjun Duan (duanj@linux.vnet.ibm.com) 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.

Dave

> 
> Thanks,
> Jianjun
> > Cheers,
> > Halil
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK