>>> On 22.04.15 at 18:00, <david.vra...@citrix.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
