| Message ID | 9ecafa4d-db5b-20a2-3a9d-6a6cda91252c@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | evtchn: recent XSAs follow-on | expand |
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. > The use of cmpxchgptr() here points out (by way of clang warning about > it) that its original use of const was slightly wrong. Adjust the > placement, or else undefined behavior of const qualifying a function > type will result. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- > v2: Use (and hence generalize) cmpxchgptr(). Add comment. Expand / > adjust description. > > --- 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]? Thanks, 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
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: > >> 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()). __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? Roger.
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. Jan
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.
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 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? Yes, as in so many other places in the code base. Jan
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). > > The use of cmpxchgptr() here points out (by way of clang warning about > it) that its original use of const was slightly wrong. Adjust the > placement, or else undefined behavior of const qualifying a function > type will result. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
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.
Hi Jan, On 20/10/2020 15:08, 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). > > The use of cmpxchgptr() here points out (by way of clang warning about > it) that its original use of const was slightly wrong. Adjust the > placement, or else undefined behavior of const qualifying a function > type will result. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
--- 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; } --- a/xen/include/asm-x86/system.h +++ b/xen/include/asm-x86/system.h @@ -148,13 +148,6 @@ static always_inline unsigned long cmpxc return prev; } -#define cmpxchgptr(ptr,o,n) ({ \ - const __typeof__(**(ptr)) *__o = (o); \ - __typeof__(**(ptr)) *__n = (n); \ - ((__typeof__(*(ptr)))__cmpxchg((ptr),(unsigned long)__o, \ - (unsigned long)__n,sizeof(*(ptr)))); \ -}) - /* * Undefined symbol to cause link failure if a wrong size is used with * arch_fetch_and_add(). --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -178,6 +178,17 @@ unsigned long long parse_size_and_unit(c uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c); +/* + * A slightly more typesafe variant of cmpxchg() when the entities dealt with + * are pointers. + */ +#define cmpxchgptr(ptr, o, n) ({ \ + __typeof__(**(ptr)) *const o_ = (o); \ + __typeof__(**(ptr)) *n_ = (n); \ + ((__typeof__(*(ptr)))__cmpxchg(ptr, (unsigned long)o_, \ + (unsigned long)n_, sizeof(*(ptr)))); \ +}) + #define TAINT_SYNC_CONSOLE (1u << 0) #define TAINT_MACHINE_CHECK (1u << 1) #define TAINT_ERROR_INJECT (1u << 2)
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). The use of cmpxchgptr() here points out (by way of clang warning about it) that its original use of const was slightly wrong. Adjust the placement, or else undefined behavior of const qualifying a function type will result. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Use (and hence generalize) cmpxchgptr(). Add comment. Expand / adjust description.