Re: [Xen-devel] [PATCHv6 2/5] gnttab: introduce per-active entry locks

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.15 at 18:00, <david.vrabel@xxxxxxxxxx> wrote:
> @@ -548,11 +542,14 @@ static void mapcount(
>  
>      for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
>      {
> +        struct active_grant_entry *act;

const please (wherever possible, to document the intent to not
modify).

> -        if ( active_entry(rd->grant_table, map->ref).frame == mfn )
> +        act = &_active_entry(rd->grant_table, map->ref);
> +        if ( act->frame == mfn )

Or, as suggested before, avoid the intermediate variable in the
first place.

> @@ -698,12 +694,9 @@ __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) );
>  
> -    spin_unlock(&rgt->lock);
> -
>      /* pg may be set, with a refcount included, from __get_paged_frame */
>      if ( !pg )
>      {
> @@ -778,8 +771,6 @@ __gnttab_map_grant_ref(
>          goto undo_out;
>      }
>  
> -    double_gt_lock(lgt, rgt);
> -
>      if ( gnttab_need_iommu_mapping(ld) )
>      {
>          unsigned int wrc, rdc;

I continue to think that using lgt unlocked here is wrong (i.e. while
the next patch may fix this, it still represents intermediate breakage).
At the very least this should be called out explicitly in the commit
message, making it clear that this patch shouldn't be applied without
the next one immediately following. But maybe the better approach
would be to avoid the breakage in the first place by keeping 
double_gt_lock() in place until the next patch, moving a spin_unlock()
right ahead of it to document the intentions, or even keeping that
one in place too until the next patch.

> @@ -939,18 +929,19 @@ __gnttab_unmap_common(
>      TRACE_1D(TRC_MEM_PAGE_GRANT_UNMAP, dom);
>  
>      rgt = rd->grant_table;
> -    double_gt_lock(lgt, rgt);
> +
> +    spin_lock(&rgt->lock);
> +    act = active_entry_acquire(rgt, op->map->ref);
>  
>      op->flags = op->map->flags;
>      if ( unlikely(!op->flags) || unlikely(op->map->domid != dom) )
>      {
>          gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle);
>          rc = GNTST_bad_handle;
> -        goto unmap_out;
> +        goto act_release_out;
>      }
>  
>      op->rd = rd;
> -    act = &active_entry(rgt, op->map->ref);

Why does this get moved up? "act" isn't need anywhere above.

> @@ -1290,13 +1287,17 @@ 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.
> + */
>  int
>  gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)

The "write" in the comment appears to be stale.

> @@ -1972,6 +1976,13 @@ __acquire_grant_for_copy(
>                  PIN_FAIL(unlock_out_clear, GNTST_general_error,
>                           "transitive grant referenced bad domain %d\n",
>                           trans_domid);
> +
> +            /* 
> +             * __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
> +             */
> +            active_entry_release(act);

Same for the "read" here.

> @@ -2939,7 +2973,7 @@ grant_table_create(
>      struct domain *d)
>  {
>      struct grant_table *t;
> -    int                 i;
> +    int                 i, j;

Could you take the opportunity to make these unsigned? And perhaps
also drop the odd looking blank padding?

> @@ -3130,7 +3167,9 @@ grant_table_destroy(
>  
>      if ( t == NULL )
>          return;
> -    
> +
> +    spin_lock(&t->lock);
> +
>      for ( i = 0; i < nr_grant_frames(t); i++ )
>          free_xenheap_page(t->shared_raw[i]);
>      xfree(t->shared_raw);
> @@ -3147,6 +3186,8 @@ grant_table_destroy(
>          free_xenheap_page(t->status[i]);
>      xfree(t->status);
>  
> +    spin_unlock(&t->lock);

Iirc I asked this before - do you really think these are needed? Because
again - if they are, that would seem to be a separate bug fix.

Jan

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