Re: [PATCH v3 1/2] xen/arm: vgic: Keep track of vIRQ used by a domain

[Ian Campbell <ian.campbell@citrix.com>]

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.