From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 1/2] xen/arm: vgic: Keep track of vIRQ used by a domain Date: Thu, 19 Feb 2015 17:19:18 +0000 Message-ID: <54E61B16.5090202@linaro.org> References: <1422546694-22797-1-git-send-email-julien.grall@linaro.org> <1422546694-22797-2-git-send-email-julien.grall@linaro.org> <1422889793.4801.13.camel@citrix.com> <54E4CDA3.1020506@linaro.org> <1424366153.30924.141.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YOUlQ-0005IH-SC for xen-devel@lists.xenproject.org; Thu, 19 Feb 2015 17:19:48 +0000 Received: by mail-wi0-f175.google.com with SMTP id r20so49752109wiv.2 for ; Thu, 19 Feb 2015 09:19:45 -0800 (PST) In-Reply-To: <1424366153.30924.141.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, tim@xen.org, parth.dixit@linaro.org, christoffer.dall@linaro.org List-Id: xen-devel@lists.xenproject.org On 19/02/15 17:15, Ian Campbell wrote: > On Wed, 2015-02-18 at 17:36 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 02/02/2015 15:09, Ian Campbell wrote: >>> On Thu, 2015-01-29 at 15:51 +0000, Julien Grall wrote: >>>> - Move the retry after looking for first/end. I keep the goto >>>> rather than a loop because it's more clear that we retry because >>>> we were unable to set the bit >>> >>> Then I think a "do {} while (!successfully allocated)" is what is >>> wanted, maybe with a comment. >> >> I though about it and I find the do {} while more difficult to read than >> the goto in this specific case. >> >> I find more explicit with the goto that we will unlikely retry. >> y. Adding a comment in the code doesn't really help. >> >> the do {} while version would look like: >> >> do { >> virq = find_next_zero_bit(d->arch.vgic.allocated_irqs, end, first); >> if ( virq >= end ) >> return -1; >> >> ret = test_and_set_bit(virq, d->arch.vgic.allocated_irqs); >> /* There is no spinlock to protect allocated_irqs, therefore >> * test_and_set_bit may unlikely fail. If so retry it. >> */ >> } while ( unlikely(ret) ) > > I think > } while ( unlikely(test_and_set_bit(virq, d->arch.vgic.allocated_irqs)) ) > would be clear enough, with the comment you have being moved before the > whole loop. > > I'm not sure the unlikely is really useful in the context of a do{}while > either, I doubt it will cause any different heuristic to be used which > isn't already triggered by the use of do-while rather than while-do. > Especially since this one isn't performance critical I'd just leave it > out. > > i.e. > > /* > * There is no spinlock to protect allocated_irqs, therefore > * test_and_set_bit may fail. If so retry it. > */ > do { > virq = find_next_zero_bit(d->arch.vgic.allocated_irqs, end, first); > if ( virq >= end ) > return -1; > } while ( test_and_set_bit(virq, d->arch.vgic.allocated_irqs) ) > Ok, I will resend the patch series with this change. Regards, -- Julien Grall