Re: [patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

Re: [patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

Well, as you know :) tglx and I are on the road ... I'll try to get to it on Friday before I take off again.

"Suresh Siddha" <suresh.b.siddha@intel.com> wrote:

>On Mon, 2010-03-15 at 06:21 -0700, holt@sgi.com wrote:
>> Convert the memtype_lock from a spin_lock to an rw_lock.  The first
>> version of my patch had this and it did improve performance for fault
>> in times.  The atomic page flags patch (first in the series) improves
>> things much greater for ram pages.  This patch is to help the other pages.
>> 
>
>Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
>
>X86 folks, can you please queue  both these patches if you don't have
>any objections.
>
>thanks,
>suresh
>

--
Sent from my mobile phone, pardon any lack of formatting.

Re: [patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

[Suresh Siddha]

On Wed, 2010-03-17 at 12:51 -0700, H. Peter Anvin wrote:
> Well, as you know :) tglx and I are on the road ... I'll try to get to it on Friday before I take off again.

Also I talked to Thomas about this rwlock conversion and he referred to
RT issues with rwlock. And the best is to avoid this using RCU.

For now, the second patch can be perhaps dropped, as we are being
proactive anyways. We can revisit this in the future.

First patch "[patch 1/2] x86,pat Update the page flags for memtype
atomically instead of using memtype_lock. -V3" is good to go.

thanks,
suresh

Re: [patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

[Peter Zijlstra]

On Wed, 2010-03-17 at 16:19 -0800, Suresh Siddha wrote:
> On Wed, 2010-03-17 at 12:51 -0700, H. Peter Anvin wrote:
> > Well, as you know :) tglx and I are on the road ... I'll try to get to it on Friday before I take off again.
> 
> Also I talked to Thomas about this rwlock conversion and he referred to
> RT issues with rwlock. And the best is to avoid this using RCU.

Its not just RT, even for mainline rwlock_t is a massive pain and often
is no better (actually worse) than a spinlock due to the massive
cacheline bouncing it introduces.

Re: [patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

[Suresh Siddha]

On Wed, 2010-03-24 at 04:32 -0700, Peter Zijlstra wrote:
> On Wed, 2010-03-17 at 16:19 -0800, Suresh Siddha wrote:
> > On Wed, 2010-03-17 at 12:51 -0700, H. Peter Anvin wrote:
> > > Well, as you know :) tglx and I are on the road ... I'll try to get to it on Friday before I take off again.
> > 
> > Also I talked to Thomas about this rwlock conversion and he referred to
> > RT issues with rwlock. And the best is to avoid this using RCU.
> 
> Its not just RT, even for mainline rwlock_t is a massive pain and often
> is no better (actually worse) than a spinlock due to the massive
> cacheline bouncing it introduces.

Don't we have the same cacheline bouncing issues with the ticket
spinlocks?

thanks,
suresh

Re: [patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

[Peter Zijlstra]

On Wed, 2010-03-24 at 09:12 -0700, Suresh Siddha wrote:
> On Wed, 2010-03-24 at 04:32 -0700, Peter Zijlstra wrote:
> > On Wed, 2010-03-17 at 16:19 -0800, Suresh Siddha wrote:
> > > On Wed, 2010-03-17 at 12:51 -0700, H. Peter Anvin wrote:
> > > > Well, as you know :) tglx and I are on the road ... I'll try to get to it on Friday before I take off again.
> > > 
> > > Also I talked to Thomas about this rwlock conversion and he referred to
> > > RT issues with rwlock. And the best is to avoid this using RCU.
> > 
> > Its not just RT, even for mainline rwlock_t is a massive pain and often
> > is no better (actually worse) than a spinlock due to the massive
> > cacheline bouncing it introduces.
> 
> Don't we have the same cacheline bouncing issues with the ticket
> spinlocks?

Sure, but the rwlock_t is unfair and can degrade into much worse
performance than the spinlock.

Thing is, rwlock_t needs to write to the cacheline for each read
acquire, so unless the hold time is much-much longer than the cacheline
bounce time, its just not worth it, but since its a rwlock_t it should
be have short hold time, hence its a useless construct :-)

[patch 0/2] x86,pat: Reduce contention on the memtype_lock -V4

[holt]

Tracking memtype on x86 uses a single global spin_lock for either reading
or changing the memory type.  This includes changes made to page flags
which is perfectly parallel.

Part one of the patchset makes the page-based tracking use cmpxchg
without a need for a lock.

Part two of the patchset converts the spin_lock into a read/write lock.


