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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.05.15 at 18:09, <dvrabel@xxxxxxxxxx> wrote:
> 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);
>> 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 entries are items of the local domain, i.e. the state
of the remote domain's lock shouldn't matter there at all. Anything
else would be extremely counterintuitive and hence prone to future
breakage. With that the earlier two comments (above) remain un-
addressed too.

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

Hotplugging a passed through device into a domain can change the
outcome of iommu_needed(). I wouldn't be surprised if the
combination of mapped grants and passed through devices didn't
correctly deal with all (corner) cases, as it seems likely to me that
such domains aren't of wide spread use (yet). In particular I don't
see arch_iommu_populate_page_table() take care of active grant
mappings at all.

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

Ah, right. But couldn't the write lock then be dropped as soon as the
two active entries got locked?

Jan

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