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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.04.15 at 15:28, <david.vrabel@xxxxxxxxxx> wrote:
>>> On 30.04.15 at 15:28, <david.vrabel@xxxxxxxxxx> wrote:
> This patch refactors grant table locking. It splits the previous
> single spinlock per grant table into multiple locks. The heavily
> modified components of the grant table (the maptrack state and the
> active entries) are now protected by their own spinlocks. The
> remaining elements of the grant table are read-mostly, so the main
> grant table lock is modified to be a rwlock to improve concurrency.

This looks to be (partly) stale.

> @@ -812,7 +829,7 @@ __gnttab_map_grant_ref(
>      mt->ref   = op->ref;
>      mt->flags = op->flags;
>  
> -    double_maptrack_unlock(lgt, rgt);
> +    active_entry_release(act);
>      read_unlock(&rgt->lock);
>  
>      op->dev_bus_addr = (u64)frame << PAGE_SHIFT;
> @@ -836,10 +853,6 @@ __gnttab_map_grant_ref(
>          put_page(pg);
>      }
>  
> -    read_lock(&rgt->lock);
> -
> -    act = &active_entry(rgt, op->ref);
> -
>      if ( op->flags & GNTMAP_device_map )
>          act->pin -= (op->flags & GNTMAP_readonly) ?
>              GNTPIN_devr_inc : GNTPIN_devw_inc;
> @@ -855,6 +868,9 @@ __gnttab_map_grant_ref(
>      if ( !act->pin )
>          gnttab_clear_flag(_GTF_reading, status);
>  
> + act_release_out:
> +    active_entry_release(act);
> +
>   unlock_out:
>      read_unlock(&rgt->lock);

This again looks like broken locking: You drop the lock twice, but the
intermediate re-acquire gets deleted. Additionally it looks like you let
go of "act" before being done with modifying it (act->pin adjustment
visible in context above) and reading it (act->pin check further
down), and then you release it another time here. Am I overlooking
something?

> @@ -2414,16 +2446,18 @@ 
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>      {
>          for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
>          {
> -            act = &active_entry(gt, i);
> +            act = active_entry_acquire(gt, i);
>              if ( act->pin != 0 )
>              {
>                  gdprintk(XENLOG_WARNING,
>                           "tried to change grant table version from %d to %d, 
> but some grant entries still in use\n",
>                           gt->gt_version,
>                           op.version);
> +                active_entry_release(act);
>                  res = -EBUSY;
>                  goto out_unlock;
>              }
> +            active_entry_release(act);
>          }

Is it really useful to acquire the entry here? I.e. wouldn't
read_atomic(act->pin) suffice?

Jan


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