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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 28/05/2021 14:29, Jan Beulich wrote:
> On 26.05.2021 17:21, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > Since XSA-288 (commit 02cbeeb62075 "gnttab: split maptrack lock to make
> XSA-228 I suppose?
Yes, I will update in the next version.

> > it fulfill its purpose again"), v->maptrack_head and v->maptrack_tail
> > are with the lock v->maptrack_freelist_lock held.
> Nit: missing "accessed" or alike?
I have added "accessed".

> > Therefore it is not necessary to update the fields using cmpxchg()
> > and also read them atomically.
> Ah yes, very good observation. Should have noticed this back at the
> time, for an immediate follow-up change.
>
> > Note that there are two cases where v->maptrack_tail is accessed without
> > the lock. They both happen _get_maptrack_handle() when the current vCPU
> > list is empty. Therefore there is no possible race.
> I think you mean the other function here, without a leading underscore
> in its name. 
Hmmm... Yes. I will update it.

> And if you want to explain the absence of a race, wouldn't
> you then better also mention that the list can get initially filled
> only on the local vCPU?
Sure. I will reword it.

> > I am not sure whether we should try to protect the remaining unprotected
> > access with the lock or maybe add a comment?
> As per above I don't view adding locking as sensible. If you feel like
> adding a helpful comment, perhaps. I will admit that it took me more
> than just a moment to recall that "local vCPU only" argument.
I will try to come up with an helpful comment.

> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -543,34 +543,26 @@ double_gt_unlock(struct grant_table *lgt, struct 
> > grant_table *rgt)
> >   static inline grant_handle_t
> >   _get_maptrack_handle(struct grant_table *t, struct vcpu *v)
> >   {
> > -    unsigned int head, next, prev_head;
> > +    unsigned int head, next;
> >   
> >       spin_lock(&v->maptrack_freelist_lock);
> >   
> > -    do {
> > -        /* No maptrack pages allocated for this VCPU yet? */
> > -        head = read_atomic(&v->maptrack_head);
> > -        if ( unlikely(head == MAPTRACK_TAIL) )
> > -        {
> > -            spin_unlock(&v->maptrack_freelist_lock);
> > -            return INVALID_MAPTRACK_HANDLE;
> Where did this and ...
>
> > -        }
> > -
> > -        /*
> > -         * Always keep one entry in the free list to make it easier to
> > -         * add free entries to the tail.
> > -         */
> > -        next = read_atomic(&maptrack_entry(t, head).ref);
> > -        if ( unlikely(next == MAPTRACK_TAIL) )
> > -        {
> > -            spin_unlock(&v->maptrack_freelist_lock);
> > -            return INVALID_MAPTRACK_HANDLE;
> ... this use of INVALID_MAPTRACK_HANDLE go? It is at present merely
> coincidence that INVALID_MAPTRACK_HANDLE == MAPTRACK_TAIL. If you
> want to fold them, you will need to do so properly (by eliminating
> one of the two constants). But I think they're separate on purpose.
Hmmm... Somehow I thought one was an alias to the other. But they are 
clearly not. I will update it on the next version.

> > -        }
> > +    /* No maptrack pages allocated for this VCPU yet? */
> > +    head = v->maptrack_head;
> > +    if ( unlikely(head == MAPTRACK_TAIL) )
> > +        goto out;
> >   
> > -        prev_head = head;
> > -        head = cmpxchg(&v->maptrack_head, prev_head, next);
> > -    } while ( head != prev_head );
> > +    /*
> > +     * Always keep one entry in the free list to make it easier to
> > +     * add free entries to the tail.
> > +     */
> > +    next = read_atomic(&maptrack_entry(t, head).ref);
> Since the lock protects the entire free list, why do you need to
> keep read_atomic() here?
Because I wasn't sure whether dropping {write, read}_atomic() when 
accessing the freelist would be fine.
Anyway, I can drop it in the next version.

> > +    if ( unlikely(next == MAPTRACK_TAIL) )
> > +        head = MAPTRACK_TAIL;
> > +    else
> > +        v->maptrack_head = next;
> >   
> > +out:
> Please indent labels by at least one blank, to avoid issues with
> diff's -p option. In fact if you didn't introduce a goto here in
> the first place, there'd be less code churn overall, as you'd
> need to alter the indentation of fewer lines.
I will have a look.

> > @@ -623,7 +615,7 @@ put_maptrack_handle(
> >   {
> >       struct domain *currd = current->domain;
> >       struct vcpu *v;
> > -    unsigned int prev_tail, cur_tail;
> > +    unsigned int prev_tail;
> >   
> >       /* 1. Set entry to be a tail. */
> >       maptrack_entry(t, handle).ref = MAPTRACK_TAIL;
> > @@ -633,11 +625,8 @@ put_maptrack_handle(
> >   
> >       spin_lock(&v->maptrack_freelist_lock);
> >   
> > -    cur_tail = read_atomic(&v->maptrack_tail);
> > -    do {
> > -        prev_tail = cur_tail;
> > -        cur_tail = cmpxchg(&v->maptrack_tail, prev_tail, handle);
> > -    } while ( cur_tail != prev_tail );
> > +    prev_tail = v->maptrack_tail;
> > +    v->maptrack_tail = handle;
> >   
> >       /* 3. Update the old tail entry to point to the new entry. */
> >       write_atomic(&maptrack_entry(t, prev_tail).ref, handle);
> Since the write_atomic() here can then also be converted, may I
> ask that you then rename the local variable to just "tail" as
> well?
Sure.

> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -255,7 +255,10 @@ struct vcpu
> >       /* VCPU paused by system controller. */
> >       int              controller_pause_count;
> >   
> > -    /* Grant table map tracking. */
> > +    /*
> > +     * Grant table map tracking. The lock maptrack_freelist_lock protect
> Nit: protects
I will fix it.

> > +     * the access to maptrack_head and maptrack_tail.
> > +     */
> I'm inclined to suggest this doesn't need spelling out, considering ...
>
> >       spinlock_t       maptrack_freelist_lock;
> >       unsigned int     maptrack_head;
> >       unsigned int     maptrack_tail;
> ... both the name of the lock and its placement next to the two
> fields it protects. Also as per the docs change of the XSA-228 change,
> the lock protects more than just these two fields, so the comment may
> be misleading the way you have it now.
So I think it would be good to document above the lock what it actually 
protects. I agree this is fairly clear that it protect maptrack_{head, 
tail} but this wasn't very clear to me that it would also protect the 
content of the freelist (so read_atomic()/write_atomic() could be dropped).
I will try to come up with a better comment.

Cheers,

--
Julien Grall