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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 22, 2020 at 10:56:15AM +0200, Jan Beulich wrote:
> On 22.10.2020 10:29, Roger Pau Monné wrote:
> > On Thu, Oct 22, 2020 at 10:15:45AM +0200, Jan Beulich wrote:
> >> On 22.10.2020 10:11, Roger Pau Monné wrote:
> >>> On Thu, Oct 22, 2020 at 09:33:27AM +0200, Jan Beulich wrote:
> >>>> On 21.10.2020 17:46, Roger Pau Monné wrote:
> >>>>> On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
> >>>>>> @@ -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()).
> >>>
> >>> __cmpxchg comment says that success of the operation is indicated when
> >>> the returned value equals the old value, so it's my understanding that
> >>> cmpxchgptr returning NULL would mean the exchange has succeed and that
> >>> xen_consumers[i] == fn?
> >>
> >> Correct. But if xen_consumers[i] == fn before the call, the return
> >> value will be fn. The cmpxchg() wasn't "successful" in this case
> >> (it didn't update anything), but the state of the array slot is what
> >> we want.
> > 
> > Oh, I get it now. You don't want the same fn populating more than one
> > slot.
> 
> FAOD it's not just "want", it's a strict requirement.

I wouldn't mind having a comment to that effect in the function, but I
won't insist.

Thanks, Roger.