[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
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: >>>> @@ -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(>->lock); >>>> + write_lock(>->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(>->lock); >>>> + write_unlock(>->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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |