[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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?

> 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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.