[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] rng: switch request queue to QSIMPLEQ
From: |
Amit Shah |
Subject: |
Re: [Qemu-devel] [PATCH] rng: switch request queue to QSIMPLEQ |
Date: |
Fri, 4 Mar 2016 15:16:13 +0530 |
On (Fri) 04 Mar 2016 [10:27:57], Paolo Bonzini wrote:
>
>
> On 04/03/2016 10:19, Ladi Prosek wrote:
> > On Fri, Mar 4, 2016 at 10:12 AM, Paolo Bonzini <address@hidden> wrote:
> >>
> >>
> >> On 04/03/2016 09:04, Ladi Prosek wrote:
> >>>>>>> + QSIMPLEQ_INIT(&s->requests);
> >>>>>>> }
> >>>>>
> >>>>> This init here isn't necessary, the accessors for the queue will take
> >>>>> care of this.
> >>> We are basically purging the queue here and we want to leave it in a
> >>> consistent state. Without the QSIMPLEQ_INIT the queue head would
> >>> become a pair of dangling pointers. Let me know if I misunderstood
> >>> your comment.
> >>
> >> It wouldn't, check out QSIMPLEQ_REMOVE_HEAD:
> >>
> >> #define QSIMPLEQ_REMOVE_HEAD(head, field) do {
> >> if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)
> >> (head)->sqh_last = &(head)->sqh_first;
> >> } while (/*CONSTCOND*/0)
> >>
> >> The queue would become { NULL, &s->requests.sqh_first }. So the
> >> QSIMPLEQ_INIT is indeed redundant.
> >
> > Right, but we're not running QSIMPLEQ_REMOVE_HEAD in this function. We
> > iterate the queue and free all elements without writing anything to
> > the head or to the next ptr. This is the only "write" we do in
> > rng_backend_free_requests.
>
> Ah, sorry, I was convinced that rng_backend_free_request did the remove,
> but now I remember checking it yesterday (after making the same
> reasoning as Amit) and indeed it doesn't. :)
>
> So the patch is okay. It's just a slightly unusual use of
> QSIMPLEQ_FOREACH_SAFE.
Yeah, it's confusing when common idioms don't apply.
Nice attention to detail, Ladi :-)
Amit
Re: [Qemu-devel] [PATCH] rng: switch request queue to QSIMPLEQ, Amit Shah, 2016/03/04