| Message ID | b8f87009-ba1a-dfaf-e130-03c5500f76c4@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | evtchn: recent XSAs follow-on | expand |
Hi Jan, On 28/09/2020 11:56, Jan Beulich wrote: > There's no global lock around the updating of this global piece of data. > Make use of cmpxchg() to avoid two entities racing with their updates. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > TBD: Initially I used cmpxchgptr() here, until realizing Arm doesn't > have it. It's slightly more type-safe than cmpxchg() (requiring > all arguments to actually be pointers), so I now wonder whether Arm > should gain it (perhaps simply by moving the x86 implementation to > xen/lib.h), or whether we should purge it from x86 as being > pointless. I would be happy with an implementation of cmpxchgptr() in xen/lib.h. > > --- 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]; This doesn't seem directly related to the changes below. Can you explain it in the commit message? > > /* Default notification action: wake up from wait_on_xen_event_channel(). */ > static void default_xen_notification_fn(struct vcpu *v, unsigned int port) > @@ -81,7 +82,7 @@ static uint8_t get_xen_consumer(xen_even > for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ ) > { > if ( xen_consumers[i] == NULL ) > - xen_consumers[i] = fn; > + (void)cmpxchg(&xen_consumers[i], NULL, fn); This wants an explanation in the code. Maybe: "As there is no global lock, the cmpxchg() will prevent race between two callers." > if ( xen_consumers[i] == fn ) > break; > } > Cheers,
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 28 September 2020 11:57 > To: xen-devel@lists.xenproject.org > Cc: 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: [PATCH 02/12] evtchn: avoid race in get_xen_consumer() > > There's no global lock around the updating of this global piece of data. > Make use of cmpxchg() to avoid two entities racing with their updates. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > TBD: Initially I used cmpxchgptr() here, until realizing Arm doesn't > have it. It's slightly more type-safe than cmpxchg() (requiring > all arguments to actually be pointers), so I now wonder whether Arm > should gain it (perhaps simply by moving the x86 implementation to > xen/lib.h), or whether we should purge it from x86 as being > pointless. > > --- 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) > @@ -81,7 +82,7 @@ static uint8_t get_xen_consumer(xen_even > for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ ) > { > if ( xen_consumers[i] == NULL ) > - xen_consumers[i] = fn; > + (void)cmpxchg(&xen_consumers[i], NULL, fn); > if ( xen_consumers[i] == fn ) Why not use the return from cmpxchg() to determine success and break out of the loop rather than re-accessing the global array? Paul > break; > } >
On 29.09.2020 17:44, Paul Durrant wrote: >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich >> Sent: 28 September 2020 11:57 >> >> @@ -81,7 +82,7 @@ static uint8_t get_xen_consumer(xen_even >> for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ ) >> { >> if ( xen_consumers[i] == NULL ) >> - xen_consumers[i] = fn; >> + (void)cmpxchg(&xen_consumers[i], NULL, fn); >> if ( xen_consumers[i] == fn ) > > Why not use the return from cmpxchg() to determine success and break > out of the loop rather than re-accessing the global array? That's an option, in which case I wouldn't be sure anymore whether adding __read_mostly to the definition of xen_consumers[] is appropriate. This way, otoh, the (LOCKed on x86) write isn't even attempted when the slot already holds non-NULL. Jan
--- 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) @@ -81,7 +82,7 @@ static uint8_t get_xen_consumer(xen_even for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ ) { if ( xen_consumers[i] == NULL ) - xen_consumers[i] = fn; + (void)cmpxchg(&xen_consumers[i], NULL, fn); if ( xen_consumers[i] == fn ) break; }
There's no global lock around the updating of this global piece of data. Make use of cmpxchg() to avoid two entities racing with their updates. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- TBD: Initially I used cmpxchgptr() here, until realizing Arm doesn't have it. It's slightly more type-safe than cmpxchg() (requiring all arguments to actually be pointers), so I now wonder whether Arm should gain it (perhaps simply by moving the x86 implementation to xen/lib.h), or whether we should purge it from x86 as being pointless.