Re: [Xen-devel] [PATCHv6 3/5] gnttab: split grant table lock into table and maptrack locks

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.15 at 18:00, <david.vrabel@xxxxxxxxxx> wrote:
> Subsequent changes make both these locks uncontented.  Is this patch
> really necessary? -- dvrabel

Yeah, particularly if this ...

> @@ -540,6 +550,16 @@ static void mapcount(
>  
>      *wrc = *rdc = 0;
>  
> +    /* 
> +     * While taking the local maptrack spinlock prevents any mapping
> +     * changes, the remote active entries could be changing while we
> +     * are counting. The caller has to hold the grant table lock, or
> +     * some other mechanism should be used to prevent concurrent
> +     * changes during this operation.
> +     */
> +    ASSERT(spin_is_locked(&rd->grant_table->lock));
> +    spin_lock(&lgt->maptrack_lock);
> +
>      for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
>      {
>          struct active_grant_entry *act;
> @@ -552,6 +572,8 @@ static void mapcount(
>          if ( act->frame == mfn )
>              (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++;
>      }
> +
> +    spin_unlock(&lgt->maptrack_lock);
>  }

... is not resulting in contention anymore, I'd prefer avoiding a
change like this without measurable benefit. And when still an
issue, I'd wonder whether an rwlock wouldn't be the better
choice here.

> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -82,7 +82,12 @@ struct grant_table {
>      struct grant_mapping **maptrack;
>      unsigned int          maptrack_head;
>      unsigned int          maptrack_limit;
> -    /* Lock protecting updates to active and shared grant tables. */
> +    /* 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.)
> +     */
>      spinlock_t            lock;

If the patch still was to be applied, these two locks should be put
on separate cache lines, to avoid unnecessary bouncing.

Jan


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