Re: [PATCH 1/3] xen/event: Add reference counting to event channels

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 10/26/2011 12:51 PM, Ian Campbell wrote:
> On Wed, 2011-10-26 at 16:47 +0100, Daniel De Graaf wrote:
>> @@ -939,6 +943,10 @@ static void unbind_from_irq(unsigned int irq)
>>  {
>>         struct evtchn_close close;
>>         int evtchn = evtchn_from_irq(irq);
>> +       struct irq_info *info = irq_get_handler_data(irq);
>> +
>> +       if (atomic_read(&info->refcnt) > 0 && !atomic_dec_and_test(&info->refcnt))
>> +               return;
> 
> This isn't all that atomic any more...
> 
> evtchn_make_refcounted() doesn't seem to have any locking which would
> save you...
> 
> Perhaps you could always manipulate this flag under the mapping lock
> (which perhaps is normally taken around about the sort of place you'd
> want to do this anyway) and make it non-atomic?
> 
> Or maybe you could build something with cmpxchg?
> 
> Ian.
> 

It's atomic for the cases where it needs to be. There are two cases in which
unbind_from_irq can be called:

1. Negative refcnt (to be exact, == -1). This is an internal reference, and
unbind_from_irq is only called once as guaranteed by the caller.

2. Positive refcnt. Once refcnt has been changed to positive, it cannot be
changed back, so "atomic_read(&info->refcnt) > 0" will always be true, for
any number of parallel callers, assuming all callers had a reference to begin
with. In this case, only the atomic_dec_and_test needs atomicity to resolve
the race when parallel callers are running evtchn_put.

The conversion from -1 to positive refcnt is done during the creation of the
event channel, before multiple references to the event channel are allowed.
There is also no way to convert an event channel from positive to negative
(without destroying it completely) so a race from that conversion is also
not possible.

I considered using atomic_dec_if_positive here, but as the existing condition
has no races this is unnecessary.

Should this explanation be copied into a comment to avoid future confusion?

-- 
Daniel De Graaf
National Security Agency