WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 1/3] xen/event: Add reference counting to event c

To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 1/3] xen/event: Add reference counting to event channels
From: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Date: Wed, 26 Oct 2011 13:28:03 -0400
Cc: "Keir \(Xen.org\)" <keir@xxxxxxx>, "jeremy@xxxxxxxx" <jeremy@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>, "konrad.wilk@xxxxxxxxxx" <konrad.wilk@xxxxxxxxxx>
Delivery-date: Wed, 26 Oct 2011 10:28:27 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1319647887.9436.55.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: National Security Agency
References: <1316207684-19860-1-git-send-email-dgdegra@xxxxxxxxxxxxx> <1319644068-25814-1-git-send-email-dgdegra@xxxxxxxxxxxxx> <1319644068-25814-2-git-send-email-dgdegra@xxxxxxxxxxxxx> <1319647887.9436.55.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20110927 Thunderbird/7.0
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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel