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, dvra...@cantab.net wrote:
On 28/05/15 15:55, Jan Beulich wrote:
On 26.05.15 at 20:00, david.vra...@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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
On 02.06.15 at 15:22, david.vra...@citrix.com wrote: On 29/05/15 09:31, Jan Beulich wrote: On 28.05.15 at 18:09, dvra...@cantab.net wrote: On 28/05/15 15:55, Jan Beulich wrote: On 26.05.15 at 20:00, david.vra...@citrix.com 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@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
On 28.05.15 at 18:09, dvra...@cantab.net wrote:
On 28/05/15 15:55, Jan Beulich wrote:
On 26.05.15 at 20:00, david.vra...@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.
-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@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
On 26.05.15 at 20:00, david.vra...@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 ...
@@ -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? 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)?
-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.
@@ -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?
@@ -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.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
On 28/05/15 15:55, Jan Beulich wrote:
On 26.05.15 at 20:00, david.vra...@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);
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
