From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v3 1/2] xen/arm: vgic: Keep track of vIRQ
 used by a domain
Date: Thu, 19 Feb 2015 17:15:53 +0000
Message-ID: <1424366153.30924.141.camel@citrix.com>
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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Ian.Campbell@citrix.com>) id 1YOUhi-0004g9-AV
	for xen-devel@lists.xenproject.org; Thu, 19 Feb 2015 17:15:58 +0000
In-Reply-To: <54E4CDA3.1020506@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@linaro.org>
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 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) )

Ian.