On 26.05.2021 17:21, Julien Grall wrote:
> From: Julien Grall <[email protected]>
>
> Since XSA-288 (commit 02cbeeb62075 "gnttab: split maptrack lock to make
XSA-228 I suppose?
> 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?
> 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. 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?
> 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.
> --- 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.
> - }
> + /* 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?
> + 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.
> @@ -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?
> --- 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
> + * 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.
Jan
