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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 09/06/2021 12:20, Jan Beulich wrote:
> 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>
Thanks. I committed with...


> 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.
... this addressed and ...

> Jan
>
> > +     *  - The entries in the freelist
... "The" removed.

> > +     *  - maptrack_head
> > +     *  - maptrack_tail
> > +     */
> >       spinlock_t       maptrack_freelist_lock;
> >       unsigned int     maptrack_head;
> >       unsigned int     maptrack_tail;
Cheers,

--
Julien Grall