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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.12.14 at 15:29, <chegger@xxxxxxxxx> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -227,23 +227,23 @@ double_gt_lock(struct grant_table *lgt, struct 
> grant_table *rgt)
>  {
>      if ( lgt < rgt )
>      {
> -        spin_lock(&lgt->lock);
> -        spin_lock(&rgt->lock);
> +        spin_lock(&lgt->maptrack_lock);
> +        spin_lock(&rgt->maptrack_lock);
>      }
>      else
>      {
>          if ( lgt != rgt )
> -            spin_lock(&rgt->lock);
> -        spin_lock(&lgt->lock);
> +            spin_lock(&rgt->maptrack_lock);
> +        spin_lock(&lgt->maptrack_lock);
>      }
>  }

I think this function needs to be renamed with this change, to clarify
that it's not the general grant table locks which get acquired here.
Same for the unlock then of course. That'll also make stand out the
places where the function is used.

> @@ -891,28 +891,28 @@ __gnttab_unmap_common(
>      ld = current->domain;
>      lgt = ld->grant_table;
>  
> +    read_lock(&lgt->lock);
>      op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
>  
>      if ( unlikely(op->handle >= lgt->maptrack_limit) )
>      {
> +        read_unlock(&lgt->lock);
>          gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle);
>          op->status = GNTST_bad_handle;
>          return;
>      }
>  
> +    read_unlock(&lgt->lock);

The added locking here seems pointless, or else there would be a
bug in the existing code (which should then be fixed by a separate,
much smaller change).

>      op->map = &maptrack_entry(lgt, op->handle);
> -    spin_lock(&lgt->lock);
>  
>      if ( unlikely(!op->map->flags) )
>      {
> -        spin_unlock(&lgt->lock);
>          gdprintk(XENLOG_INFO, "Zero flags for handle (%d).\n", op->handle);
>          op->status = GNTST_bad_handle;
>          return;
>      }
>  
>      dom = op->map->domid;
> -    spin_unlock(&lgt->lock);

And the removed locking here needs special mentioning in the commit
message, explaining why no locking is needed.

> @@ -944,6 +944,7 @@ __gnttab_unmap_common(
>      }
>  
>      op->rd = rd;
> +    read_lock(&rgt->lock);
>      act = &active_entry(rgt, op->map->ref);
>  
>      if ( op->frame == 0 )

The nesting of the two locks should be mentioned in the doc change.

> @@ -1004,6 +1005,7 @@ __gnttab_unmap_common(
>  
>   unmap_out:
>      double_gt_unlock(lgt, rgt);
> +    read_unlock(&rgt->lock);

And I'd prefer unlocking sequence to be the inverse of the locking
one, just to avoid confusion about their nesting.

> @@ -1284,10 +1286,13 @@ gnttab_unpopulate_status_frames(struct domain *d, 
> struct grant_table *gt)
>      gt->nr_status_frames = 0;
>  }
>  
> +/* Grow the grant table. The caller must hold the grant table's
> + * write lock before calling this function.
> + */

Coding style.

> @@ -1965,17 +1970,21 @@ __acquire_grant_for_copy(
>                  PIN_FAIL(unlock_out_clear, GNTST_general_error,
>                           "transitive grant referenced bad domain %d\n",
>                           trans_domid);
> -            spin_unlock(&rgt->lock);
> +
> +            /* __acquire_grant_for_copy() could take the read lock on
> +             * the right table (if rd == td), so we have to drop the
> +             * lock here and reacquire */

Coding style again.

> @@ -2900,7 +2910,7 @@ gnttab_release_mappings(
>          }
>  
>          rgt = rd->grant_table;
> -        spin_lock(&rgt->lock);
> +        read_lock(&rgt->lock);
>  
>          act = &active_entry(rgt, ref);
>          sha = shared_entry_header(rgt, ref);
> @@ -2960,7 +2970,7 @@ gnttab_release_mappings(
>          if ( act->pin == 0 )
>              gnttab_clear_flag(_GTF_reading, status);
>  
> -        spin_unlock(&rgt->lock);
> +        read_unlock(&rgt->lock);

The maptrack entries are being accessed between these two - don't
you need both locks here?

> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -82,8 +82,11 @@ struct grant_table {
>      struct grant_mapping **maptrack;
>      unsigned int          maptrack_head;
>      unsigned int          maptrack_limit;
> -    /* Lock protecting updates to active and shared grant tables. */
> -    spinlock_t            lock;
> +    /* Lock protecting the maptrack page list, head, and limit */
> +    spinlock_t            maptrack_lock;
> +    /* Lock protecting updates to grant table state (version, active
> +       entry list, etc.) */
> +    rwlock_t              lock;
>      /* The defined versions are 1 and 2.  Set to 0 if we don't know
>         what version to use yet. */
>      unsigned              gt_version;

Coding style again - the malformed existing comment is no reason to
add another malformed one.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel