Re: [Xen-devel] [PATCH v4 1/2] gnttab: Introduce rwlock to protect updates to grant table state

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.01.15 at 16:12, <chegger@xxxxxxxxx> wrote:
> @@ -899,26 +899,27 @@ __gnttab_unmap_common(
>  
>      op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
>  
> +    read_lock(&lgt->lock);
>      if ( unlikely(op->handle >= lgt->maptrack_limit) )
>      {
> +        read_unlock(&lgt->lock);
>          gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle);
>          op->status = GNTST_bad_handle;
>          return;
>      }
>  
>      op->map = &maptrack_entry(lgt, op->handle);
> -    spin_lock(&lgt->lock);

The extension of the locked region is still not being mentioned in the
description - as said for v3, if this is really needed, it should then be
fixed by a separate, much smaller change. (The main reason why
the first if() doesn't need to happen under lock is - afair - that
lgt->maptrack_limit can only ever increase.)

> @@ -939,7 +940,8 @@ __gnttab_unmap_common(
>      TRACE_1D(TRC_MEM_PAGE_GRANT_UNMAP, dom);
>  
>      rgt = rd->grant_table;
> -    double_gt_lock(lgt, rgt);
> +    read_lock(&rgt->lock);
> +    double_maptrack_lock(lgt, rgt);

Repeating what I said for v3: "The nesting of the two locks should
be mentioned in the doc change" (at the very least).

> @@ -1290,10 +1293,12 @@ 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. */

Above you said you fixed the coding style issues, but at least here
you didn't.

> @@ -1971,17 +1976,21 @@ __acquire_grant_for_copy(
>                  PIN_FAIL(unlock_out_clear, GNTST_general_error,
>                           "transitive grant referenced bad domain %d\n",
>                           trans_domid);
> -            spin_unlock(&rgt->lock);
> +
> +            /* __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 */

Nor here.

> @@ -2447,7 +2456,7 @@ __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t 
> ref_b)
>      struct active_grant_entry *act;
>      s16 rc = GNTST_okay;
>  
> -    spin_lock(&gt->lock);
> +    read_lock(&gt->lock);

Considering the purpose of this function, wouldn't this need to be a
write lock?

> @@ -2909,7 +2919,7 @@ gnttab_release_mappings(
>          }
>  
>          rgt = rd->grant_table;
> -        spin_lock(&rgt->lock);
> +        read_lock(&rgt->lock);
>  
>          act = &active_entry(rgt, ref);
>          sha = shared_entry_header(rgt, ref);
> @@ -2969,7 +2979,7 @@ gnttab_release_mappings(
>          if ( act->pin == 0 )
>              gnttab_clear_flag(_GTF_reading, status);
>  
> -        spin_unlock(&rgt->lock);
> +        read_unlock(&rgt->lock);

Repeating the question on v3: "The maptrack entries are being
accessed between these two - don't you need both locks here?"

Overall I find it quite unfriendly (wasting reviewing bandwidth) to
submit a new version with a meaningful number of comments on the
previous version un-addressed.

Jan


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