Re: [Xen-devel] [PATCHv6 2/3] grant_table: convert grant table rwlock to percpu rwlock
>>> On 22.01.16 at 14:41,wrote: > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -178,6 +178,8 @@ struct active_grant_entry { > #define _active_entry(t, e) \ > ((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE]) > > +DEFINE_PERCPU_RWLOCK_GLOBAL(grant_rwlock); > + > static inline void gnttab_flush_tlb(const struct domain *d) > { > if ( !paging_mode_external(d) ) > @@ -208,7 +210,13 @@ active_entry_acquire(struct grant_table *t, grant_ref_t > e) > { > struct active_grant_entry *act; > > -ASSERT(rw_is_locked(>lock)); > +/* > + * The grant table for the active entry should be locked but the > + * percpu rwlock cannot be checked for read lock without race conditions > + * or high overhead so we cannot use an ASSERT > + * > + * ASSERT(rw_is_locked(>lock)); > + */ There are a number of trailing blanks being added here (and further down), which I'm fixing up as I'm in the process of applying this. The reason I noticed though is that this hunk ... > @@ -660,7 +668,13 @@ static int grant_map_exists(const struct domain *ld, > { > unsigned int ref, max_iter; > > -ASSERT(rw_is_locked(>lock)); > +/* > + * The remote grant table should be locked but the percpu rwlock > + * cannot be checked for read lock without race conditions or high > + * overhead so we cannot use an ASSERT > + * > + * ASSERT(rw_is_locked(>lock)); > + */ > > max_iter = min(*ref_count + (1 << GNTTABOP_CONTINUATION_ARG_SHIFT), > nr_grant_entries(rgt)); ... doesn't apply at all due to being white space damaged (the line immediately preceding the ASSERT() which gets removed actually has four blanks on it in the source tree (which is wrong, but should nevertheless be reflected in your patch). Due to the other trailing whitespace found above I can also exclude the mail system to have eaten that white space on the way here, so I really wonder which tree this patch got created against. Considering the hassle with the first commit attempt yesterday, may I please ask that you apply a little more care? Thanks, Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 2/3] grant_table: convert grant table rwlock to percpu rwlock
On Fri, 2016-01-22 at 08:15 -0700, Jan Beulich wrote: > > There are a number of trailing blanks being added here (and further > down), which I'm fixing up as I'm in the process of applying this. Aside: Do you know about "git am --whitespace=fix" ? It automates the removal of trailing whitespace... Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCHv6 2/3] grant_table: convert grant table rwlock to percpu rwlock
The per domain grant table read lock suffers from significant contention when performance multi-queue block or network IO due to the parallel grant map/unmaps/copies occurring on the DomU's grant table. On multi-socket systems, the contention results in the locked compare swap operation failing frequently which results in a tight loop of retries of the compare swap operation. As the coherency fabric can only support a specific rate of compare swap operations for a particular data location then taking the read lock itself becomes a bottleneck for grant operations. Standard rwlock performance of a single VIF VM-VM transfer with 16 queues configured was limited to approximately 15 gbit/s on a 2 socket Haswell-EP host. Percpu rwlock performance with the same configuration is approximately 48 gbit/s. Oprofile was used to determine the initial overhead of the read-write locks and to confirm the overhead was dramatically reduced by the percpu rwlocks. Signed-off-by: Malcolm CrossleyAcked-by: Ian Campbell -- Changes since v5: - None Changes since v4: - Rename grant table rwlock wrappers and use grant table pointer as argument Changes since v3: - None Changes since v2: - Switched to using wrappers for taking percpu rwlock - Added percpu structure pointer to percpu rwlock initialisation - Added comment on removal of ASSERTS for grant table rw_is_locked() Changes since v1: - Used new macros provided in updated percpu rwlock v2 patch - Converted grant table rwlock_t to percpu_rwlock_t - Patched a missed grant table rwlock_t usage site --- xen/arch/arm/mm.c | 4 +- xen/arch/x86/mm.c | 4 +- xen/common/grant_table.c | 124 +++--- xen/include/xen/grant_table.h | 24 +++- 4 files changed, 96 insertions(+), 60 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 47bfb27..81f9e2e 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1055,7 +1055,7 @@ int xenmem_add_to_physmap_one( switch ( space ) { case XENMAPSPACE_grant_table: -write_lock(>grant_table->lock); +grant_write_lock(d->grant_table); if ( d->grant_table->gt_version == 0 ) d->grant_table->gt_version = 1; @@ -1085,7 +1085,7 @@ int xenmem_add_to_physmap_one( t = p2m_ram_rw; -write_unlock(>grant_table->lock); +grant_write_unlock(d->grant_table); break; case XENMAPSPACE_shared_info: if ( idx != 0 ) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index b81d1fd..a5a9b6f 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4671,7 +4671,7 @@ int xenmem_add_to_physmap_one( mfn = virt_to_mfn(d->shared_info); break; case XENMAPSPACE_grant_table: -write_lock(>grant_table->lock); +grant_write_lock(d->grant_table); if ( d->grant_table->gt_version == 0 ) d->grant_table->gt_version = 1; @@ -4693,7 +4693,7 @@ int xenmem_add_to_physmap_one( mfn = virt_to_mfn(d->grant_table->shared_raw[idx]); } -write_unlock(>grant_table->lock); +grant_write_unlock(d->grant_table); break; case XENMAPSPACE_gmfn_range: case XENMAPSPACE_gmfn: diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 5d52d1e..6a536d2 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -178,6 +178,8 @@ struct active_grant_entry { #define _active_entry(t, e) \ ((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE]) +DEFINE_PERCPU_RWLOCK_GLOBAL(grant_rwlock); + static inline void gnttab_flush_tlb(const struct domain *d) { if ( !paging_mode_external(d) ) @@ -208,7 +210,13 @@ active_entry_acquire(struct grant_table *t, grant_ref_t e) { struct active_grant_entry *act; -ASSERT(rw_is_locked(>lock)); +/* + * The grant table for the active entry should be locked but the + * percpu rwlock cannot be checked for read lock without race conditions + * or high overhead so we cannot use an ASSERT + * + * ASSERT(rw_is_locked(>lock)); + */ act = &_active_entry(t, e); spin_lock(>lock); @@ -270,23 +278,23 @@ double_gt_lock(struct grant_table *lgt, struct grant_table *rgt) */ if ( lgt < rgt ) { -write_lock(>lock); -write_lock(>lock); +grant_write_lock(lgt); +grant_write_lock(rgt); } else { if ( lgt != rgt ) -write_lock(>lock); -write_lock(>lock); +grant_write_lock(rgt); +grant_write_lock(lgt); } } static inline void double_gt_unlock(struct grant_table *lgt, struct grant_table *rgt) { -write_unlock(>lock); +grant_write_unlock(lgt); if ( lgt != rgt ) -write_unlock(>lock); +grant_write_unlock(rgt); } static inline int @@
Re: [Xen-devel] [PATCHv6 2/3] grant_table: convert grant table rwlock to percpu rwlock
>>> On 22.01.16 at 16:22,wrote: > On Fri, 2016-01-22 at 08:15 -0700, Jan Beulich wrote: >> >> There are a number of trailing blanks being added here (and further >> down), which I'm fixing up as I'm in the process of applying this. > > Aside: Do you know about "git am --whitespace=fix" ? It automates the > removal of trailing whitespace... No, I didn't, but it'd be maximally useful only if I could store this as the default into ~/.gitconfig (and maybe that's possible, just that my git foo is too lame). Besides that I'm also not always using "git am", not the least because what my mail frontend saves is not always compatible with that command (leading to lost metadata). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 2/3] grant_table: convert grant table rwlock to percpu rwlock
On Fri, 2016-01-22 at 08:34 -0700, Jan Beulich wrote: > >>> On 22.01.16 at 16:22,wrote: > > On Fri, 2016-01-22 at 08:15 -0700, Jan Beulich wrote: > >> > >> There are a number of trailing blanks being added here (and > further > >> down), which I'm fixing up as I'm in the process of applying this. > > > > Aside: Do you know about "git am --whitespace=fix" ? It automates > the > > removal of trailing whitespace... > > No, I didn't, but it'd be maximally useful only if I could store this > as > the default into ~/.gitconfig (and maybe that's possible, just that > my git foo is too lame). It ends up being a git apply option, so it looks like apply.whitespace = "fix" is the answer. I don't do that and tend to just rerun if git am complains about whitespace (which is the default) and it looks like it is worth fixing. Or is correct to fix since checked in patches have deliberate trailing whitepace on the context lines which you don't want to squash. IME trailing whitespace in patches is actually astonishingly rare in practice. > Besides that I'm also not always using "git > am", not the least because what my mail frontend saves is not > always compatible with that command (leading to lost metadata). I use a sneaky trick, which is that the bug tracker will serve up raw, unadulterated messages sent to xen-devel by message-id: curl --silent http://bugs.xenproject.org/xen/mid/ /raw Doesn't help if the patch didn't go to xen-devel, but most of the ones I'm interested in do. I actually use the skanky script below which has a little bit of smarts to do series at a time if they were sent with git send-email... git-msgid -g '1 10' ' ' | git am Ian. 8<-- #!/bin/bash help() { echo "help!" 1>&2 } GIT= while getopts g: OPT ; do case $OPT in g) GIT="$OPTARG" ;; h) help ; exit 1 ;; \?) exit 1 ;; esac done shift $(expr $OPTIND - 1) fetch_messages() { for i in $@ ; do echo "Fetching Message ID $i" 1>&2 if [ -n "$X" ] ; then ssh celaeno cat /srv/mldrop/xen-devel/"\"$i\"" else #wget -O - -q http://bugs.xenproject.org/xen/mid/"$i"/raw i=${i/\+/%2B} curl --silent http://bugs.xenproject.org/xen/mid/"$i"/raw fi done } if [ -z "$GIT" ] ; then fetch_messages $@ else #<1349427871-31195-4-git-send-email-anthony.per...@citrix.com> for i in $@ ; do PATTERN=$(echo "$i" | sed -e 's/^\(<[0-9]*-[0-9]*-\)[0-9]*\(-.*>\)/\1@@NR@@\2/g') echo "GIT pattern $PATTERN" 1>&2 for n in $(seq $GIT) ; do MSG=$(echo "$PATTERN" | sed -e "s/@@NR@@/$n/") fetch_messages $MSG done done fi ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel