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

[David Vrabel <david.vrabel@citrix.com>]

On 29/05/15 09:31, Jan Beulich wrote:
>>>> On 28.05.15 at 18:09, <dvrabel@cantab.net> wrote:
>> On 28/05/15 15:55, Jan Beulich wrote:
>>>>>> On 26.05.15 at 20:00, <david.vrabel@citrix.com> 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.

mapcount() looks at the active entries of the remote domain and hence
these cannot change while counting, thus the write lock is required.

I cannot see how to do what you ask.

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

No, because at least the read lock is required for the subsequent
gt->gt_version check.  If the write lock was dropped and the read lock
acquired we would get the active entry and read lock ordering wrong.

David