gnttab: Improve scaleability
v6 cover gnttab: Improve scaleability
The series makes the grant table locking for fine-grained and add per-VCPU maptrack free lists, which greatly improves scalability.
The series builds on the original series by Matt Wilson and Christoph Egger from Amazon.
The per-VCPU maptrack free lists makes one of our aggregate intrahost network throughput benchmarks increases from 15 Gbit/s to 75 Gbit/s, when compared to just Amazon's original patches.
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 v6 1/5 gnttab: add locking documentation
From: Matt Wilson <[email protected]>
The grant table locking is becomes more fine-grained in subsequent commits. Describe how it will work.
--- docs/misc/grant-tables.txt | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
docs/misc/grant-tables.txt +34 -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,39 @@ 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 protect grant table size, version, + etc. updates + grant_table->maptrack_lock : spinlock used to protect the maptrack state + 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 modify or access members of struct grant_table must acquire the + lock around critical sections. As an exception, testing that ref in + in range of nr_grant_frames and nr_status_frames or gt_version is + initialized, does _not_ require the grant table lock. This is safe + because the grant table only grows and the table version is only set + once (to 1 or 2). In particular, acquiring an active entry (see + below) does not require the grant table lock to be held. + + 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. + + Active entries are obtained by calling active_entry_acquire(gt, ref). + This function returns a pointer to the active entry after locking its + spinlock. Once all access to the active entry is complete, release + the lock by calling active_entry_release(act). ********************************************************************************
Signature · 1 line
1.7.10.4
PATCH v6 2/5 gnttab: introduce per-active entry locks
In reply to David VrabelFrom: Matt Wilson <[email protected]>
Instead of protecting the state of a grant table active entry with the grant table lock, use per active entry locks.
Active entries must be acquired with active_entry_acquire() and released with active_entry_release() which lock and unlock the entry's spinlock.
This is the first step to improving grant table scalability and will allow the heaviliy contented grant table lock to be used less.
--- xen/common/grant_table.c | 213 ++++++++++++++++++++++++++++------------------ 1 file changed, 129 insertions(+), 84 deletions(-)
xen/common/grant_table.c +129 -84
@@ -157,10 +157,13 @@ struct active_grant_entry {in the page. */ unsigned length:16; /* For sub-page grants, the length of thegrant. */ +spinlock_t lock;/* lock to protect access of this entry. +see docs/misc/grant-tables.txt for +locking protocol */ };#define ACGNT_PER_PAGE (PAGE_SIZE / sizeof(struct active_grant_entry)) -#define active_entry (t, e) \ +#define _active_entry (t, e) \ ((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE])static inline void gnttab_flush_tlb (const struct domain *d)
@@ -188,6 +191,24 @@ nr_active_grant_frames(struct grant_table *gt)return num_act_frames_from_sha_frames (nr_grant_frames (gt)); } +static inline struct active_grant_entry * +active_entry_acquire (struct grant_table *t,grant_ref_t e) +{ +struct active_grant_entry *act; + +ASSERT (spin_is_locked (&t->lock)); + + act = &_active_entry (t, e); +spin_lock (&act->lock); + +return act; +} + +static inline void active_entry_release (struct active_grant_entry *act) +{ +spin_unlock (&act->lock); +} +/* Check if the page has been paged out, or needs unsharing. If rc == GNTST_okay, *page contains the page struct with a ref taken. Caller must do put_page(*page).
@@ -228,30 +249,6 @@ static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pagreturn rc; } -static inline void -double_gt_lock (struct grant_table *lgt,struct grant_table *rgt) -{ -if ( lgt < rgt ) - { -spin_lock (&lgt->lock); -spin_lock (&rgt->lock); - } -else - { -if ( lgt != rgt ) -spin_lock (&rgt->lock); -spin_lock (&lgt->lock); - } -} - -static inline void -double_gt_unlock (struct grant_table *lgt,struct grant_table *rgt) -{ -spin_unlock (&lgt->lock); -if ( lgt != rgt ) -spin_unlock (&rgt->lock); -} -static inline int __get_maptrack_handle (struct grant_table *t)
@@ -505,26 +502,23 @@ static int grant_map_exists(const struct domain *ld,unsigned long mfn, unsigned int *ref_count) { - const struct active_grant_entry *act; + struct active_grant_entry *act;unsigned int ref, max_iter;ASSERT (spin_is_locked (&rgt->lock)); max_iter =min (*ref_count + (1 <<GNTTABOP_CONTINUATION_ARG_SHIFT ),nr_grant_entries (rgt)); -for ( ref = *ref_count; ref < max_iter; ref++ ) +for ( ref = *ref_count; ref < max_iter;active_entry_release (act), ref++ ) { - act = &active_entry (rgt, ref); + act =active_entry_acquire (rgt, ref); -if ( !act->pin ) -continue ; - -if ( act->domid != ld->domain_id ) -continue ; - -if ( act->frame != mfn ) +if ( !act->pin || + act->domid != ld->domain_id || + act->frame != mfn )continue ; +active_entry_release (act);return 0 ; }
@@ -548,11 +542,14 @@ static void mapcount(for ( handle =0 ; handle < lgt->maptrack_limit; handle++ ) { +struct active_grant_entry *act; + map = &maptrack_entry (lgt, handle);if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) || map->domid != rd->domain_id )continue ; -if (active_entry (rd->grant_table, map->ref).frame == mfn ) + act = &_active_entry (rd->grant_table, map->ref); +if ( act->frame == mfn ) (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++; } }
@@ -576,7 +573,6 @@ __gnttab_map_grant_ref(struct page_info *pg =NULL ;int rc = GNTST_okay;u32 old_pin; -u32 act_pin;unsigned int cache_flags;struct active_grant_entry *act =NULL ;struct grant_mapping *mt;
@@ -639,7 +635,7 @@ __gnttab_map_grant_ref(if (unlikely (op->ref >=nr_grant_entries (rgt)))PIN_FAIL (unlock_out, GNTST_bad_gntref,"Bad ref (%d).\n" , op->ref); - act = &active_entry (rgt, op->ref); + act =active_entry_acquire (rgt, op->ref); shah =shared_entry_header (rgt, op->ref);if (rgt->gt_version ==1 ) { sha1 = &shared_entry_v1 (rgt, op->ref);
@@ -656,7 +652,7 @@ __gnttab_map_grant_ref( ((act->domid != ld->domain_id) || (act->pin & 0x80808080U) != 0 || (act->is_sub_page)) ) -PIN_FAIL (unlock_out, GNTST_general_error, +PIN_FAIL (act_release_out, GNTST_general_error,"Bad domain (%d != %d), or risk of counter overflow %08x, or subpage %d\n" , act->domid, ld->domain_id, act->pin, act->is_sub_page);
@@ -667,7 +663,7 @@ __gnttab_map_grant_ref(if ( (rc =_set_status (rgt->gt_version, ld->domain_id, op->flags & GNTMAP_readonly,1 , shah, act, status) ) != GNTST_okay ) - goto unlock_out; + goto act_release_out;if ( !act->pin ) {
@@ -698,12 +694,9 @@ __gnttab_map_grant_ref( GNTPIN_hstr_inc : GNTPIN_hstw_inc; frame = act->frame; - act_pin = act->pin; cache_flags = (shah->flags & (GTF_PAT |GTF_PWT |GTF_PCD ) ); -spin_unlock (&rgt->lock); -/* pg may be set, with a refcount included, from __get_paged_frame */ if ( !pg ) {
@@ -778,8 +771,6 @@ __gnttab_map_grant_ref( goto undo_out; } -double_gt_lock (lgt, rgt); -if (gnttab_need_iommu_mapping (ld) ) {unsigned int wrc, rdc;
@@ -787,21 +778,20 @@ __gnttab_map_grant_ref(/* We're not translated, so we know that gmfns and mfns are the same things, so the IOMMU entry is always 1-to-1. */ mapcount (lgt, rd, frame, &wrc, &rdc); -if ( (act_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) && +if ( (act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) && !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) ) {if ( wrc ==0 ) err =iommu_map_page (ld, frame, frame, IOMMUF_readable|IOMMUF_writable); } -else if ( act_pin && !old_pin ) +else if ( act->pin && !old_pin ) {if ( (wrc + rdc) ==0 ) err =iommu_map_page (ld, frame, frame, IOMMUF_readable); }if ( err ) { -double_gt_unlock (lgt, rgt); rc = GNTST_general_error; goto undo_out; }
@@ -814,7 +804,8 @@ __gnttab_map_grant_ref( mt->ref = op->ref; mt->flags = op->flags; -double_gt_unlock (lgt, rgt); +active_entry_release (act); +spin_unlock (&rgt->lock); op->dev_bus_addr = (u64 )frame <<PAGE_SHIFT ; op->handle = handle;
@@ -837,10 +828,6 @@ __gnttab_map_grant_ref(put_page (pg); } -spin_lock (&rgt->lock); - - act = &active_entry (rgt, op->ref); -if ( op->flags & GNTMAP_device_map ) act->pin -= (op->flags & GNTMAP_readonly) ? GNTPIN_devr_inc : GNTPIN_devw_inc;
@@ -856,6 +843,9 @@ __gnttab_map_grant_ref(if ( !act->pin )gnttab_clear_flag (_GTF_reading, status); + act_release_out: +active_entry_release (act); + unlock_out:spin_unlock (&rgt->lock); op->status = rc;
@@ -939,18 +929,19 @@ __gnttab_unmap_common(TRACE_1D (TRC_MEM_PAGE_GRANT_UNMAP , dom); rgt = rd->grant_table; -double_gt_lock (lgt, rgt); + +spin_lock (&rgt->lock); + act =active_entry_acquire (rgt, op->map->ref); op->flags = op->map->flags;if (unlikely (!op->flags) ||unlikely (op->map->domid != dom) ) {gdprintk (XENLOG_WARNING ,"Unstable handle %u\n" , op->handle); rc = GNTST_bad_handle; - goto unmap_out; + goto act_release_out; } op->rd = rd; - act = &active_entry (rgt, op->map->ref);if ( op->frame ==0 ) {
@@ -959,7 +950,7 @@ __gnttab_unmap_common(else {if (unlikely (op->frame != act->frame) ) -PIN_FAIL (unmap_out, GNTST_general_error, +PIN_FAIL (act_release_out, GNTST_general_error,"Bad frame number doesn't match gntref. (%lx != %lx)\n" , op->frame, act->frame);if ( op->flags & GNTMAP_device_map )
@@ -978,7 +969,7 @@ __gnttab_unmap_common(if ( (rc =replace_grant_host_mapping (op->host_addr, op->frame, op->new_addr, op->flags)) <0 ) - goto unmap_out; + goto act_release_out;ASSERT (act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask)); op->map->flags &= ~GNTMAP_host_map;
@@ -1000,7 +991,7 @@ __gnttab_unmap_common(if ( err ) { rc = GNTST_general_error; - goto unmap_out; + goto act_release_out; } }
@@ -1008,8 +999,10 @@ __gnttab_unmap_common(if ( !(op->flags & GNTMAP_readonly) )gnttab_mark_dirty (rd, op->frame); - unmap_out: -double_gt_unlock (lgt, rgt); + act_release_out: +active_entry_release (act); +spin_unlock (&rgt->lock); + op->status = rc;rcu_unlock_domain (rd); }
@@ -1039,12 +1032,12 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)rcu_lock_domain (rd); rgt = rd->grant_table; -spin_lock (&rgt->lock); +spin_lock (&rgt->lock);if ( rgt->gt_version ==0 ) - goto unmap_out; + goto unlock_out; - act = &active_entry (rgt, op->map->ref); + act =active_entry_acquire (rgt, op->map->ref); sha =shared_entry_header (rgt, op->map->ref);if ( rgt->gt_version ==1 )
@@ -1058,7 +1051,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)* Suggests that __gntab_unmap_common failed early and so * nothing further to do */ - goto unmap_out; + goto act_release_out; } pg =mfn_to_page (op->frame);
@@ -1082,7 +1075,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)* Suggests that __gntab_unmap_common failed in * replace_grant_host_mapping() so nothing further to do */ - goto unmap_out; + goto act_release_out; }if ( !is_iomem_page (op->frame) )
@@ -1103,8 +1096,12 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)if ( act->pin ==0 )gnttab_clear_flag (_GTF_reading, status); - unmap_out: + act_release_out: +active_entry_release (act); + + unlock_out:spin_unlock (&rgt->lock); +if ( put_handle ) { op->map->flags =0 ;
@@ -1290,13 +1287,17 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) gt->nr_status_frames =0 ; } +/* +* Grow the grant table. The caller must hold the grant table's +* write lock before calling this function. +*/ int gnttab_grow_table (struct domain *d,unsigned int req_nr_frames) { -/* d's grant table lock must be held by the caller */ +/* d's grant table write lock must be held by the caller */ struct grant_table *gt = d->grant_table; -unsigned int i; +unsigned int i, j;ASSERT (req_nr_frames <= max_grant_frames);
@@ -1311,6 +1312,8 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)if ( (gt->active[i] =alloc_xenheap_page ()) ==NULL ) goto active_alloc_failed;clear_page (gt->active[i]); +for ( j =0 ; j <ACGNT_PER_PAGE ; j++ ) +spin_lock_init (>->active[i][j].lock); }/* Shared */
@@ -1806,7 +1809,7 @@ __release_grant_for_copy(spin_lock (&rgt->lock); - act = &active_entry (rgt, gref); + act =active_entry_acquire (rgt, gref); sha =shared_entry_header (rgt, gref); r_frame = act->frame;
@@ -1845,6 +1848,7 @@ __release_grant_for_copy( released_read =1 ; } +active_entry_release (act);spin_unlock (&rgt->lock);if ( td != rd )
@@ -1906,14 +1910,14 @@ __acquire_grant_for_copy(spin_lock (&rgt->lock);if ( rgt->gt_version ==0 ) -PIN_FAIL (unlock_out, GNTST_general_error, +PIN_FAIL (gnt_unlock_out, GNTST_general_error,"remote grant table not ready\n" );if (unlikely (gref >=nr_grant_entries (rgt)) ) -PIN_FAIL (unlock_out, GNTST_bad_gntref, +PIN_FAIL (gnt_unlock_out, GNTST_bad_gntref,"Bad grant reference %ld\n" , gref); - act = &active_entry (rgt, gref); + act =active_entry_acquire (rgt, gref); shah =shared_entry_header (rgt, gref);if ( rgt->gt_version ==1 ) {
@@ -1972,6 +1976,13 @@ __acquire_grant_for_copy(PIN_FAIL (unlock_out_clear, GNTST_general_error,"transitive grant referenced bad domain %d\n" , trans_domid); + +/* +* __acquire_grant_for_copy() could take the read lock on +* the right table (if rd == td), so we have to drop the +* lock here and reacquire +*/ +active_entry_release (act);spin_unlock (&rgt->lock); rc = __acquire_grant_for_copy(td, trans_gref, rd->domain_id,
@@ -1979,10 +1990,13 @@ __acquire_grant_for_copy( &trans_page_off, &trans_length, 0);spin_lock (&rgt->lock); + act =active_entry_acquire (rgt, gref); +if ( rc != GNTST_okay ) {__fixup_status_for_copy_pin (act, status); -rcu_unlock_domain (td); +active_entry_release (act);spin_unlock (&rgt->lock); +rcu_unlock_domain (td);return rc; }
@@ -1994,6 +2008,7 @@ __acquire_grant_for_copy( {__fixup_status_for_copy_pin (act, status);rcu_unlock_domain (td); +active_entry_release (act);spin_unlock (&rgt->lock);put_page (*page); return __acquire_grant_for_copy(rd, gref, ldom, readonly,
@@ -2062,6 +2077,7 @@ __acquire_grant_for_copy( *length = act->length; *frame = act->frame; +active_entry_release (act);spin_unlock (&rgt->lock);return rc;
@@ -2074,7 +2090,11 @@ __acquire_grant_for_copy(gnttab_clear_flag (_GTF_reading, status); unlock_out: +active_entry_release (act); + + gnt_unlock_out:spin_unlock (&rgt->lock); +return rc; }
@@ -2399,16 +2419,18 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop) {for ( i =GNTTAB_NR_RESERVED_ENTRIES ; i <nr_grant_entries (gt); i++ ) { - act = &active_entry (gt, i); + act =active_entry_acquire (gt, i);if ( act->pin !=0 ) {gdprintk (XENLOG_WARNING ,"tried to change grant table version from %d to %d, but some grant entries still in use\n" , gt->gt_version, op.version); +active_entry_release (act); res = -EBUSY ; goto out_unlock; } +active_entry_release (act); } }
@@ -2587,9 +2609,17 @@ __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b) {struct domain *d =rcu_lock_current_domain ();struct grant_table *gt = d->grant_table; -struct active_grant_entry *act; +struct active_grant_entry *act_a =NULL ; +struct active_grant_entry *act_b =NULL ;s16 rc = GNTST_okay; +if ( ref_a == ref_b ) +/* +* noop, so avoid acquiring the same active entry +* twice which would case a deadlock. +*/ +return rc; +spin_lock (>->lock);/* Bounds check on the grant refs */
@@ -2598,12 +2628,12 @@ __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)if (unlikely (ref_b >=nr_grant_entries (d->grant_table)))PIN_FAIL (out, GNTST_bad_gntref,"Bad ref-b (%d).\n" , ref_b); - act = &active_entry (gt, ref_a); -if ( act->pin ) + act_a =active_entry_acquire (gt, ref_a); +if ( act_a->pin )PIN_FAIL (out, GNTST_eagain,"ref a %ld busy\n" , (long )ref_a); - act = &active_entry (gt, ref_b); -if ( act->pin ) + act_b =active_entry_acquire (gt, ref_b); +if ( act_b->pin )PIN_FAIL (out, GNTST_eagain,"ref b %ld busy\n" , (long )ref_b);if ( gt->gt_version ==1 )
@@ -2630,6 +2660,10 @@ __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b) } out: +if ( act_b !=NULL ) +active_entry_release (act_b); +if ( act_a !=NULL ) +active_entry_release (act_a);spin_unlock (>->lock);rcu_unlock_domain (d);
@@ -2939,7 +2973,7 @@ grant_table_create( struct domain *d) {struct grant_table *t; -int i; +int i, j; if ( (t = xzalloc(struct grant_table)) == NULL ) goto no_mem_0;
@@ -2958,6 +2992,8 @@ grant_table_create(if ( (t->active[i] =alloc_xenheap_page ()) ==NULL ) goto no_mem_2;clear_page (t->active[i]); +for ( j =0 ; j <ACGNT_PER_PAGE ; j++ ) +spin_lock_init (&t->active[i][j].lock); }/* Tracking of mapped foreign frames table */
@@ -3054,7 +3090,7 @@ gnttab_release_mappings( rgt = rd->grant_table;spin_lock (&rgt->lock); - act = &active_entry (rgt, ref); + act =active_entry_acquire (rgt, ref); sha =shared_entry_header (rgt, ref);if (rgt->gt_version ==1 ) status = &sha->flags;
@@ -3112,6 +3148,7 @@ gnttab_release_mappings(if ( act->pin ==0 )gnttab_clear_flag (_GTF_reading, status); +active_entry_release (act);spin_unlock (&rgt->lock);rcu_unlock_domain (rd);
@@ -3130,7 +3167,9 @@ grant_table_destroy(if ( t ==NULL )return ; - + +spin_lock (&t->lock); +for ( i =0 ; i <nr_grant_frames (t); i++ )free_xenheap_page (t->shared_raw[i]);xfree (t->shared_raw);
@@ -3147,6 +3186,8 @@ grant_table_destroy(free_xenheap_page (t->status[i]);xfree (t->status); +spin_unlock (&t->lock); +xfree (t); d->grant_table =NULL ; }
@@ -3174,9 +3215,12 @@ static void gnttab_usage_print(struct domain *rd)uint16_t status;uint64_t frame; - act = &active_entry (gt, ref); + act =active_entry_acquire (gt, ref);if ( !act->pin ) + { +active_entry_release (act);continue ; + } sha =shared_entry_header (gt, ref);
@@ -3206,6 +3250,7 @@ static void gnttab_usage_print(struct domain *rd)printk ("[%3d] %5d 0x%06lx 0x%08x %5d 0x%06" PRIx64" 0x%02x\n" , ref, act->domid, act->frame, act->pin, sha->domid, frame, status); +active_entry_release (act); } out:
Signature · 1 line
1.7.10.4
Re: [PATCHv6 2/5] gnttab: introduce per-active entry locks
xen/common/grant_table.c static void mapcount( · line 545
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
xen/common/grant_table.c +1
-0@@ -548,11 +542,14 @@ static void mapcount(for ( handle =0 ; handle < lgt->maptrack_limit; handle++ ) { +struct active_grant_entry *act;
const please (wherever possible, to document the intent to not modify).
xen/common/grant_table.c static void mapcount( · line 552
Quoted 3 lines
xen/common/grant_table.c +2
-1-if (active_entry (rd->grant_table, map->ref).frame == mfn ) + act = &_active_entry (rd->grant_table, map->ref); +if ( act->frame == mfn )
Or, as suggested before, avoid the intermediate variable in the first place.
Quoted 14 lines
xen/common/grant_table.c +0
-3@@ -698,12 +694,9 @@ __gnttab_map_grant_ref( GNTPIN_hstr_inc : GNTPIN_hstw_inc; frame = act->frame; - act_pin = act->pin; cache_flags = (shah->flags & (GTF_PAT |GTF_PWT |GTF_PCD ) ); -spin_unlock (&rgt->lock); -/* pg may be set, with a refcount included, from __get_paged_frame */ if ( !pg ) {
xen/common/grant_table.c __gnttab_map_grant_ref( · line 776
Quoted 8 lines
xen/common/grant_table.c +0
-2@@ -778,8 +771,6 @@ __gnttab_map_grant_ref( goto undo_out; } -double_gt_lock (lgt, rgt); -if (gnttab_need_iommu_mapping (ld) ) {unsigned int wrc, rdc;
I continue to think that using lgt unlocked here is wrong (i.e. while the next patch may fix this, it still represents intermediate breakage). At the very least this should be called out explicitly in the commit message, making it clear that this patch shouldn't be applied without the next one immediately following. But maybe the better approach would be to avoid the breakage in the first place by keeping double_gt_lock() in place until the next patch, moving a spin_unlock() right ahead of it to document the intentions, or even keeping that one in place too until the next patch.
Quoted 10 lines
xen/common/grant_table.c +3
-1@@ -939,18 +929,19 @@ __gnttab_unmap_common(TRACE_1D (TRC_MEM_PAGE_GRANT_UNMAP , dom); rgt = rd->grant_table; -double_gt_lock (lgt, rgt); + +spin_lock (&rgt->lock); + act =active_entry_acquire (rgt, op->map->ref); op->flags = op->map->flags;
xen/common/grant_table.c __gnttab_unmap_common( · line 953
Quoted 10 lines
xen/common/grant_table.c +1
-2if (unlikely (!op->flags) ||unlikely (op->map->domid != dom) ) {gdprintk (XENLOG_WARNING ,"Unstable handle %u\n" , op->handle); rc = GNTST_bad_handle; - goto unmap_out; + goto act_release_out; } op->rd = rd; - act = &active_entry (rgt, op->map->ref);
Why does this get moved up? "act" isn't need anywhere above.
xen/common/grant_table.c gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) · line 1295
Quoted 10 lines
xen/common/grant_table.c +4
-0@@ -1290,13 +1287,17 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) gt->nr_status_frames =0 ; } +/* +* Grow the grant table. The caller must hold the grant table's +* write lock before calling this function. +*/ int gnttab_grow_table (struct domain *d,unsigned int req_nr_frames)
The "write" in the comment appears to be stale.
xen/common/grant_table.c __acquire_grant_for_copy( · line 1985
Quoted 11 lines
xen/common/grant_table.c +7
-0@@ -1972,6 +1976,13 @@ __acquire_grant_for_copy(PIN_FAIL (unlock_out_clear, GNTST_general_error,"transitive grant referenced bad domain %d\n" , trans_domid); + +/* +* __acquire_grant_for_copy() could take the read lock on +* the right table (if rd == td), so we have to drop the +* lock here and reacquire +*/ +active_entry_release (act);
Same for the "read" here.
xen/common/grant_table.c grant_table_create( · line 2976
Quoted 6 lines
xen/common/grant_table.c +1
-1@@ -2939,7 +2973,7 @@ grant_table_create( struct domain *d) {struct grant_table *t; -int i; +int i, j;
Could you take the opportunity to make these unsigned? And perhaps also drop the odd looking blank padding?
Quoted 12 lines
xen/common/grant_table.c +3
-1@@ -3130,7 +3167,9 @@ grant_table_destroy(if ( t ==NULL )return ; - + +spin_lock (&t->lock); +for ( i =0 ; i <nr_grant_frames (t); i++ )free_xenheap_page (t->shared_raw[i]);xfree (t->shared_raw);
xen/common/grant_table.c grant_table_destroy( · line 3189
Quoted 4 lines
xen/common/grant_table.c +1
-0@@ -3147,6 +3186,8 @@ grant_table_destroy(free_xenheap_page (t->status[i]);xfree (t->status); +spin_unlock (&t->lock);
Iirc I asked this before - do you really think these are needed? Because again - if they are, that would seem to be a separate bug fix.
Jan
Re: [PATCHv6 2/5] gnttab: introduce per-active entry locks
Quoted 16 lines · Jan Beulich
Quoted 16 lines
xen/common/grant_table.c +3
-1@@ -3130,7 +3167,9 @@ grant_table_destroy(if ( t ==NULL )return ; - + +spin_lock (&t->lock); +for ( i =0 ; i <nr_grant_frames (t); i++ )free_xenheap_page (t->shared_raw[i]);xfree (t->shared_raw);@@ -3147,6 +3186,8 @@ grant_table_destroy(free_xenheap_page (t->status[i]);xfree (t->status);
Quoted 4 lines
xen/common/grant_table.c grant_table_destroy( · line 3189
Quoted 1 line
xen/common/grant_table.c +1
-0+spin_unlock (&t->lock);Iirc I asked this before - do you really think these are needed? Because again - if they are, that would seem to be a separate bug fix.
It looks unnecessary to me. Unless Matt can explain why he added it, I'll drop it.
David
PATCH v6 3/5 gnttab: split grant table lock into table and maptrack locks
In reply to David VrabelFrom: Matt Wilson <[email protected]>
The maptrack lock protects the maptrack state only.
---
Subsequent changes make both these locks uncontented. Is this patch really necessary? -- dvrabel
--- xen/common/grant_table.c | 33 ++++++++++++++++++++++++++++----- xen/include/xen/grant_table.h | 7 ++++++- 2 files changed, 34 insertions(+), 6 deletions(-)
xen/common/grant_table.c +28 -5
@@ -264,10 +264,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
@@ -279,7 +279,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 ) ) {
@@ -308,12 +308,15 @@ get_maptrack_handle( nr_frames + 1); } -spin_unlock (&lgt->lock); +spin_unlock (&lgt->maptrack_lock);return handle; } -/* Number of grant table entries. Caller must hold d's grant table lock. */ +/* +* Number of grant table entries. Caller must hold d's grant table +* read lock. +*/ static unsigned int nr_grant_entries (struct grant_table *gt) {ASSERT (gt->gt_version !=0 );
@@ -531,6 +534,13 @@ static int grant_map_exists(const struct domain *ld,return -EINVAL ; } +/* +* Count the number of mapped ro or rw entries tracked in the left +* grant table given a provided mfn provided by a foreign domain. +* +* This function takes the maptrack lock from the left grant table and +* must be called with the right grant table's rwlock held. +*/ static void mapcount (struct grant_table *lgt,struct domain *rd,unsigned long mfn,unsigned int *wrc,unsigned int *rdc)
@@ -540,6 +550,16 @@ static void mapcount( *wrc = *rdc =0 ; +/* +* While taking the local maptrack spinlock prevents any mapping +* changes, the remote active entries could be changing while we +* are counting. The caller has to hold the grant table lock, or +* some other mechanism should be used to prevent concurrent +* changes during this operation. +*/ +ASSERT (spin_is_locked (&rd->grant_table->lock)); +spin_lock (&lgt->maptrack_lock); +for ( handle =0 ; handle < lgt->maptrack_limit; handle++ ) {struct active_grant_entry *act;
@@ -552,6 +572,8 @@ static void mapcount(if ( act->frame == mfn ) (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++; } + +spin_unlock (&lgt->maptrack_lock); }/*
@@ -2980,6 +3002,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 +6 -1
@@ -82,7 +82,12 @@ struct grant_table {struct grant_mapping **maptrack;unsigned int maptrack_head;unsigned int maptrack_limit; -/* Lock protecting updates to active and shared grant tables. */ +/* Lock protecting the maptrack page list, head, and limit */ +spinlock_t maptrack_lock; +/* +* Lock protecting updates to grant table state (version, active +* entry list, etc.) +*/ spinlock_t lock;/* The defined versions are 1 and 2. Set to 0 if we don't know what version to use yet. */
Signature · 1 line
1.7.10.4
Re: [PATCHv6 3/5] gnttab: split grant table lock into table and maptrack locks
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
Subsequent changes make both these locks uncontented. Is this patch really necessary? -- dvrabel
Yeah, particularly if this ...
Quoted 18 lines
xen/common/grant_table.c +10
-0@@ -540,6 +550,16 @@ static void mapcount( *wrc = *rdc =0 ; +/* +* While taking the local maptrack spinlock prevents any mapping +* changes, the remote active entries could be changing while we +* are counting. The caller has to hold the grant table lock, or +* some other mechanism should be used to prevent concurrent +* changes during this operation. +*/ +ASSERT (spin_is_locked (&rd->grant_table->lock)); +spin_lock (&lgt->maptrack_lock); +for ( handle =0 ; handle < lgt->maptrack_limit; handle++ ) {struct active_grant_entry *act;
xen/common/grant_table.c static void mapcount( · line 577
Quoted 6 lines
xen/common/grant_table.c +2
-0@@ -552,6 +572,8 @@ static void mapcount(if ( act->frame == mfn ) (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++; } + +spin_unlock (&lgt->maptrack_lock); }
... is not resulting in contention anymore, I'd prefer avoiding a change like this without measurable benefit. And when still an issue, I'd wonder whether an rwlock wouldn't be the better choice here.
Quoted 4 lines
xen/include/xen/grant_table.h +0
-0@@ -82,7 +82,12 @@ struct grant_table {struct grant_mapping **maptrack;
xen/include/xen/grant_table.h struct grant_table { · line 91
Quoted 10 lines
xen/include/xen/grant_table.h +6
-1unsigned int maptrack_head;unsigned int maptrack_limit; -/* Lock protecting updates to active and shared grant tables. */ +/* Lock protecting the maptrack page list, head, and limit */ +spinlock_t maptrack_lock; +/* +* Lock protecting updates to grant table state (version, active +* entry list, etc.) +*/ spinlock_t lock;
If the patch still was to be applied, these two locks should be put on separate cache lines, to avoid unnecessary bouncing.
Jan
Re: [PATCHv6 3/5] gnttab: split grant table lock into table and maptrack locks
Quoted 10 lines · Jan Beulich
Quoted 10 lines
xen/include/xen/grant_table.h +2
-1@@ -82,7 +82,12 @@ struct grant_table {struct grant_mapping **maptrack;unsigned int maptrack_head;unsigned int maptrack_limit; -/* Lock protecting updates to active and shared grant tables. */ +/* Lock protecting the maptrack page list, head, and limit */ +spinlock_t maptrack_lock;
Quoted 8 lines
xen/include/xen/grant_table.h struct grant_table { · line 91
Quoted 5 lines
xen/include/xen/grant_table.h +4
-0+/* +* Lock protecting updates to grant table state (version, active +* entry list, etc.) +*/ spinlock_t lock;If the patch still was to be applied, these two locks should be put on separate cache lines, to avoid unnecessary bouncing.
I was shuffling the fields around to make this happen and I ended up shrinking the structure to fit it in a single cache line instead.
Shall I add some explicit padding to get the alignment you want? Note that with the per-CPU maptrack free lists the maptrack lock isn't heavily contended.
David
Re: [PATCHv6 3/5] gnttab: split grant table lock into table and maptrack locks
Quoted 20 lines
Quoted 1 line
Quoted 1 line
David Vrabel <[email protected]> 04/29/15 12:54 PM >>>
Quoted 9 lines · Jan Beulich
Quoted 9 lines
xen/include/xen/grant_table.h +2
-1@@ -82,7 +82,12 @@ struct grant_table {struct grant_mapping **maptrack;unsigned int maptrack_head;unsigned int maptrack_limit; -/* Lock protecting updates to active and shared grant tables. */ +/* Lock protecting the maptrack page list, head, and limit */ +spinlock_t maptrack_lock;Quoted 8 lines
xen/include/xen/grant_table.h struct grant_table { · line 91
Quoted 5 lines
xen/include/xen/grant_table.h +4
-0+/* +* Lock protecting updates to grant table state (version, active +* entry list, etc.) +*/ spinlock_t lock;If the patch still was to be applied, these two locks should be put on separate cache lines, to avoid unnecessary bouncing.
Quoted 6 lines
I was shuffling the fields around to make this happen and I ended up shrinking the structure to fit it in a single cache line instead.
Shall I add some explicit padding to get the alignment you want? Note that with the per-CPU maptrack free lists the maptrack lock isn't heavily contended.
Ah, right. In that case artificially bloating the structure is likely not worth it.
Jan
PATCH v6 4/5 gnttab: remove unnecessary grant table locks
In reply to David VrabelFrom: Malcolm Crossley <[email protected]>
The grant table lock is not required to protect reads of the table version, the active entry array, or the map track array.
This is safe because: a) the grant table version only changes once from 0 to 1 or 2; b) the active entry array only grows; and c) the map track array only grows.
--- xen/common/grant_table.c | 76 +++++++++-------------------------------- xen/include/xen/grant_table.h | 8 ++--- 2 files changed, 21 insertions(+), 63 deletions(-)
xen/common/grant_table.c +17 -59
@@ -196,8 +196,6 @@ active_entry_acquire(struct grant_table *t, grant_ref_t e) {struct active_grant_entry *act; -ASSERT (spin_is_locked (&t->lock)); - act = &_active_entry (t, e);spin_lock (&act->lock);
@@ -647,7 +645,6 @@ __gnttab_map_grant_ref( } rgt = rd->grant_table; -spin_lock (&rgt->lock);if ( rgt->gt_version ==0 ) PIN_FAIL(unlock_out, GNTST_general_error,
@@ -827,7 +824,6 @@ __gnttab_map_grant_ref( mt->flags = op->flags;active_entry_release (act); -spin_unlock (&rgt->lock); op->dev_bus_addr = (u64 )frame <<PAGE_SHIFT ; op->handle = handle;
@@ -869,7 +865,6 @@ __gnttab_map_grant_ref(active_entry_release (act); unlock_out: -spin_unlock (&rgt->lock); op->status = rc;put_maptrack_handle (lgt, handle);rcu_unlock_domain (rd);
@@ -919,18 +914,15 @@ __gnttab_unmap_common( } op->map = &maptrack_entry (lgt, op->handle); -spin_lock (&lgt->lock);if (unlikely (!op->map->flags) ) { -spin_unlock (&lgt->lock);gdprintk (XENLOG_INFO ,"Zero flags for handle (%d).\n" , op->handle); op->status = GNTST_bad_handle;return ; } dom = op->map->domid; -spin_unlock (&lgt->lock);if (unlikely ((rd =rcu_lock_domain_by_id (dom)) ==NULL ) ) {
@@ -952,7 +944,6 @@ __gnttab_unmap_common( rgt = rd->grant_table; -spin_lock (&rgt->lock); act =active_entry_acquire (rgt, op->map->ref); op->flags = op->map->flags;
@@ -1023,7 +1014,6 @@ __gnttab_unmap_common( act_release_out:active_entry_release (act); -spin_unlock (&rgt->lock); op->status = rc;rcu_unlock_domain (rd);
@@ -1055,7 +1045,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)rcu_lock_domain (rd); rgt = rd->grant_table; -spin_lock (&rgt->lock);if ( rgt->gt_version ==0 ) goto unlock_out;
@@ -1122,8 +1111,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)active_entry_release (act); unlock_out: -spin_unlock (&rgt->lock); -if ( put_handle ) { op->map->flags =0 ;
@@ -1277,7 +1264,7 @@ gnttab_populate_status_frames(struct domain *d, struct grant_table *gt,for ( i =nr_status_frames (gt); i < req_status_frames; i++ )gnttab_create_status_page (d, gt, i); - gt->nr_status_frames = req_status_frames; +atomic_set (>->nr_status_frames, req_status_frames);return 0 ;
@@ -1306,7 +1293,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)free_xenheap_page (gt->status[i]); gt->status[i] =NULL ; } - gt->nr_status_frames =0 ; +atomic_set (>->nr_status_frames,0 ); }/*
@@ -1356,7 +1343,7 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)/* Share the new shared frames with the recipient domain */ for ( i =nr_grant_frames (gt); i < req_nr_frames; i++ )gnttab_create_shared_page (d, gt, i); - gt->nr_grant_frames = req_nr_frames; +atomic_set (>->nr_grant_frames, req_nr_frames);return 1 ;
@@ -1423,7 +1410,17 @@ gnttab_setup_table( } gt = d->grant_table; -spin_lock (>->lock); + +/* Tracking of mapped foreign frames table */ + if ( (gt->maptrack = xzalloc_array(struct grant_mapping *, + max_maptrack_frames * d->max_vcpus)) ==NULL ) + goto out2; +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 ;
@@ -1437,7 +1434,7 @@ gnttab_setup_table("Expand grant table to %u failed. Current: %u Max: %u\n" , op.nr_frames, nr_grant_frames(gt), max_grant_frames); op.status = GNTST_general_error; - goto out3; + goto out2; } op.status = GNTST_okay;
@@ -1450,8 +1447,6 @@ gnttab_setup_table( op.status = GNTST_bad_virt_addr; } - out3: -spin_unlock (>->lock); out2:rcu_unlock_domain (d); out1:
@@ -1525,8 +1520,6 @@ gnttab_prepare_for_transfer(union grant_combo scombo, prev_scombo, new_scombo;int retries =0 ; -spin_lock (&rgt->lock); -if ( rgt->gt_version ==0 ) { gdprintk(XENLOG_INFO,
@@ -1575,12 +1568,9 @@ gnttab_prepare_for_transfer( scombo = prev_scombo; } - -spin_unlock (&rgt->lock);return 1 ; fail: -spin_unlock (&rgt->lock);return 0 ; }
@@ -1773,8 +1763,6 @@ 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); -if ( e->grant_table->gt_version ==1 ) {grant_entry_v1_t *sha = &shared_entry_v1 (e->grant_table, gop.ref);
@@ -1791,8 +1779,6 @@ gnttab_transfer(shared_entry_header (e->grant_table, gop.ref)->flags |= GTF_transfer_completed; -spin_unlock (&e->grant_table->lock); -rcu_unlock_domain (e); gop.status = GNTST_okay;
@@ -1829,8 +1815,6 @@ __release_grant_for_copy( released_read =0 ; released_write =0 ; -spin_lock (&rgt->lock); - act =active_entry_acquire (rgt, gref); sha =shared_entry_header (rgt, gref); r_frame = act->frame;
@@ -1871,7 +1855,6 @@ __release_grant_for_copy( }active_entry_release (act); -spin_unlock (&rgt->lock);if ( td != rd ) {
@@ -1929,8 +1912,6 @@ __acquire_grant_for_copy( *page =NULL ; -spin_lock (&rgt->lock); -if ( rgt->gt_version ==0 )PIN_FAIL (gnt_unlock_out, GNTST_general_error,"remote grant table not ready\n" );
@@ -2005,19 +1986,16 @@ __acquire_grant_for_copy(* lock here and reacquire */ active_entry_release (act); -spin_unlock (&rgt->lock); rc =__acquire_grant_for_copy (td, trans_gref, rd->domain_id, readonly, &grant_frame, page, &trans_page_off, &trans_length,0 ); -spin_lock (&rgt->lock); act =active_entry_acquire (rgt, gref);if ( rc != GNTST_okay ) {__fixup_status_for_copy_pin (act, status);active_entry_release (act); -spin_unlock (&rgt->lock);rcu_unlock_domain (td);return rc; }
@@ -2031,7 +2009,6 @@ __acquire_grant_for_copy(__fixup_status_for_copy_pin (act, status);rcu_unlock_domain (td);active_entry_release (act); -spin_unlock (&rgt->lock);put_page (*page); return __acquire_grant_for_copy(rd, gref, ldom, readonly, frame, page, page_off, length,
@@ -2100,7 +2077,6 @@ __acquire_grant_for_copy( *frame = act->frame;active_entry_release (act); -spin_unlock (&rgt->lock);return rc; unlock_out_clear:
@@ -2115,8 +2091,6 @@ __acquire_grant_for_copy(active_entry_release (act); gnt_unlock_out: -spin_unlock (&rgt->lock); -return rc; }
@@ -2432,7 +2406,6 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)if ( gt->gt_version == op.version ) goto out; -spin_lock (>->lock);/* Make sure that the grant table isn't currently in use when we change the version number, except for the first 8 entries which are allowed to be in use (xenstore/xenconsole keeps them mapped).
@@ -2642,8 +2615,6 @@ __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)*/ return rc; -spin_lock (>->lock); -/* Bounds check on the grant refs */ if (unlikely (ref_a >=nr_grant_entries (d->grant_table)))PIN_FAIL (out, GNTST_bad_gntref,"Bad ref-a (%d).\n" , ref_a);
@@ -2686,7 +2657,6 @@ out:active_entry_release (act_b);if ( act_a !=NULL )active_entry_release (act_a); -spin_unlock (>->lock);rcu_unlock_domain (d);
@@ -2757,12 +2727,9 @@ static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush,if ( d != owner ) { -spin_lock (&owner->grant_table->lock); - ret =grant_map_exists (d, owner->grant_table, mfn, ref_count);if ( ret !=0 ) { -spin_unlock (&owner->grant_table->lock);rcu_unlock_domain (d);put_page (page);return ret;
@@ -2781,8 +2748,6 @@ static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush,else ret =0 ; -if ( d != owner ) -spin_unlock (&owner->grant_table->lock);unmap_domain_page (v);put_page (page);
@@ -3003,7 +2968,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 ; +atomic_set (&t->nr_grant_frames,INITIAL_NR_GRANT_FRAMES );/* Active grant table. */ if ( (t->active = xzalloc_array(struct active_grant_entry *,
@@ -3050,7 +3015,7 @@ grant_table_create(for ( i =0 ; i <INITIAL_NR_GRANT_FRAMES ; i++ )gnttab_create_shared_page (d, t, i); - t->nr_status_frames =0 ; +atomic_set (&t->nr_status_frames,0 );/* Okay, install the structure. */ d->grant_table = t;
@@ -3111,8 +3076,6 @@ gnttab_release_mappings( } rgt = rd->grant_table; -spin_lock (&rgt->lock); - act =active_entry_acquire (rgt, ref); sha =shared_entry_header (rgt, ref);if (rgt->gt_version ==1 )
@@ -3172,7 +3135,6 @@ gnttab_release_mappings(gnttab_clear_flag (_GTF_reading, status);active_entry_release (act); -spin_unlock (&rgt->lock);rcu_unlock_domain (rd);
@@ -3224,8 +3186,6 @@ static void gnttab_usage_print(struct domain *rd)printk (" -------- active -------- -------- shared --------\n" );printk ("[ref] localdom mfn pin localdom gmfn flags\n" ); -spin_lock (>->lock); -if ( gt->gt_version ==0 ) goto out;
@@ -3277,8 +3237,6 @@ static void gnttab_usage_print(struct domain *rd) } out: -spin_unlock (>->lock); -if ( first )printk ("grant-table for remote domain:%5d ... " "no active grant table entries\n" , rd->domain_id);
xen/include/xen/grant_table.h +4 -4
@@ -65,7 +65,7 @@ struct grant_mapping {/* Per-domain grant information. */ struct grant_table {/* Table size. Number of frames shared with guest */ -unsigned int nr_grant_frames; +atomic_t nr_grant_frames;/* Shared grant table (see include/public/grant_table.h). */ union {void **shared_raw;
@@ -73,7 +73,7 @@ struct grant_table {union grant_entry_v2 **shared_v2; };/* Number of grant status frames shared with guest (for version 2) */ -unsigned int nr_status_frames; +atomic_t nr_status_frames;/* State grant table (see include/public/grant_table.h). */ grant_status_t **status;/* Active grant table. */
@@ -114,13 +114,13 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames);/* Number of grant table frames. Caller must hold d's grant table lock. */ static inline unsigned int nr_grant_frames (struct grant_table *gt) { -return gt->nr_grant_frames; +return atomic_read (>->nr_grant_frames); }/* Number of status grant table frames. Caller must hold d's gr. table lock.*/ static inline unsigned int nr_status_frames (struct grant_table *gt) { -return gt->nr_status_frames; +return atomic_read (>->nr_status_frames); }#define GRANT_STATUS_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
Signature · 1 line
1.7.10.4
Re: [PATCHv6 4/5] gnttab: remove unnecessary grant table locks
xen/common/grant_table.c __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b) · line 2646
Quoted 6 lines · David Vrabel
xen/common/grant_table.c +0
-2@@ -2642,8 +2615,6 @@ __gnttab_swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)*/ return rc; -spin_lock (>->lock); -
Now that this lock is gone, is anything stopping us racing against another CPU with ref_a and ref_b the other way around?
Cheers,
Tim.
Re: [PATCHv6 4/5] gnttab: remove unnecessary grant table locks
In reply to David VrabelQuoted 3 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
This is safe because: a) the grant table version only changes once from 0 to 1 or 2;
gnttab_set_version() also allows it to transition between 1 and 2 afaics.
Quoted 6 lines
xen/common/grant_table.c +0
-1@@ -919,18 +914,15 @@ __gnttab_unmap_common( } op->map = &maptrack_entry (lgt, op->handle); -spin_lock (&lgt->lock);
xen/common/grant_table.c __gnttab_unmap_common( · line 933
Quoted 10 lines
xen/common/grant_table.c +0
-2if (unlikely (!op->map->flags) ) { -spin_unlock (&lgt->lock);gdprintk (XENLOG_INFO ,"Zero flags for handle (%d).\n" , op->handle); op->status = GNTST_bad_handle;return ; } dom = op->map->domid; -spin_unlock (&lgt->lock);
Don't you need a lock here to see a consistent op->map->{dom,ref,flags} tuple (with the writing side also holding a lock while updating them)?
xen/common/grant_table.c __gnttab_unmap_common( · line 949
Quoted 8 lines
xen/common/grant_table.c +0
-1@@ -952,7 +944,6 @@ __gnttab_unmap_common( rgt = rd->grant_table; -spin_lock (&rgt->lock); act =active_entry_acquire (rgt, op->map->ref); op->flags = op->map->flags;
Same here then.
xen/common/grant_table.c gnttab_setup_table( · line 1417
Quoted 10 lines
xen/common/grant_table.c +5
-1@@ -1423,7 +1410,17 @@ gnttab_setup_table( } gt = d->grant_table; -spin_lock (>->lock); + +/* Tracking of mapped foreign frames table */ + if ( (gt->maptrack = xzalloc_array(struct grant_mapping *, + max_maptrack_frames * d->max_vcpus)) ==NULL ) + goto out2;
This provides no error indication to the caller. And is this allocation guaranteed to be no larger than a page? Finally - does this belong here (and not instead into the last patch)?
Quoted 4 lines
xen/include/xen/grant_table.h +0
-0@@ -114,13 +114,13 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames);/* Number of grant table frames. Caller must hold d's grant table lock. */ static inline unsigned int nr_grant_frames (struct grant_table *gt) {
xen/include/xen/grant_table.h gnttab_grow_table(struct domain *d, unsigned int req_nr_frames); · line 124
Quoted 10 lines
xen/include/xen/grant_table.h +2
-2-return gt->nr_grant_frames; +return atomic_read (>->nr_grant_frames); }/* Number of status grant table frames. Caller must hold d's gr. table lock.*/ static inline unsigned int nr_status_frames (struct grant_table *gt) { -return gt->nr_status_frames; +return atomic_read (>->nr_status_frames); }
Both comments need changing.
Jan
PATCH v6 5/5 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).
A domain's maptrack limit is multiplied by the number of VCPUs. This ensures that a worst case domain that only performs grant table operations via one VCPU will not see a lower map track limit.
--- xen/common/grant_table.c | 129 ++++++++++++++++++++++++----------------- xen/include/xen/grant_table.h | 8 ++- xen/include/xen/sched.h | 5 ++ 3 files changed, 86 insertions(+), 56 deletions(-)
xen/common/grant_table.c +76 -53
@@ -118,9 +118,9 @@ struct gnttab_unmap_common { ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])static inline unsigned int -nr_maptrack_frames (struct grant_table *t) +nr_vcpu_maptrack_frames (struct vcpu *v) { -return t->maptrack_limit /MAPTRACK_PER_PAGE ; +return v->maptrack_limit /MAPTRACK_PER_PAGE ; }#define MAPTRACK_TAIL (~0u)
@@ -249,66 +249,91 @@ static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pagstatic 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; + + head = v->maptrack_head; +if (unlikely (head ==MAPTRACK_TAIL ) ) +return -1 ; + + next =maptrack_entry (t, head).ref; +if (unlikely (next ==MAPTRACK_TAIL ) )return -1 ; - t->maptrack_head =maptrack_entry (t, h).ref; -return h; + + 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; +u32 prev_tail, cur_tail; + +/* Set current hande to have TAIL reference */ +maptrack_entry (t, handle).ref =MAPTRACK_TAIL ; + v = d->vcpu[maptrack_entry (t,handle).vcpu]; + cur_tail = v->maptrack_tail; +do { + prev_tail = cur_tail; + cur_tail =cmpxchg ((u32 *)&v->maptrack_tail, prev_tail, handle); + }while ( cur_tail != prev_tail ); +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); - -while (unlikely ((handle =__get_maptrack_handle (lgt)) ==-1 ) ) - { - nr_frames =nr_maptrack_frames (lgt); -if ( nr_frames >= max_maptrack_frames ) -break ; - - new_mt =alloc_xenheap_page (); -if ( !new_mt ) -break ; + handle =__get_maptrack_handle (lgt, v); +if (likely (handle !=-1 ) ) +return handle; -clear_page (new_mt); + nr_frames =nr_vcpu_maptrack_frames (v); +if ( nr_frames >= max_maptrack_frames ) +return -1 ; - new_mt_limit = lgt->maptrack_limit +MAPTRACK_PER_PAGE ; + new_mt =alloc_xenheap_page (); +if ( !new_mt ) +return -1 ; -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; +clear_page (new_mt); - lgt->maptrack[nr_frames] = new_mt; -smp_wmb (); - lgt->maptrack_limit = new_mt_limit; + new_mt_limit = v->maptrack_limit +MAPTRACK_PER_PAGE ; -gdprintk (XENLOG_INFO ,"Increased maptrack size to %u frames\n" , - nr_frames +1 ); +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; + v->maptrack_head = lgt->maptrack_pages *MAPTRACK_PER_PAGE ; +if (v->maptrack_tail ==MAPTRACK_TAIL ) + { + v->maptrack_tail = (lgt->maptrack_pages *MAPTRACK_PER_PAGE ) + +MAPTRACK_PER_PAGE -1 ; + new_mt[i -1 ].ref =MAPTRACK_TAIL ; } +spin_lock (&lgt->maptrack_lock); + lgt->maptrack[lgt->maptrack_pages++] = new_mt;spin_unlock (&lgt->maptrack_lock); -return handle; + v->maptrack_limit = new_mt_limit; + +return __get_maptrack_handle (lgt, v); }/*
@@ -558,7 +583,7 @@ static void mapcount(ASSERT (spin_is_locked (&rd->grant_table->lock));spin_lock (&lgt->maptrack_lock); -for ( handle =0 ; handle < lgt->maptrack_limit; handle++ ) +for ( handle =0 ; handle < lgt->maptrack_pages *MAPTRACK_PER_PAGE ; handle++ ) {struct active_grant_entry *act;
@@ -906,7 +931,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;
@@ -1371,6 +1396,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;
@@ -1422,6 +1448,17 @@ gnttab_setup_table( } gt->maptrack_pages =0 ; +/* Tracking of mapped foreign frames table */ + if ( (gt->maptrack = xzalloc_array(struct grant_mapping *, + max_maptrack_frames * d->max_vcpus)) ==NULL ) + goto out2; +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 ;
@@ -2984,18 +3021,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;
@@ -3026,12 +3051,10 @@ 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++ )free_xenheap_page (t->active[i]); + no_mem_2:xfree (t->active); no_mem_1:xfree (t);
@@ -3055,7 +3078,7 @@ 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)) )
@@ -3159,7 +3182,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 */ +u16 pad[2 ]; };/* Per-domain grant information. */
@@ -78,10 +80,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;/*
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_limit; +/* 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: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
Quoted 57 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
static inline int
get_maptrack_handle(
structgrant_table*lgt)
{xen/common/grant_table.c +20
-22+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); - -while (unlikely ((handle =__get_maptrack_handle (lgt)) ==-1 ) ) - { - nr_frames =nr_maptrack_frames (lgt); -if ( nr_frames >= max_maptrack_frames ) -break ; - - new_mt =alloc_xenheap_page (); -if ( !new_mt ) -break ; + handle =__get_maptrack_handle (lgt, v); +if (likely (handle !=-1 ) ) +return handle; -clear_page (new_mt); + nr_frames =nr_vcpu_maptrack_frames (v); +if ( nr_frames >= max_maptrack_frames ) +return -1 ; - new_mt_limit = lgt->maptrack_limit +MAPTRACK_PER_PAGE ; + new_mt =alloc_xenheap_page (); +if ( !new_mt ) +return -1 ; -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; +clear_page (new_mt); - lgt->maptrack[nr_frames] = new_mt; -smp_wmb (); - lgt->maptrack_limit = new_mt_limit; + new_mt_limit = v->maptrack_limit +MAPTRACK_PER_PAGE ; -gdprintk (XENLOG_INFO ,"Increased maptrack size to %u frames\n" , - nr_frames +1 ); +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; + v->maptrack_head = lgt->maptrack_pages *MAPTRACK_PER_PAGE ;
xen/common/grant_table.c static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pag · line 332
Quoted 10 lines
xen/common/grant_table.c +7
-0+if (v->maptrack_tail ==MAPTRACK_TAIL ) + { + v->maptrack_tail = (lgt->maptrack_pages *MAPTRACK_PER_PAGE ) + +MAPTRACK_PER_PAGE -1 ; + new_mt[i -1 ].ref =MAPTRACK_TAIL ; } +spin_lock (&lgt->maptrack_lock); + lgt->maptrack[lgt->maptrack_pages++] = new_mt;spin_unlock (&lgt->maptrack_lock);
The uses of ->maptrack_pages ahead of taking the lock can race with updates inside the lock. And with locking elsewhere dropped by the previous patch it looks like you can't update ->maptrack[] like you do (you'd need a barrier between the pointer store and the increment, and with that I think the lock would become superfluous if it was needed only for this update).
Also note the coding style issues in the changes above^(and also in code further down).
xen/common/grant_table.c static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pag · line 336
Quoted 4 lines
xen/common/grant_table.c +3
-1-return handle; + v->maptrack_limit = new_mt_limit; + +return __get_maptrack_handle (lgt, v);
With the lock dropped, nothing guarantees this to succeed, which it ought to unless the table size reached its allowed maximum.
xen/common/grant_table.c gnttab_setup_table( · line 1454
Quoted 8 lines
xen/common/grant_table.c +4
-0@@ -1422,6 +1448,17 @@ gnttab_setup_table( } gt->maptrack_pages =0 ; +/* Tracking of mapped foreign frames table */ + if ( (gt->maptrack = xzalloc_array(struct grant_mapping *, + max_maptrack_frames * d->max_vcpus)) ==NULL ) + goto out2;
See the comments on the similar misplaced hunk in the previous patch.
xen/include/xen/grant_table.h struct grant_mapping { · line 65
Quoted 9 lines
xen/include/xen/grant_table.h +2
-0@@ -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 */ +u16 pad[2 ]; };
What is this pad[] good for?
Jan
Re: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
Quoted 14 lines · Jan Beulich
xen/common/grant_table.c static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pag · line 332
Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
xen/common/grant_table.c +8
-0+ v->maptrack_head = lgt->maptrack_pages *MAPTRACK_PER_PAGE ; +if (v->maptrack_tail ==MAPTRACK_TAIL ) + { + v->maptrack_tail = (lgt->maptrack_pages *MAPTRACK_PER_PAGE ) + +MAPTRACK_PER_PAGE -1 ; + new_mt[i -1 ].ref =MAPTRACK_TAIL ; } +spin_lock (&lgt->maptrack_lock); + lgt->maptrack[lgt->maptrack_pages++] = new_mt;spin_unlock (&lgt->maptrack_lock);
Quoted 6 lines
The uses of ->maptrack_pages ahead of taking the lock can race with updates inside the lock. And with locking elsewhere dropped by the previous patch it looks like you can't update ->maptrack[] like you do (you'd need a barrier between the pointer store and the increment, and with that I think the lock would become superfluous if it was needed only for this update).
This was my mistake. Malcolm's original patch had correct locking here.
Quoted 10 lines
Also note the coding style issues in the changes above^(and also in code further down).
xen/common/grant_table.c static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pag · line 336
Quoted 4 lines
xen/common/grant_table.c +3
-1-return handle; + v->maptrack_limit = new_mt_limit; + +return __get_maptrack_handle (lgt, v);With the lock dropped, nothing guarantees this to succeed, which it ought to unless the table size reached its allowed maximum.
We've just added a new page of handles for this VCPU. This will succeed.
David
Re: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 23.04.15 at 18:29, <[email protected]> wrote:
Quoted 6 lines · Jan Beulich
Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
xen/common/grant_table.c +3
-1-return handle; + v->maptrack_limit = new_mt_limit; + +return __get_maptrack_handle (lgt, v);
Quoted 4 lines
Quoted 2 lines
With the lock dropped, nothing guarantees this to succeed, which it ought to unless the table size reached its allowed maximum.
We've just added a new page of handles for this VCPU. This will succeed.
Ah, right, this is per-vCPU now.
Jan
Re: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
In reply to Jan BeulichQuoted 75 lines · Jan Beulich
Quoted 57 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
static inline int
get_maptrack_handle(
structgrant_table*lgt)
{xen/common/grant_table.c +20
-22+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); - -while (unlikely ((handle =__get_maptrack_handle (lgt)) ==-1 ) ) - { - nr_frames =nr_maptrack_frames (lgt); -if ( nr_frames >= max_maptrack_frames ) -break ; - - new_mt =alloc_xenheap_page (); -if ( !new_mt ) -break ; + handle =__get_maptrack_handle (lgt, v); +if (likely (handle !=-1 ) ) +return handle; -clear_page (new_mt); + nr_frames =nr_vcpu_maptrack_frames (v); +if ( nr_frames >= max_maptrack_frames ) +return -1 ; - new_mt_limit = lgt->maptrack_limit +MAPTRACK_PER_PAGE ; + new_mt =alloc_xenheap_page (); +if ( !new_mt ) +return -1 ; -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; +clear_page (new_mt); - lgt->maptrack[nr_frames] = new_mt; -smp_wmb (); - lgt->maptrack_limit = new_mt_limit; + new_mt_limit = v->maptrack_limit +MAPTRACK_PER_PAGE ; -gdprintk (XENLOG_INFO ,"Increased maptrack size to %u frames\n" , - nr_frames +1 ); +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; + v->maptrack_head = lgt->maptrack_pages *MAPTRACK_PER_PAGE ;xen/common/grant_table.c static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pag · line 332
Quoted 10 lines
xen/common/grant_table.c +7
-0+if (v->maptrack_tail ==MAPTRACK_TAIL ) + { + v->maptrack_tail = (lgt->maptrack_pages *MAPTRACK_PER_PAGE ) + +MAPTRACK_PER_PAGE -1 ; + new_mt[i -1 ].ref =MAPTRACK_TAIL ; } +spin_lock (&lgt->maptrack_lock); + lgt->maptrack[lgt->maptrack_pages++] = new_mt;spin_unlock (&lgt->maptrack_lock);The uses of ->maptrack_pages ahead of taking the lock can race with updates inside the lock. And with locking elsewhere dropped by the previous patch it looks like you can't update ->maptrack[] like you do (you'd need a barrier between the pointer store and the increment, and with that I think the lock would become superfluous if it was needed only for this update).
Quoted 2 lines
Also note the coding style issues in the changes above^(and also in code further down).
This was a last minute optimisation, this isn't on the hot patch so we'll expand the spin_lock to cover all users of maptrack_pages.
Sorry about the coding style problems.
Quoted 21 lines
xen/common/grant_table.c static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pag · line 336
Quoted 5 lines
xen/common/grant_table.c +3
-1-return handle; + v->maptrack_limit = new_mt_limit; + +return __get_maptrack_handle (lgt, v);With the lock dropped, nothing guarantees this to succeed, which it ought to unless the table size reached its allowed maximum.
xen/common/grant_table.c gnttab_setup_table( · line 1454
Quoted 8 lines
xen/common/grant_table.c +4
-0@@ -1422,6 +1448,17 @@ gnttab_setup_table( } gt->maptrack_pages =0 ; +/* Tracking of mapped foreign frames table */ + if ( (gt->maptrack = xzalloc_array(struct grant_mapping *, + max_maptrack_frames * d->max_vcpus)) ==NULL ) + goto out2;See the comments on the similar misplaced hunk in the previous patch.
Quoted 11 lines
xen/include/xen/grant_table.h struct grant_mapping { · line 65
Quoted 9 lines
xen/include/xen/grant_table.h +2
-0@@ -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 */ +u16 pad[2 ]; };What is this pad[] good for?
The pad is to keep the struct power of 2 sized because this allows the compiler to optimise these macro's to right and left shifts:
#define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
#define maptrack_entry(t, e) \
((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])Malcolm
Quoted 3 lines
Jan
Re: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
Quoted 15 lines
Quoted 1 line
Quoted 1 line · On 24.04.15 at 11:09, <[email protected]> wrote:
Quoted 12 lines · Jan Beulich
xen/include/xen/grant_table.h struct grant_mapping { · line 65
Quoted 10 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
xen/include/xen/grant_table.h +2
-0@@ -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 */ +u16 pad[2 ]; };What is this pad[] good for?
Quoted 6 lines
The pad is to keep the struct power of 2 sized because this allows the compiler to optimise these macro's to right and left shifts:
#define
MAPTRACK_PER_PAGE(PAGE_SIZE/sizeof(struct grant_mapping))#define maptrack_entry(t, e) \ ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
Okay, then why u16[2] instead of u32? And please add a brief comment explaining the reason.
Apart from that I wonder whether fitting vcpu in the 10 unused flags bits (not 11, as the comment on the field suggests) would be an option. That would require limiting vCPU count to 4k, which I don't think would really be a problem for anyone.
Jan
Re: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
Quoted 13 lines · Jan Beulich
Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 24.04.15 at 11:09, <[email protected]> wrote:
Quoted 11 lines · Jan Beulich
xen/include/xen/grant_table.h struct grant_mapping { · line 65
Quoted 10 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
xen/include/xen/grant_table.h +2
-0@@ -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 */ +u16 pad[2 ]; };What is this pad[] good for?
Quoted 8 lines
Quoted 6 lines
The pad is to keep the struct power of 2 sized because this allows the compiler to optimise these macro's to right and left shifts:
#define
MAPTRACK_PER_PAGE(PAGE_SIZE/sizeof(struct grant_mapping))#define maptrack_entry(t, e) \ ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])Okay, then why u16[2] instead of u32? And please add a brief comment explaining the reason.
Most likely because vcpu was a u16 in the first draft, and got bumped during internal review.
Quoted 5 lines
Apart from that I wonder whether fitting vcpu in the 10 unused flags bits (not 11, as the comment on the field suggests) would be an option. That would require limiting vCPU count to 4k, which I don't think would really be a problem for anyone.
8k VCPU PV guests do function, and are very good at finding scalability limits.
It would be nice not to break this.
~Andrew
Re: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
In reply to Jan BeulichQuoted 22 lines · Jan Beulich
Quoted 15 lines
Quoted 1 line
Quoted 1 line · On 24.04.15 at 11:09, <[email protected]> wrote:
Quoted 12 lines · Jan Beulich
xen/include/xen/grant_table.h struct grant_mapping { · line 65
Quoted 10 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
xen/include/xen/grant_table.h +2
-0@@ -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 */ +u16 pad[2 ]; };What is this pad[] good for?
Quoted 6 lines
The pad is to keep the struct power of 2 sized because this allows the compiler to optimise these macro's to right and left shifts:
#define
MAPTRACK_PER_PAGE(PAGE_SIZE/sizeof(struct grant_mapping))#define maptrack_entry(t, e) \ ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
Quoted 7 lines
Okay, then why u16[2] instead of u32? And please add a brief comment explaining the reason.
Apart from that I wonder whether fitting vcpu in the 10 unused flags bits (not 11, as the comment on the field suggests) would be an option. That would require limiting vCPU count to 4k, which I don't think would really be a problem for anyone.
I'd like to not have the overhead of bit operations on a hot path.
We're going from 512 grant entries per page to 256 grant entries per page. This is taking the Xen memory overhead from 0.2% to 0.4% for grant mapped pages.
Is the extra 0.2% memory overhead that concerning?
Malcolm
Quoted 3 lines
Jan
Re: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
Quoted 32 lines
Quoted 1 line
Quoted 1 line · On 24.04.15 at 12:21, <[email protected]> wrote:
Quoted 22 lines · Jan Beulich
Quoted 15 lines
Quoted 1 line
Quoted 1 line · On 24.04.15 at 11:09, <[email protected]> wrote:
Quoted 12 lines · Jan Beulich
xen/include/xen/grant_table.h struct grant_mapping { · line 65
Quoted 10 lines
Quoted 1 line
Quoted 1 line · On 22.04.15 at 18:00, <[email protected]> wrote:
xen/include/xen/grant_table.h +2
-0@@ -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 */ +u16 pad[2 ]; };What is this pad[] good for?
Quoted 6 lines
The pad is to keep the struct power of 2 sized because this allows the compiler to optimise these macro's to right and left shifts:
#define
MAPTRACK_PER_PAGE(PAGE_SIZE/sizeof(struct grant_mapping))#define maptrack_entry(t, e) \ ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])Quoted 7 lines
Okay, then why u16[2] instead of u32? And please add a brief comment explaining the reason.
Apart from that I wonder whether fitting vcpu in the 10 unused flags bits (not 11, as the comment on the field suggests) would be an option. That would require limiting vCPU count to 4k, which I don't think would really be a problem for anyone.
Quoted 7 lines
I'd like to not have the overhead of bit operations on a hot path.
We're going from 512 grant entries per page to 256 grant entries per page. This is taking the Xen memory overhead from 0.2% to 0.4% for grant mapped pages.
Is the extra 0.2% memory overhead that concerning?
Perhaps not so much the memory overhead but the doubled cache bandwidth needed. But anyway, this was only a question, and with both you and Andrew being opposed let's just stay with what you submitted.
Jan