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

[Julien Grall <julien.grall@linaro.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