Re: [PATCH 1/2] gnttab: Introduce rwlock to protect updates to grant table state

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/12/14 10:06, Christoph Egger wrote:
> Rename lock to maptrack_lock and use it to protect maptrack state.
>
> The new rwlock is used to prevent readers from accessing
> inconsistent grant table state such as current
> version, partially initialized active table pages,
> etc.

I would suggest phrasing this slightly differently, as it isn't a simple
rename.

What you are doing is taking the existing grant table lock, splitting it
in two to separate the grant locking from the maptrack locking, and
changing the resulting grant lock to be a rwlock.

With this noted, it would certainly be easier to review if this patch
was split in two; one patch to split the spinlocks and one patch to
change the grant lock from a spinlock to a rwlock.

> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 8fba923..018b5b6 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2501,8 +2510,19 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
>              return i;
>          if ( unlikely(__copy_from_guest(&op, uop, 1)) )
>              return -EFAULT;
> -        op.status = __gnttab_swap_grant_ref(op.ref_a, op.ref_b);
> -        if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
> +        if ( unlikely(op.ref_a == op.ref_b) )
> +        {
> +            /* noop, so avoid acquiring the same active entry
> +             * twice in __gnttab_swap_grant_ref(), which would
> +             * case a deadlock.
> +             */
> +            op.status = GNTST_okay;
> +        }
> +        else
> +        {
> +            op.status = __gnttab_swap_grant_ref(op.ref_a, op.ref_b);
> +        }
> +        if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )

I believe this comment only applies to the changes in active locking
introduced in the subsequent patch?

Either way, I think the a == b check should be in
__gnttab_swap_grant_ref() make any caller safe, not just the this one.

~Andrew