Re: [PATCH v2] xen/grant-table: Simplify the update to the per-vCPU maptrack freelist

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.06.2021 12:08, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Since XSA-228 (commit 02cbeeb62075 "gnttab: split maptrack lock
> to make it fulfill its purpose again"), v->maptrack_head,
> v->maptrack_tail and the content of the freelist are accessed with
> the lock v->maptrack_freelist_lock held.
> 
> Therefore it is not necessary to update the fields using cmpxchg()
> and also read them atomically.
> 
> Note that there are two cases where v->maptrack_tail is accessed without
> the lock. They both happen in get_maptrack_handle() when initializing
> the free list of the current vCPU. Therefore there is no possible race.
> 
> The code is now reworked to remove any use of cmpxch() and read_atomic()
> when accessing the fields v->maptrack_{head, tail} as wel as the
> freelist.
> 
> Take the opportunity to add a comment on top of the lock definition
> and explain what it protects.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one nit:

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -255,7 +255,13 @@ struct vcpu
>      /* VCPU paused by system controller. */
>      int              controller_pause_count;
>  
> -    /* Grant table map tracking. */
> +    /*
> +     * Grant table map tracking. The lock maptrack_freelist_lock
> +     * protects to:

I don't think you want "to" here.

Jan

> +     *  - The entries in the freelist
> +     *  - maptrack_head
> +     *  - maptrack_tail
> +     */
>      spinlock_t       maptrack_freelist_lock;
>      unsigned int     maptrack_head;
>      unsigned int     maptrack_tail;
>