Re: [Xen-devel] [PATCHv7 2/3] gnttab: refactor locking for scalability

[Tim Deegan <tim@xxxxxxx>]

At 14:28 +0100 on 30 Apr (1430404124), David Vrabel wrote:
> +    /* 
> +     * N.B.: while taking the left side maptrack spinlock prevents
> +     * any mapping changes, the right side active entries could be
> +     * changing while we are counting.

IIRC, the lX/rX variables are local/remote rather than left/right. 

> @@ -639,7 +660,7 @@ __gnttab_map_grant_ref(
>      if ( unlikely(op->ref >= nr_grant_entries(rgt)))
>          PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref (%d).\n", op->ref);
>  
> -    act = &active_entry(rgt, op->ref);
> +    act = active_entry_acquire(rgt, op->ref);
>      shah = shared_entry_header(rgt, op->ref);
>      if (rgt->gt_version == 1) {
>          sha1 = &shared_entry_v1(rgt, op->ref);
> @@ -656,7 +677,7 @@ __gnttab_map_grant_ref(
>           ((act->domid != ld->domain_id) ||
>            (act->pin & 0x80808080U) != 0 ||
>            (act->is_sub_page)) )
> -        PIN_FAIL(unlock_out, GNTST_general_error,
> +        PIN_FAIL(act_release_out, GNTST_general_error,
>                   "Bad domain (%d != %d), or risk of counter overflow %08x, 
> or subpage %d\n",
>                   act->domid, ld->domain_id, act->pin, act->is_sub_page);
>  
> @@ -667,7 +688,7 @@ __gnttab_map_grant_ref(
>          if ( (rc = _set_status(rgt->gt_version, ld->domain_id,
>                                 op->flags & GNTMAP_readonly,
>                                 1, shah, act, status) ) != GNTST_okay )
> -             goto unlock_out;
> +            goto act_release_out;
>  
>          if ( !act->pin )
>          {
> @@ -698,7 +719,6 @@ __gnttab_map_grant_ref(
>              GNTPIN_hstr_inc : GNTPIN_hstw_inc;
>  
>      frame = act->frame;
> -    act_pin = act->pin;
>  
>      cache_flags = (shah->flags & (GTF_PAT | GTF_PWT | GTF_PCD) );
>  
> @@ -776,8 +796,6 @@ __gnttab_map_grant_ref(
>          goto undo_out;
>      }
>  
> -    double_maptrack_lock(lgt, rgt);
> -

OK, so here we hold rd->grant_table->lock and act->lock (which is in
the rd table) and are going to acquire lgt->maptrack_lock in mapcount().

That means we can't ever have a path holding domA's maptrack lock that
acquires domB's gt lock or one of domB's act->locks.  I think that's
OK because after this patch the only paths left that hold the maptrack
lock can't acquire anything except an act->lock of the same domain.
Do I understand that correctly?

Also: because mapcount() itself doesn't take any act->lock, there's no
path that holds two act->locks at the same time?

I think it would be nice to expand the locking-discipline comment to
make those restrictions clear. 

Cheers,

Tim.

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