Re: [PATCH 02/12] evtchn: avoid race in get_xen_consumer()

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxx>
> ---
> 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,

--
Julien Grall