Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer()

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Wei Liu <wl@xen.org>, Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer()
Date: Thu, 22 Oct 2020 09:33:27 +0200	[thread overview]
Message-ID: <3fd4c197-617e-dd48-6781-9ff0b1a82bf8@suse.com> (raw)
In-Reply-To: <20201021154650.zz77ircyuedr7gpm@Air-de-Roger>

On 21.10.2020 17:46, Roger Pau Monné wrote:
> On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
>> There's no global lock around the updating of this global piece of data.
>> Make use of cmpxchgptr() to avoid two entities racing with their
>> updates.
>>
>> While touching the functionality, mark xen_consumers[] read-mostly (or
>> else the if() condition could use the result of cmpxchgptr(), writing to
>> the slot unconditionally).
> 
> I'm not sure I get this, likely related to the comment I have below.

This is about the alternative case of invoking cmpxchgptr()
without the if() around it. On x86 this would mean always
writing the field, even if the designated value is already in
place.

>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -57,7 +57,8 @@
>>   * with a pointer, we stash them dynamically in a small lookup array which
>>   * can be indexed by a small integer.
>>   */
>> -static xen_event_channel_notification_t xen_consumers[NR_XEN_CONSUMERS];
>> +static xen_event_channel_notification_t __read_mostly
>> +    xen_consumers[NR_XEN_CONSUMERS];
>>  
>>  /* Default notification action: wake up from wait_on_xen_event_channel(). */
>>  static void default_xen_notification_fn(struct vcpu *v, unsigned int port)
>> @@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even
>>  
>>      for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
>>      {
>> +        /* Use cmpxchgptr() in lieu of a global lock. */
>>          if ( xen_consumers[i] == NULL )
>> -            xen_consumers[i] = fn;
>> +            cmpxchgptr(&xen_consumers[i], NULL, fn);
>>          if ( xen_consumers[i] == fn )
>>              break;
> 
> I think you could join it as:
> 
> if ( !xen_consumers[i] &&
>      !cmpxchgptr(&xen_consumers[i], NULL, fn) )
>     break;
> 
> As cmpxchgptr will return the previous value of &xen_consumers[i]?

But then you also have to check whether the returned value is
fn (or retain the 2nd if()).

Jan