gnttab: Improve scaleability
v10 cover gnttab: Improve scaleability
The series builds on the original series by Matt Wilson and Christoph Egger from Amazon.
Performance results for aggregate intrahost network throughput (between 20 VM pairs, with 16 dom0 VCPUs) show substantial improvements.
Throughput/Gbit/s
Base 9.7
Split locks 25.8
Split locks 42.9
+ per-VCPU maptrack lists
v10:
* Reduce scope of act in grant_map_exists().
* Make unlock sequence in error paths consistent in
__acquire_grant_for_copy().
* gnt_unlock_out -> to gt_unlock_out in __acquire_grant_for_copy().
* In gnttab_map_grant_ref(), keep double lock around maptrack update if gnttab_need_iommu_mapping(). Use a wmb(), otherwise.
* Divide max_maptrack_frames evenly amongst the VCPUs.
* Increase default max_maptrack_frames to compensate.
v9:
* Refactor the locking patches into three commits:
1. Add per-active entry locking.
2. Add maptrack_lock.
3. Make the grant table lock a read-write lock.
* Keep the double lock around IOMMU updates, but adjust their scope to only the IOMMU updates.
v8:
* Misc locking fixes.
* Use read/write_atomic() in per-VCPU maptrack free list.
* Improve locking docs.
v7:
* Re-add grant table rwlock (locking was broken without it and the gains are minimal anyway).
* Remove unneeded locks from grant_table_destroy().
* Fix get_maptrack_handle() locking.
v6:
* Remove most uses of the grant table lock.
* Make the grant table lock a spin lock again (there were only writers left after the above)
* Add per-VCPU maptrack free lists.
v5:
* Addressed locking issue pointed out by Jan Beulich
* Fixed git rebase merge issue introduced in v4 (acquiring locking twice)
* Change for ()-loop in grant_map_exists
* Coding style fixes
v4:
* Coding style nits from Jan Beulich
* Fixup read locks pointed out by Jan Beulich
* renamed double_gt_(un)lock to double_maptrack_(un)lock per request from Jan Beulich
* Addressed ASSERT()'s from Jan Beulich
* Addressed locking issues in unmap_common pointed out by Jan Beulich
v3:
* Addressed gnttab_swap_grant_ref() comment from Andrew Cooper
v2:
* Add arm part per request from Julien Grall
David
PATCH v10 1/4 gnttab: per-active entry locking
Introduce a per-active entry spin lock to protect active entry state The grant table lock must be locked before acquiring (locking) an active entry.
This is a step in reducing contention on the grant table lock, but will only do so once the grant table lock is turned into a read-write lock.
Based on a patch originally by Matt Wilson <[email protected]>.
---
Changes in v10:
- Reduce scope of act in grant_map_exists().
- Make unlock sequence in error paths consistent in __acquire_grant_for_copy().
- gnt_unlock_out -> to gt_unlock_out in __acquire_grant_for_copy().
--- docs/misc/grant-tables.txt | 42 ++++++++++++- xen/common/grant_table.c | 146 ++++++++++++++++++++++++++++++++------------ 2 files changed, 147 insertions(+), 41 deletions(-)
docs/misc/grant-tables.txt +41 -1
@@ -63,6 +63,7 @@ is complete. act->domid : remote domain being granted rights act->frame : machine frame being granted act->pin : used to hold reference counts + act->lock : spinlock used to serialize access to active entry state Map tracking ~~~~~~~~~~~~
@@ -74,7 +75,46 @@ is complete. matching map track entry is then removed, as if unmap had been invoked. These are not used by the transfer mechanism. map->domid : owner of the mapped frame - map->ref_and_flags : grant reference, ro/rw, mapped for host or device access + map->ref : grant reference + map->flags : ro/rw, mapped for host or device access + +******************************************************************************** + Locking + ~~~~~~~ + Xen uses several locks to serialize access to the internal grant table state. + + grant_table->lock : lock used to prevent readers from accessing + inconsistent grant table state such as current + version, partially initialized active table pages, + etc. + active_grant_entry->lock : spinlock used to serialize modifications to + active entries + + The primary lock for the grant table is a spinlock. All functions + that access members of struct grant_table must acquire the lock + around critical sections. + + Active entries are obtained by calling active_entry_acquire(gt, ref). + This function returns a pointer to the active entry after locking its + spinlock. The caller must hold the grant table lock for the gt in + question before calling active_entry_acquire(). This is because the + grant table can be dynamically extended via gnttab_grow_table() while + a domain is running and must be fully initialized. Once all access to + the active entry is complete, release the lock by calling + active_entry_release(act). + + Summary of rules for locking: + active_entry_acquire() and active_entry_release() can only be + called when holding the relevant grant table's lock. I.e.: + spin_lock(>->lock); + act = active_entry_acquire(gt, ref); + ... + active_entry_release(act); + spin_unlock(>->lock); + + Active entries cannot be acquired while holding the maptrack lock. + Multiple active entries can be acquired while holding the grant table + lock. ********************************************************************************
xen/common/grant_table.c +106 -40
422 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Re: [PATCHv10 1/4] gnttab: per-active entry locking
Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
xen/common/grant_table.c +2
-1@@ -514,18 +534,19 @@ static int grant_map_exists(const struct domain *ld, nr_grant_entries(rgt));for ( ref = *ref_count; ref < max_iter; ref++ ) { - act = &active_entry (rgt, ref); +struct active_grant_entry *act; +bool_t exists;
xen/common/grant_table.c static int grant_map_exists(const struct domain *ld, · line 544
Quoted 10 lines
xen/common/grant_table.c +4
-4-if ( !act->pin ) -continue ; + act =active_entry_acquire (rgt, ref); -if ( act->domid != ld->domain_id ) -continue ; + exists = act->pin + && act->domid == ld->domain_id + && act->frame != mfn;
== (you're inverting the entire but split up original || expression, i.e. namely ...
xen/common/grant_table.c static int grant_map_exists(const struct domain *ld, · line 526
Quoted 2 lines
xen/common/grant_table.c +0
-2-if ( act->frame != mfn ) -continue ;
... also this one)
With that fixed,
PATCH v10 2/4 gnttab: introduce maptrack lock
In reply to David VrabelSplit grant table lock into two separate locks. One to protect maptrack state (maptrack_lock) and one for everything else (lock).
Based on a patch originally by Matt Wilson <[email protected]>.
--- docs/misc/grant-tables.txt | 9 +++++++++ xen/common/grant_table.c | 9 +++++---- xen/include/xen/grant_table.h | 2 ++ 3 files changed, 16 insertions(+), 4 deletions(-)
docs/misc/grant-tables.txt +9 -0
@@ -87,6 +87,7 @@ is complete. inconsistent grant table state such as current version, partially initialized active table pages, etc. + grant_table->maptrack_lock : spinlock used to protect the maptrack state active_grant_entry->lock : spinlock used to serialize modifications to active entries
@@ -94,6 +95,14 @@ is complete. that access members of struct grant_table must acquire the lock around critical sections. + The maptrack state is protected by its own spinlock. Any access (read + or write) of struct grant_table members that have a "maptrack_" + prefix must be made while holding the maptrack lock. The maptrack + state can be rapidly modified under some workloads, and the critical + sections are very small, thus we use a spinlock to protect them. + + The maptrack lock may be locked while holding the grant table lock. + Active entries are obtained by calling active_entry_acquire(gt, ref). This function returns a pointer to the active entry after locking its spinlock. The caller must hold the grant table lock for the gt in
xen/common/grant_table.c +5 -4
@@ -288,10 +288,10 @@ static inline void put_maptrack_handle( struct grant_table *t, int handle) { -spin_lock (&t->lock); +spin_lock (&t->maptrack_lock);maptrack_entry (t, handle).ref = t->maptrack_head; t->maptrack_head = handle; -spin_unlock (&t->lock); +spin_unlock (&t->maptrack_lock); } static inline int
@@ -303,7 +303,7 @@ get_maptrack_handle(struct grant_mapping *new_mt;unsigned int new_mt_limit, nr_frames; -spin_lock (&lgt->lock); +spin_lock (&lgt->maptrack_lock);while (unlikely ((handle =__get_maptrack_handle (lgt)) ==-1 ) ) {
@@ -332,7 +332,7 @@ get_maptrack_handle( nr_frames + 1); } -spin_unlock (&lgt->lock); +spin_unlock (&lgt->maptrack_lock);return handle; }
@@ -3005,6 +3005,7 @@ grant_table_create(/* Simple stuff. */ spin_lock_init (&t->lock); +spin_lock_init (&t->maptrack_lock); t->nr_grant_frames =INITIAL_NR_GRANT_FRAMES ;/* Active grant table. */
xen/include/xen/grant_table.h +2 -0
@@ -82,6 +82,8 @@ struct grant_table {struct grant_mapping **maptrack;unsigned int maptrack_head;unsigned int maptrack_limit; +/* Lock protecting the maptrack page list, head, and limit */ +spinlock_t maptrack_lock;/* Lock protecting updates to active and shared grant tables. */ spinlock_t lock;/* The defined versions are 1 and 2. Set to 0 if we don't know
Signature · 1 line
1.7.10.4
Re: [PATCHv10 2/4] gnttab: introduce maptrack lock
Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
Split grant table lock into two separate locks. One to protect maptrack state (
maptrack_lock) and one for everything else (lock).Based on a patch originally by Matt Wilson <[email protected]>.
Signed-off-by David Vrabel <[email protected]>
PATCH v10 3/4 gnttab: make the grant table lock a read-write lock
In reply to David VrabelIn combination with the per-active entry locks, the grant table lock can be made a read-write lock since the majority of cases only the read lock is required. The grant table read lock protects against changes to the table version or size (which are done with the write lock held).
The write lock is also required when two active entries must be acquired.
The double lock is still required when updating IOMMU page tables.
With the lock contention being only on the maptrack lock (unless IOMMU updates are required), performance and scalability is improved.
Based on a patch originally by Matt Wilson <[email protected]>.
---
v10:
- In gnttab_map_grant_ref(), keep double lock around maptrack update if gnttab_need_iommu_mapping(). Use a wmb(), otherwise.
--- docs/misc/grant-tables.txt | 30 ++++---- xen/arch/arm/mm.c | 4 +- xen/arch/x86/mm.c | 4 +- xen/common/grant_table.c | 156 +++++++++++++++++++++++------------------ xen/include/xen/grant_table.h | 9 ++- 5 files changed, 114 insertions(+), 89 deletions(-)
docs/misc/grant-tables.txt +17 -13
@@ -83,7 +83,7 @@ is complete. ~~~~~~~ Xen uses several locks to serialize access to the internal grant table state. - grant_table->lock : lock used to prevent readers from accessing + grant_table->lock : rwlock used to prevent readers from accessing inconsistent grant table state such as current version, partially initialized active table pages, etc.
@@ -91,9 +91,13 @@ is complete. active_grant_entry->lock : spinlock used to serialize modifications to active entries - The primary lock for the grant table is a spinlock. All functions - that access members of struct grant_table must acquire the lock - around critical sections. + The primary lock for the grant table is a read/write spinlock. All + functions that access members of struct grant_table must acquire a + read lock around critical sections. Any modification to the members + of struct grant_table (e.g., nr_status_frames, nr_grant_frames, + active frames, etc.) must only be made if the write lock is + held. These elements are read-mostly, and read critical sections can + be large, which makes a rwlock a good choice. The maptrack state is protected by its own spinlock. Any access (read or write) of struct grant_table members that have a "maptrack_"
@@ -105,25 +109,25 @@ is complete. Active entries are obtained by calling active_entry_acquire(gt, ref). This function returns a pointer to the active entry after locking its - spinlock. The caller must hold the grant table lock for the gt in - question before calling active_entry_acquire(). This is because the - grant table can be dynamically extended via gnttab_grow_table() while - a domain is running and must be fully initialized. Once all access to - the active entry is complete, release the lock by calling + spinlock. The caller must hold the grant table read lock before + calling active_entry_acquire(). This is because the grant table can + be dynamically extended via gnttab_grow_table() while a domain is + running and must be fully initialized. Once all access to the active + entry is complete, release the lock by calling active_entry_release(act). Summary of rules for locking: active_entry_acquire() and active_entry_release() can only be - called when holding the relevant grant table's lock. I.e.: - spin_lock(>->lock); + called when holding the relevant grant table's read lock. I.e.: + read_lock(>->lock); act = active_entry_acquire(gt, ref); ... active_entry_release(act); - spin_unlock(>->lock); + read_unlock(>->lock); Active entries cannot be acquired while holding the maptrack lock. Multiple active entries can be acquired while holding the grant table - lock. + _write_ lock. ********************************************************************************
xen/arch/arm/mm.c +2 -2
@@ -1048,7 +1048,7 @@ int xenmem_add_to_physmap_one(switch ( space ) {case XENMAPSPACE_grant_table: -spin_lock (&d->grant_table->lock); +write_lock (&d->grant_table->lock);if ( d->grant_table->gt_version ==0 ) d->grant_table->gt_version =1 ;
@@ -1078,7 +1078,7 @@ int xenmem_add_to_physmap_one( t = p2m_ram_rw; -spin_unlock (&d->grant_table->lock); +write_unlock (&d->grant_table->lock);break ;case XENMAPSPACE_shared_info:if ( idx !=0 )
xen/arch/x86/mm.c +2 -2
@@ -4593,7 +4593,7 @@ int xenmem_add_to_physmap_one( mfn =virt_to_mfn (d->shared_info);break ;case XENMAPSPACE_grant_table: -spin_lock (&d->grant_table->lock); +write_lock (&d->grant_table->lock);if ( d->grant_table->gt_version ==0 ) d->grant_table->gt_version =1 ;
@@ -4615,7 +4615,7 @@ int xenmem_add_to_physmap_one( mfn =virt_to_mfn (d->grant_table->shared_raw[idx]); } -spin_unlock (&d->grant_table->lock); +write_unlock (&d->grant_table->lock);break ;case XENMAPSPACE_gmfn_range:case XENMAPSPACE_gmfn:
xen/common/grant_table.c +87 -69
485 lines not on this page · raw message
xen/include/xen/grant_table.h +6 -3
@@ -64,6 +64,11 @@ struct grant_mapping {/* Per-domain grant information. */ struct grant_table { +/* +* Lock protecting updates to grant table state (version, active +* entry list, etc.) +*/ +rwlock_t lock;/* Table size. Number of frames shared with guest */ unsigned int nr_grant_frames;/* Shared grant table (see include/public/grant_table.h). */
@@ -84,8 +89,6 @@ struct grant_table {unsigned int maptrack_limit;/* Lock protecting the maptrack page list, head, and limit */ spinlock_t maptrack_lock; -/* Lock protecting updates to active and shared grant tables. */ -spinlock_t lock;/* The defined versions are 1 and 2. Set to 0 if we don't know what version to use yet. */ unsigned gt_version;
@@ -103,7 +106,7 @@ gnttab_release_mappings( struct domain *d);/* Increase the size of a domain's grant table. -* Caller must hold d's grant table lock. +* Caller must hold d's grant table write lock. */ int gnttab_grow_table (struct domain *d,unsigned int req_nr_frames);
Signature · 1 line
1.7.10.4
Re: [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
xen/common/grant_table.c +2
-2@@ -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);
xen/common/grant_table.c double_gt_lock(struct grant_table *lgt, struct grant_table *rgt) · line 266
Quoted 10 lines
xen/common/grant_table.c +2
-2}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 ...
xen/common/grant_table.c static void mapcount( · line 574
Quoted 11 lines
xen/common/grant_table.c +3
-3@@ -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.
Quoted 8 lines
xen/common/grant_table.c +4
-0@@ -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. +*
xen/common/grant_table.c __gnttab_map_grant_ref( · line 853
Quoted 10 lines
xen/common/grant_table.c +6
-1+* 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)?
xen/common/grant_table.c __gnttab_map_grant_ref( · line 856
Quoted 3 lines
xen/common/grant_table.c +2
-1-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.
Quoted 10 lines
xen/common/grant_table.c +1
-1@@ -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 ) {
xen/common/grant_table.c gnttab_transfer( · line 1826
Quoted 5 lines
xen/common/grant_table.c +1
-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?
Quoted 10 lines
xen/common/grant_table.c +1
-1@@ -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)))
xen/common/grant_table.c out: · line 2710
Quoted 5 lines
xen/common/grant_table.c +1
-1@@ -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.
Jan
Re: [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
Quoted 16 lines · Jan Beulich
Quoted 16 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
xen/common/grant_table.c +3
-4@@ -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);
Quoted 8 lines
xen/common/grant_table.c double_gt_lock(struct grant_table *lgt, struct grant_table *rgt) · line 265
Quoted 3 lines
xen/common/grant_table.c +1
-0+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.
Quoted 28 lines
xen/common/grant_table.c static void mapcount( · line 574
Quoted 11 lines
xen/common/grant_table.c +3
-3@@ -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.Quoted 13 lines
xen/common/grant_table.c +8
-0@@ -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);
Quoted 8 lines
xen/common/grant_table.c __gnttab_map_grant_ref( · line 853
Quoted 5 lines
xen/common/grant_table.c +2
-1mt->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()andrmb()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.
Quoted 7 lines
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 theop->map->flagsandop->map->domidaccesses 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.
Quoted 7 lines
xen/common/grant_table.c __gnttab_map_grant_ref( · line 856
Quoted 3 lines
xen/common/grant_table.c +2
-1-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.
Quoted 13 lines
Quoted 13 lines
xen/common/grant_table.c +1
-1@@ -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;
Quoted 10 lines
xen/common/grant_table.c gnttab_transfer( · line 1826
Quoted 2 lines
xen/common/grant_table.c +1
-1-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.
Quoted 7 lines
Quoted 7 lines
xen/common/grant_table.c +1
-1@@ -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);
Quoted 11 lines
xen/common/grant_table.c out: · line 2710
Quoted 8 lines
xen/common/grant_table.c +1
-1/* 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.
David
Re: [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
Quoted 70 lines
Quoted 1 line
Quoted 1 line · On 28.05.15 at 18:09, <[email protected]> wrote:
Quoted 16 lines · Jan Beulich
Quoted 16 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
xen/common/grant_table.c +3
-4@@ -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);Quoted 8 lines
xen/common/grant_table.c double_gt_lock(struct grant_table *lgt, struct grant_table *rgt) · line 265
Quoted 3 lines
xen/common/grant_table.c +1
-0+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.
Quoted 32 lines
xen/common/grant_table.c static void mapcount( · line 574
Quoted 11 lines
xen/common/grant_table.c +3
-3@@ -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.Quoted 17 lines
xen/common/grant_table.c +9
-1@@ -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 ();Quoted 8 lines
xen/common/grant_table.c __gnttab_map_grant_ref( · line 853
Quoted 1 line
xen/common/grant_table.c +1
-0+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 theop->map->flagsandop->map->domidaccesses 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)?
Quoted 3 lines
The
double_gt_lock()takes both write locks, thus does not race with__gnttab_unmap_commonclearing 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.
Quoted 11 lines
Quoted 7 lines
xen/common/grant_table.c __gnttab_map_grant_ref( · line 856
Quoted 3 lines
xen/common/grant_table.c +2
-1-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.
Quoted 19 lines
Quoted 7 lines
Quoted 7 lines
xen/common/grant_table.c +1
-1@@ -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);Quoted 11 lines
xen/common/grant_table.c out: · line 2710
Quoted 8 lines
xen/common/grant_table.c +1
-1/* 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.
Quoted 2 lines
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
Re: [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
Quoted 74 lines · Jan Beulich
Quoted 70 lines
Quoted 1 line
Quoted 1 line · On 28.05.15 at 18:09, <[email protected]> wrote:
Quoted 16 lines · Jan Beulich
Quoted 16 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
xen/common/grant_table.c +3
-4@@ -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);Quoted 8 lines
xen/common/grant_table.c double_gt_lock(struct grant_table *lgt, struct grant_table *rgt) · line 265
Quoted 3 lines
xen/common/grant_table.c +1
-0+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.
Quoted 32 lines
xen/common/grant_table.c static void mapcount( · line 574
Quoted 11 lines
xen/common/grant_table.c +3
-3@@ -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.Quoted 17 lines
xen/common/grant_table.c +9
-1@@ -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 ();Quoted 8 lines
xen/common/grant_table.c __gnttab_map_grant_ref( · line 853
Quoted 1 line
xen/common/grant_table.c +1
-0+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 theop->map->flagsandop->map->domidaccesses 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)?Quoted 3 lines
The
double_gt_lock()takes both write locks, thus does not race with__gnttab_unmap_commonclearing the flag on the maptrack entry which is done while holding the remote read lock.
Quoted 5 lines
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.
Quoted 19 lines
Quoted 19 lines
Quoted 18 lines
Quoted 10 lines
xen/common/grant_table.c +1
-1@@ -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)))xen/common/grant_table.c out: · line 2710
Quoted 5 lines
xen/common/grant_table.c +1
-1@@ -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.
Quoted 5 lines
Quoted 2 lines
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
Re: [PATCHv10 3/4] gnttab: make the grant table lock a read-write lock
Quoted 35 lines
Quoted 1 line
Quoted 1 line · On 02.06.15 at 15:22, <[email protected]> wrote:
Quoted 33 lines · Jan Beulich
Quoted 32 lines
Quoted 1 line
Quoted 1 line · On 28.05.15 at 18:09, <[email protected]> wrote:
Quoted 18 lines · Jan Beulich
Quoted 18 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
xen/common/grant_table.c +9
-1@@ -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 ();Quoted 8 lines
xen/common/grant_table.c __gnttab_map_grant_ref( · line 853
Quoted 1 line
xen/common/grant_table.c +1
-0+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 theop->map->flagsandop->map->domidaccesses 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_commonclearing the flag on the maptrack entry which is done while holding the remote read lock.
Quoted 8 lines
Quoted 5 lines
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().
Quoted 22 lines
Quoted 22 lines
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
xen/common/grant_table.c +1
-1@@ -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);Quoted 11 lines
xen/common/grant_table.c out: · line 2710
Quoted 8 lines
xen/common/grant_table.c +1
-1/* 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.
Quoted 6 lines
Quoted 2 lines
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_versioncheck. 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
PATCH v10 4/4 gnttab: use per-VCPU maptrack free lists
In reply to David VrabelFrom: Malcolm Crossley <[email protected]>
Performance analysis of aggregate network throughput with many VMs shows that performance is signficantly limited by contention on the maptrack lock when obtaining/releasing maptrack handles from the free list.
Instead of a single free list use a per-VCPU list. This avoids any contention when obtaining a handle. Handles must be released back to their original list and since this may occur on a different VCPU there is some contention on the destination VCPU's free list tail pointer (but this is much better than a per-domain lock).
Increase the default maximum number of maptrack frames by 4 times
because: a) struct grant_mapping is now 16 bytes (instead of 8); and
b) a guest may not evenly distribute all the grant map operations across the VCPUs (meaning some VCPUs need more maptrack entries than others).
---
v10:
- Divide max_maptrack_frames evenly amongst the VCPUs.
- Increase default max_maptrack_frames to compensate.
--- xen/common/grant_table.c | 146 +++++++++++++++++++++++++---------------- xen/include/xen/grant_table.h | 8 ++- xen/include/xen/sched.h | 5 ++ 3 files changed, 99 insertions(+), 60 deletions(-)
xen/common/grant_table.c +89 -57
@@ -57,7 +57,7 @@ integer_param("gnttab_max_frames", max_grant_frames);* New options allow to set max_maptrack_frames and * map_grant_table_frames independently. */ -#define DEFAULT_MAX_MAPTRACK_FRAMES 256 +#define DEFAULT_MAX_MAPTRACK_FRAMES 1024static unsigned int __read_mostly max_maptrack_frames;integer_param ("gnttab_max_maptrack_frames" , max_maptrack_frames);
@@ -117,12 +117,6 @@ struct gnttab_unmap_common {#define maptrack_entry (t, e) \ ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE]) -static inline unsigned int -nr_maptrack_frames (struct grant_table *t) -{ -return t->maptrack_limit /MAPTRACK_PER_PAGE ; -} -#define MAPTRACK_TAIL (~0u)#define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
@@ -275,66 +269,104 @@ double_gt_unlock(struct grant_table *lgt, struct grant_table *rgt)static inline int __get_maptrack_handle ( -struct grant_table *t) +struct grant_table *t, +struct vcpu *v) { -unsigned int h; -if (unlikely ((h = t->maptrack_head) ==MAPTRACK_TAIL ) ) +unsigned int head, next; + +/* No maptrack pages allocated for this VCPU yet? */ + head = v->maptrack_head; +if (unlikely (head ==MAPTRACK_TAIL ) )return -1 ; - t->maptrack_head =maptrack_entry (t, h).ref; -return h; + +/* +* Always keep one entry in the free list to make it easier to add +* free entries to the tail. +*/ + next =read_atomic (&maptrack_entry (t, head).ref); +if (unlikely (next ==MAPTRACK_TAIL ) ) +return -1 ; + + v->maptrack_head = next; + +return head; }static inline void put_maptrack_handle (struct grant_table *t,int handle) { -spin_lock (&t->maptrack_lock); -maptrack_entry (t, handle).ref = t->maptrack_head; - t->maptrack_head = handle; -spin_unlock (&t->maptrack_lock); +struct domain *d = current->domain; +struct vcpu *v; +unsigned int prev_tail, cur_tail; + +/* 1. Set entry to be a tail. */ +maptrack_entry (t, handle).ref =MAPTRACK_TAIL ; + +/* 2. Add entry to the tail of the list on the original VCPU. */ + v = d->vcpu[maptrack_entry (t,handle).vcpu]; + + cur_tail =read_atomic (&v->maptrack_tail); +do { + prev_tail = cur_tail; + cur_tail =cmpxchg (&v->maptrack_tail, prev_tail, handle); + }while ( cur_tail != prev_tail ); + +/* 3. Update the old tail entry to point to the new entry. */ +write_atomic (&maptrack_entry (t, prev_tail).ref, handle); }static inline int get_maptrack_handle (struct grant_table *lgt) { +struct vcpu *v = current;int i;grant_handle_t handle;struct grant_mapping *new_mt; -unsigned int new_mt_limit, nr_frames; -spin_lock (&lgt->maptrack_lock); + handle =__get_maptrack_handle (lgt, v); +if (likely (handle !=-1 ) ) +return handle; -while (unlikely ((handle =__get_maptrack_handle (lgt)) ==-1 ) ) - { - nr_frames =nr_maptrack_frames (lgt); -if ( nr_frames >= max_maptrack_frames ) -break ; +/* +* max_maptrack_frames is per domain so each VCPU gets a share of +* the maximum, but allow at least one frame per VCPU. +*/ +if ( v->maptrack_frames + && v->maptrack_frames >= max_maptrack_frames / v->domain->max_vcpus ) +return -1 ; - new_mt =alloc_xenheap_page (); -if ( !new_mt ) -break ; + new_mt =alloc_xenheap_page (); +if ( !new_mt ) +return -1 ; +clear_page (new_mt); -clear_page (new_mt); +spin_lock (&lgt->maptrack_lock); - new_mt_limit = lgt->maptrack_limit +MAPTRACK_PER_PAGE ; +for ( i =1 ; i <MAPTRACK_PER_PAGE ; i++ ) + { + new_mt[i -1 ].ref = (lgt->maptrack_pages *MAPTRACK_PER_PAGE ) + i; + new_mt[i -1 ].vcpu = v->vcpu_id; + } +/* Set last entry vcpu and ref */ + new_mt[i -1 ].ref = v->maptrack_head; + new_mt[i -1 ].vcpu = v->vcpu_id; -for ( i =1 ; i <MAPTRACK_PER_PAGE ; i++ ) - new_mt[i -1 ].ref = lgt->maptrack_limit + i; - new_mt[i -1 ].ref = lgt->maptrack_head; - lgt->maptrack_head = lgt->maptrack_limit; + v->maptrack_head = lgt->maptrack_pages *MAPTRACK_PER_PAGE ; - lgt->maptrack[nr_frames] = new_mt; -smp_wmb (); - lgt->maptrack_limit = new_mt_limit; +/* Set tail directly if this is the first page for this VCPU. */ +if ( v->maptrack_tail ==MAPTRACK_TAIL ) + v->maptrack_tail = (lgt->maptrack_pages *MAPTRACK_PER_PAGE ) + +MAPTRACK_PER_PAGE -1 ; -gdprintk (XENLOG_INFO ,"Increased maptrack size to %u frames\n" , - nr_frames +1 ); - } + lgt->maptrack[lgt->maptrack_pages++] = new_mt;spin_unlock (&lgt->maptrack_lock); -return handle; + v->maptrack_frames++; + +return __get_maptrack_handle (lgt, v); }/* Number of grant table entries. Caller must hold d's grant table lock. */
@@ -573,7 +605,8 @@ static void mapcount(*/ ASSERT (rw_is_write_locked (&rd->grant_table->lock)); -for ( handle =0 ; handle < lgt->maptrack_limit; handle++ ) +for ( handle =0 ; handle < lgt->maptrack_pages *MAPTRACK_PER_PAGE ; + handle++ ) { map = &maptrack_entry (lgt, handle); if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
@@ -941,7 +974,7 @@ __gnttab_unmap_common( op->frame = (unsigned long )(op->dev_bus_addr >>PAGE_SHIFT ); -if (unlikely (op->handle >= lgt->maptrack_limit) ) +if (unlikely (op->handle >= lgt->maptrack_pages *MAPTRACK_PER_PAGE ) ) {gdprintk (XENLOG_INFO ,"Bad handle (%d).\n" , op->handle); op->status = GNTST_bad_handle;
@@ -1416,6 +1449,7 @@ gnttab_setup_table(struct gnttab_setup_table op;struct domain *d;struct grant_table *gt; +struct vcpu *v;int i;xen_pfn_t gmfn;
@@ -1457,6 +1491,17 @@ gnttab_setup_table( gt = d->grant_table;write_lock (>->lock); +/* Tracking of mapped foreign frames table */ + gt->maptrack = xzalloc_array(struct grant_mapping *, max_maptrack_frames); +if ( gt->maptrack ==NULL ) + goto out3; +for_each_vcpu ( d, v ) + { + v->maptrack_head =MAPTRACK_TAIL ; + v->maptrack_tail =MAPTRACK_TAIL ; + } + gt->maptrack_pages =0 ; +if ( gt->gt_version ==0 ) gt->gt_version =1 ;
@@ -3040,18 +3085,6 @@ grant_table_create(spin_lock_init (&t->active[i][j].lock); } -/* Tracking of mapped foreign frames table */ - if ( (t->maptrack = xzalloc_array(struct grant_mapping *, - max_maptrack_frames)) ==NULL ) - goto no_mem_2; -if ( (t->maptrack[0 ] =alloc_xenheap_page ()) ==NULL ) - goto no_mem_3; -clear_page (t->maptrack[0 ]); - t->maptrack_limit =MAPTRACK_PER_PAGE ; -for ( i =1 ; i <MAPTRACK_PER_PAGE ; i++ ) - t->maptrack[0 ][i -1 ].ref = i; - t->maptrack[0 ][i -1 ].ref =MAPTRACK_TAIL ; -/* Shared grant table. */ if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL ) goto no_mem_3;
@@ -3082,8 +3115,6 @@ grant_table_create(free_xenheap_page (t->shared_raw[i]);xfree (t->shared_raw); no_mem_3: -free_xenheap_page (t->maptrack[0 ]); -xfree (t->maptrack); no_mem_2:for ( i =0 ; i <num_act_frames_from_sha_frames (INITIAL_NR_GRANT_FRAMES ); i++ )
@@ -3111,7 +3142,8 @@ gnttab_release_mappings(BUG_ON (!d->is_dying); -for ( handle =0 ; handle < gt->maptrack_limit; handle++ ) +for ( handle =0 ; handle < gt->maptrack_pages *MAPTRACK_PER_PAGE ; + handle++ ) { map = &maptrack_entry (gt, handle);if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
@@ -3216,7 +3248,7 @@ grant_table_destroy(free_xenheap_page (t->shared_raw[i]);xfree (t->shared_raw); -for ( i =0 ; i <nr_maptrack_frames (t); i++ ) +for ( i =0 ; i < t->maptrack_pages; i++ )free_xenheap_page (t->maptrack[i]);xfree (t->maptrack);
xen/include/xen/grant_table.h +5 -3
@@ -60,6 +60,8 @@ struct grant_mapping {u32 ref;/* grant ref */ u16 flags;/* 0-4: GNTMAP_* ; 5-15: unused */ domid_t domid;/* granting domain */ +u32 vcpu;/* vcpu which created the grant mapping */ +u32 pad;/* round size to a power of 2 */ };/* Per-domain grant information. */
@@ -83,10 +85,10 @@ struct grant_table {grant_status_t **status;/* Active grant table. */ struct active_grant_entry **active; -/* Mapping tracking table. */ +/* Mapping tracking table per vcpu. */ struct grant_mapping **maptrack; -unsigned int maptrack_head; -unsigned int maptrack_limit; +/* Total pages used for mapping tracking table */ +unsigned int maptrack_pages;/* Lock protecting the maptrack page list, head, and limit */ spinlock_t maptrack_lock;/* The defined versions are 1 and 2. Set to 0 if we don't know
xen/include/xen/sched.h +5 -0
@@ -219,6 +219,11 @@ struct vcpu/* VCPU paused by system controller. */ int controller_pause_count; +/* Maptrack */ +unsigned int maptrack_head; +unsigned int maptrack_tail; +unsigned int maptrack_frames; +/* IRQ-safe virq_lock protects against delivering VIRQ to stale evtchn. */ evtchn_port_t virq_to_evtchn[NR_VIRQS ];spinlock_t virq_lock;
Signature · 1 line
1.7.10.4
Re: [PATCHv10 4/4] gnttab: use per-VCPU maptrack free lists
Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
v10:
- Dividemax_maptrack_framesevenly amongst the VCPUs.
- Increase defaultmax_maptrack_framesto compensate.
I'm not really happy about this, but also can't immediately suggest a better model (without removing maptrack frames from vCPU-s). I'd really like to hear opinions of others, namely maintainers, regarding this resource use issue. Should we perhaps start accounting maptrack frames against the domain's allocation (i.e. a domain can get as many as it likes beyond the set limit, provided it ballooned out suitably many pages up front)?
xen/common/grant_table.c integer_param("gnttab_max_frames", max_grant_frames); · line 60
Quoted 8 lines
xen/common/grant_table.c +1
-1@@ -57,7 +57,7 @@ integer_param("gnttab_max_frames", max_grant_frames);* New options allow to set max_maptrack_frames and * map_grant_table_frames independently. */ -#define DEFAULT_MAX_MAPTRACK_FRAMES 256 +#define DEFAULT_MAX_MAPTRACK_FRAMES 1024
Apart from everything else this again results in ...
xen/common/grant_table.c gnttab_setup_table( · line 1495
Quoted 6 lines
xen/common/grant_table.c +2
-0@@ -1457,6 +1491,17 @@ gnttab_setup_table( gt = d->grant_table;write_lock (>->lock); +/* Tracking of mapped foreign frames table */ + gt->maptrack = xzalloc_array(struct grant_mapping *, max_maptrack_frames);
... this becoming an order-1 runtime allocation on other than 32-bit ARM.
Jan
Re: [PATCHv10 4/4] gnttab: use per-VCPU maptrack free lists
Quoted 16 lines · Jan Beulich
xen/common/grant_table.c integer_param("gnttab_max_frames", max_grant_frames); · line 60
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
xen/common/grant_table.c +1
-1@@ -57,7 +57,7 @@ integer_param("gnttab_max_frames", max_grant_frames);* New options allow to set max_maptrack_frames and * map_grant_table_frames independently. */ -#define DEFAULT_MAX_MAPTRACK_FRAMES 256 +#define DEFAULT_MAX_MAPTRACK_FRAMES 1024Apart from everything else this again results in ...
Quoted 4 lines
xen/common/grant_table.c +0
-0@@ -1457,6 +1491,17 @@ gnttab_setup_table( gt = d->grant_table;write_lock (>->lock);
Quoted 5 lines
xen/common/grant_table.c gnttab_setup_table( · line 1495
Quoted 2 lines
xen/common/grant_table.c +2
-0+/* Tracking of mapped foreign frames table */ + gt->maptrack = xzalloc_array(struct grant_mapping *, max_maptrack_frames);... this becoming an order-1 runtime allocation on other than 32-bit ARM.
I thought we agreed that this was temporary until vzalloc() was added?
David
Re: [PATCHv10 4/4] gnttab: use per-VCPU maptrack free lists
Quoted 18 lines
Quoted 1 line
Quoted 1 line · On 28.05.15 at 18:10, <[email protected]> wrote:
Quoted 16 lines · Jan Beulich
xen/common/grant_table.c integer_param("gnttab_max_frames", max_grant_frames); · line 60
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 26.05.15 at 20:00, <[email protected]> wrote:
xen/common/grant_table.c +1
-1@@ -57,7 +57,7 @@ integer_param("gnttab_max_frames", max_grant_frames);* New options allow to set max_maptrack_frames and * map_grant_table_frames independently. */ -#define DEFAULT_MAX_MAPTRACK_FRAMES 256 +#define DEFAULT_MAX_MAPTRACK_FRAMES 1024Apart from everything else this again results in ...
Quoted 4 lines
xen/common/grant_table.c +0
-0@@ -1457,6 +1491,17 @@ gnttab_setup_table( gt = d->grant_table;write_lock (>->lock);
Quoted 7 lines
Quoted 5 lines
xen/common/grant_table.c gnttab_setup_table( · line 1495
Quoted 2 lines
xen/common/grant_table.c +2
-0+/* Tracking of mapped foreign frames table */ + gt->maptrack = xzalloc_array(struct grant_mapping *, max_maptrack_frames);... this becoming an order-1 runtime allocation on other than 32-bit ARM.
I thought we agreed that this was temporary until
vzalloc()was added?
Was it this one? And anyway, the vzalloc() addition went in almost two weeks ago.
Jan