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. I assume the reads of xen_consumers are not using ACCESS_ONCE or read_atomic because we rely on the compiler performing such reads as single instructions? Roger.