To: Ingo Molnar <mingo@redhat.com>
To: H. Peter Anvin <hpa@zytor.com>
To: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Robin Holt <holt@sgi.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@gmail.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Cc: x86@kernel.org
Cc: Rafael Wysocki <rjw@novell.com>

---

 arch/x86/include/asm/cacheflush.h |   44 +++++++++++++++++++++-----------------
 arch/x86/mm/pat.c                 |   30 +++++++++----------------
 2 files changed, 36 insertions(+), 38 deletions(-)

[patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

[holt]

[-- Attachment #1: memtype_rwlock_V4 --]
[-- Type: text/plain, Size: 3012 bytes --]


Convert the memtype_lock from a spin_lock to an rw_lock.  The first
version of my patch had this and it did improve performance for fault
in times.  The atomic page flags patch (first in the series) improves
things much greater for ram pages.  This patch is to help the other pages.

To: Ingo Molnar <mingo@redhat.com>
To: H. Peter Anvin <hpa@zytor.com>
To: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Robin Holt <holt@sgi.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@gmail.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Cc: x86@kernel.org

---

 arch/x86/mm/pat.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Index: linux-next/arch/x86/mm/pat.c
===================================================================
--- linux-next.orig/arch/x86/mm/pat.c	2010-03-15 08:13:47.262469632 -0500
+++ linux-next/arch/x86/mm/pat.c	2010-03-15 08:13:47.294446141 -0500
@@ -130,7 +130,7 @@ void pat_init(void)
 
 #undef PAT
 
-static DEFINE_SPINLOCK(memtype_lock);	/* protects memtype accesses */
+static DEFINE_RWLOCK(memtype_lock);	/* protects memtype accesses */
 
 /*
  * Does intersection of PAT memory type and MTRR memory type and returns
@@ -310,7 +310,7 @@ int reserve_memtype(u64 start, u64 end, 
 	new->end	= end;
 	new->type	= actual_type;
 
-	spin_lock(&memtype_lock);
+	write_lock(&memtype_lock);
 
 	err = rbt_memtype_check_insert(new, new_type);
 	if (err) {
@@ -318,12 +318,12 @@ int reserve_memtype(u64 start, u64 end, 
 		       "track %s, req %s\n",
 		       start, end, cattr_name(new->type), cattr_name(req_type));
 		kfree(new);
-		spin_unlock(&memtype_lock);
+		write_unlock(&memtype_lock);
 
 		return err;
 	}
 
-	spin_unlock(&memtype_lock);
+	write_unlock(&memtype_lock);
 
 	dprintk("reserve_memtype added 0x%Lx-0x%Lx, track %s, req %s, ret %s\n",
 		start, end, cattr_name(new->type), cattr_name(req_type),
@@ -354,9 +354,9 @@ int free_memtype(u64 start, u64 end)
 		return -EINVAL;
 	}
 
-	spin_lock(&memtype_lock);
+	write_lock(&memtype_lock);
 	err = rbt_memtype_erase(start, end);
-	spin_unlock(&memtype_lock);
+	write_unlock(&memtype_lock);
 
 	if (err) {
 		printk(KERN_INFO "%s:%d freeing invalid memtype %Lx-%Lx\n",
@@ -400,7 +400,7 @@ static unsigned long lookup_memtype(u64 
 		return rettype;
 	}
 
-	spin_lock(&memtype_lock);
+	read_lock(&memtype_lock);
 
 	entry = rbt_memtype_lookup(paddr);
 	if (entry != NULL)
@@ -408,7 +408,7 @@ static unsigned long lookup_memtype(u64 
 	else
 		rettype = _PAGE_CACHE_UC_MINUS;
 
-	spin_unlock(&memtype_lock);
+	read_unlock(&memtype_lock);
 	return rettype;
 }
 
@@ -748,9 +748,9 @@ static struct memtype *memtype_get_idx(l
 	if (!print_entry)
 		return NULL;
 
-	spin_lock(&memtype_lock);
+	read_lock(&memtype_lock);
 	ret = rbt_memtype_copy_nth_element(print_entry, pos);
-	spin_unlock(&memtype_lock);
+	read_unlock(&memtype_lock);
 
 	if (!ret) {
 		return print_entry;

[patch 0/2] x86,pat: Reduce contention on the memtype_lock -V3

[holt]

Tracking memtype on x86 uses a single global spin_lock for either reading
or changing the memory type.  This includes changes made to page flags
which is perfectly parallel.

Part one of the patchset makes the page-based tracking use cmpxchg
without a need for a lock.

Part two of the patchset converts the spin_lock into a read/write lock.


To: Ingo Molnar <mingo@redhat.com>
To: H. Peter Anvin <hpa@zytor.com>
To: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Robin Holt <holt@sgi.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@gmail.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Cc: x86@kernel.org

---

 arch/x86/include/asm/cacheflush.h |   44 +++++++++++++++++++++-----------------
 arch/x86/mm/pat.c                 |   28 ++++++++----------------
 2 files changed, 35 insertions(+), 37 deletions(-)

[patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

[holt]

[-- Attachment #1: memtype_rwlock_V3 --]
[-- Type: text/plain, Size: 3012 bytes --]


Convert the memtype_lock from a spin_lock to an rw_lock.  The first
version of my patch had this and it did improve performance for fault
in times.  The atomic page flags patch (first in the series) improves
things much greater for ram pages.  This patch is to help the other pages.

To: Ingo Molnar <mingo@redhat.com>
To: H. Peter Anvin <hpa@zytor.com>
To: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Robin Holt <holt@sgi.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@gmail.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Cc: x86@kernel.org

---

 arch/x86/mm/pat.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Index: linux-next/arch/x86/mm/pat.c
===================================================================
--- linux-next.orig/arch/x86/mm/pat.c	2010-03-15 08:13:47.262469632 -0500
+++ linux-next/arch/x86/mm/pat.c	2010-03-15 08:13:47.294446141 -0500
@@ -130,7 +130,7 @@ void pat_init(void)
 
 #undef PAT
 
-static DEFINE_SPINLOCK(memtype_lock);	/* protects memtype accesses */
+static DEFINE_RWLOCK(memtype_lock);	/* protects memtype accesses */
 
 /*
  * Does intersection of PAT memory type and MTRR memory type and returns
@@ -310,7 +310,7 @@ int reserve_memtype(u64 start, u64 end, 
 	new->end	= end;
 	new->type	= actual_type;
 
-	spin_lock(&memtype_lock);
+	write_lock(&memtype_lock);
 
 	err = rbt_memtype_check_insert(new, new_type);
 	if (err) {
@@ -318,12 +318,12 @@ int reserve_memtype(u64 start, u64 end, 
 		       "track %s, req %s\n",
 		       start, end, cattr_name(new->type), cattr_name(req_type));
 		kfree(new);
-		spin_unlock(&memtype_lock);
+		write_unlock(&memtype_lock);
 
 		return err;
 	}
 
-	spin_unlock(&memtype_lock);
+	write_unlock(&memtype_lock);
 
 	dprintk("reserve_memtype added 0x%Lx-0x%Lx, track %s, req %s, ret %s\n",
 		start, end, cattr_name(new->type), cattr_name(req_type),
@@ -354,9 +354,9 @@ int free_memtype(u64 start, u64 end)
 		return -EINVAL;
 	}
 
-	spin_lock(&memtype_lock);
+	write_lock(&memtype_lock);
 	err = rbt_memtype_erase(start, end);
-	spin_unlock(&memtype_lock);
+	write_unlock(&memtype_lock);
 
 	if (err) {
 		printk(KERN_INFO "%s:%d freeing invalid memtype %Lx-%Lx\n",
@@ -400,7 +400,7 @@ static unsigned long lookup_memtype(u64 
 		return rettype;
 	}
 
-	spin_lock(&memtype_lock);
+	read_lock(&memtype_lock);
 
 	entry = rbt_memtype_lookup(paddr);
 	if (entry != NULL)
@@ -408,7 +408,7 @@ static unsigned long lookup_memtype(u64 
 	else
 		rettype = _PAGE_CACHE_UC_MINUS;
 
-	spin_unlock(&memtype_lock);
+	read_unlock(&memtype_lock);
 	return rettype;
 }
 
@@ -748,9 +748,9 @@ static struct memtype *memtype_get_idx(l
 	if (!print_entry)
 		return NULL;
 
-	spin_lock(&memtype_lock);
+	read_lock(&memtype_lock);
 	ret = rbt_memtype_copy_nth_element(print_entry, pos);
-	spin_unlock(&memtype_lock);
+	read_unlock(&memtype_lock);
 
 	if (!ret) {
 		return print_entry;

Re: [patch 2/2] x86,pat Convert memtype_lock into an rw_lock.

[Suresh Siddha]

On Mon, 2010-03-15 at 06:21 -0700, holt@sgi.com wrote:
> Convert the memtype_lock from a spin_lock to an rw_lock.  The first
> version of my patch had this and it did improve performance for fault
> in times.  The atomic page flags patch (first in the series) improves
> things much greater for ram pages.  This patch is to help the other pages.
> 

Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

X86 folks, can you please queue  both these patches if you don't have
any objections.

thanks,
suresh