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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.06.15 at 15:22, <david.vrabel@xxxxxxxxxx> wrote:
> On 29/05/15 09:31, Jan Beulich wrote:
>>>>> 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:
>>>>> @@ -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.

Hmm, okay, this then goes under the "write lock required for access
to multiple active entries" rule. Yet as said, this isn't what one would
expect as behavior, so I think along with the doc changes you do an
explanatory comment should be added to double_gt_lock().

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

Right. We could demote the write to a read lock, but probably that's
not worth it here.

Jan


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