From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42317) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abmJY-0001Jh-0b for qemu-devel@nongnu.org; Fri, 04 Mar 2016 04:46:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abmJU-0006Ev-17 for qemu-devel@nongnu.org; Fri, 04 Mar 2016 04:46:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36393) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abmJT-0006Ef-SD for qemu-devel@nongnu.org; Fri, 04 Mar 2016 04:46:23 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 4D31CC00B8EB for ; Fri, 4 Mar 2016 09:46:23 +0000 (UTC) Date: Fri, 4 Mar 2016 15:16:13 +0530 From: Amit Shah Message-ID: <20160304094613.GJ15443@grmbl.mre> References: <1457010971-24771-1-git-send-email-lprosek@redhat.com> <20160304062723.GH15443@grmbl.mre> <56D95173.9050108@redhat.com> <56D9551D.3070607@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56D9551D.3070607@redhat.com> Subject: Re: [Qemu-devel] [PATCH] rng: switch request queue to QSIMPLEQ List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: pagupta , Ladi Prosek , qemu-devel@nongnu.org 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 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