Re: [Xen-devel] [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.05.15 at 20:00, <david.vrabel@xxxxxxxxxx> wrote:
> @@ -254,23 +254,23 @@ double_gt_lock(struct grant_table *lgt, struct 
> grant_table *rgt)
>  {
>      if ( lgt < rgt )
>      {
> -        spin_lock(&lgt->lock);
> -        spin_lock(&rgt->lock);
> +        write_lock(&lgt->lock);
> +        write_lock(&rgt->lock);
>      }
>      else
>      {
>          if ( lgt != rgt )
> -            spin_lock(&rgt->lock);
> -        spin_lock(&lgt->lock);
> +            write_lock(&rgt->lock);
> +        write_lock(&lgt->lock);
>      }
>  }

So I looked at the two uses of double_gt_lock() again: in both cases
only a read lock is needed on rgt (which is also the natural thing to
expect: we aren't supposed to modify the remote domain's grant
table in any way here). Albeit that's contradicting ...

> @@ -568,10 +568,10 @@ static void mapcount(
>      *wrc = *rdc = 0;
>  
>      /*
> -     * Must have the remote domain's grant table lock while counting
> -     * its active entries.
> +     * Must have the remote domain's grant table write lock while
> +     * counting its active entries.
>       */
> -    ASSERT(spin_is_locked(&rd->grant_table->lock));
> +    ASSERT(rw_is_write_locked(&rd->grant_table->lock));

... this: Why would we need to hold the write lock here? We're
not changing anything in rd->grant_table.

> @@ -837,12 +838,22 @@ __gnttab_map_grant_ref(
>  
>      TRACE_1D(TRC_MEM_PAGE_GRANT_MAP, op->dom);
>  
> +    /*
> +     * All maptrack entry users check mt->flags first before using the
> +     * other fields so just ensure the flags field is stored last.
> +     *
> +     * However, if gnttab_need_iommu_mapping() then this would race
> +     * with a concurrent mapcount() call (on an unmap, for example)
> +     * and a lock is required.
> +     */
>      mt = &maptrack_entry(lgt, handle);
>      mt->domid = op->dom;
>      mt->ref   = op->ref;
> -    mt->flags = op->flags;
> +    wmb();
> +    write_atomic(&mt->flags, op->flags);

Doesn't that then also require the use of read_atomic() and rmb()
on the reading side? Further, why are only races against mapcount()
a problem, but not such against __gnttab_unmap_common() as a
whole? I.e. what's the locking around the op->map->flags and
op->map->domid accesses below good for? Or, alternatively, isn't
this an indication of a problem with the previous patch splitting off
the maptrack lock (effectively leaving some map track entry
accesses without any guard)?

> -    double_gt_unlock(lgt, rgt);
> +    if ( gnttab_need_iommu_mapping(ld) )
> +        double_gt_unlock(lgt, rgt);

I think you shouldn't rely on gnttab_need_iommu_mapping() to
produce the same result upon this and the earlier invocation, i.e.
you ought to latch the first call's result into a local variable.

> @@ -1787,7 +1805,7 @@ gnttab_transfer(
>          TRACE_1D(TRC_MEM_PAGE_GRANT_TRANSFER, e->domain_id);
>  
>          /* Tell the guest about its new page frame. */
> -        spin_lock(&e->grant_table->lock);
> +        read_lock(&e->grant_table->lock);
>  
>          if ( e->grant_table->gt_version == 1 )
>          {
> @@ -1805,7 +1823,7 @@ gnttab_transfer(
>          shared_entry_header(e->grant_table, gop.ref)->flags |=
>              GTF_transfer_completed;
>  
> -        spin_unlock(&e->grant_table->lock);
> +        read_unlock(&e->grant_table->lock);

I'm not sure a read lock suffices here: Consider the effect of two
parallel invocations of this code with identical gop.ref, but different
gop.mfn. Or wait, gnttab_prepare_for_transfer() makes sure only
one gets here, unless the granting domain fiddles with the shared
entry. But it feels wrong nevertheless, considering that we alter
state of e here. Or should this perhaps lock the matching active
entry, even if it doesn't touch it?

> @@ -2645,7 +2663,7 @@ __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t 
> ref_b)
>      struct active_grant_entry *act_b = NULL;
>      s16 rc = GNTST_okay;
>  
> -    spin_lock(&gt->lock);
> +    write_lock(&gt->lock);
>  
>      /* Bounds check on the grant refs */
>      if ( unlikely(ref_a >= nr_grant_entries(d->grant_table)))
> @@ -2689,7 +2707,7 @@ out:
>          active_entry_release(act_b);
>      if ( act_a != NULL )
>          active_entry_release(act_a);
> -    spin_unlock(&gt->lock);
> +    write_unlock(&gt->lock);

It would seem to me that these could be dropped when the per-active-
entry locks get introduced.

Jan

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