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

[David Vrabel <dvrabel@xxxxxxxxxx>]

On 28/05/15 15:55, Jan Beulich wrote:
>>>> 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 ...

See comment below.

>> @@ -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?

rmb() isn't required because the read_lock() is already a full barrier.
 Will need to add some read_atomic()s though.

> 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)?

The double_gt_lock() takes both write locks, thus does not race with
__gnttab_unmap_common clearing the flag on the maptrack entry which is
done while holding the remote read lock.

The maptrack lock only protects the free list, not the maptrack entries
themselves.  The docs may not correctly say this though -- i'll double
check.

>> -    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.

Um. Okay. But if this changes during the life time of a domain it's
going to either leak iommu mappings or fail to create them which sounds
rather broken to me.

>> @@ -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?

Locking the active entry looks sensible to me.

>> @@ -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.

I'm not sure what you want dropped here?  We require the write lock here
because we're taking two active entries at once.

David

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