[PATCH] UV: fix incorrect tlb flush all issue

[PATCH] UV: fix incorrect tlb flush all issue

[Alex Shi]

The flush tlb optimization code has logical issue on UV platform.
It doesn't flush the full range at all, since it simply
ignores its 'end' parameter (and hence also the "all" indicator)
in uv_flush_tlb_others() function.

This patch fixed this issue, but untested due to hardware leaking.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Alex Shi <alex.shi@intel.com>
---
 arch/x86/include/asm/uv/uv_bau.h |    6 ++++--
 arch/x86/platform/uv/tlb_uv.c    |   14 ++++++++------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/uv/uv_bau.h b/arch/x86/include/asm/uv/uv_bau.h
index a06983c..ac6f326 100644
--- a/arch/x86/include/asm/uv/uv_bau.h
+++ b/arch/x86/include/asm/uv/uv_bau.h
@@ -225,8 +225,10 @@ struct bau_local_cpumask {
  * The payload is software-defined for INTD transactions
  */
 struct bau_msg_payload {
-	unsigned long	address;		/* signifies a page or all
-						   TLB's of the cpu */
+	unsigned long	start;			/* start address to flush TLB
+						   of the cpu */
+	unsigned long	end;			/* end address to flush TLB
+						   of the cpu */
 	/* 64 bits */
 	unsigned short	sending_cpu;		/* filled in by sender */
 	/* 16 bits */
diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c
index b8b3a37..c603d15 100644
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -280,12 +280,12 @@ static void bau_process_message(struct msg_desc *mdp, struct bau_control *bcp,
 	/*
 	 * This must be a normal message, or retry of a normal message
 	 */
-	if (msg->address == TLB_FLUSH_ALL) {
+	if (msg->end == 0) {
+		__flush_tlb_one(msg->start);
+		stat->d_onetlb++;
+	} else {
 		local_flush_tlb();
 		stat->d_alltlb++;
-	} else {
-		__flush_tlb_one(msg->address);
-		stat->d_onetlb++;
 	}
 	stat->d_requestee++;
 
@@ -1034,7 +1034,8 @@ static int set_distrib_bits(struct cpumask *flush_mask, struct bau_control *bcp,
  * globally purge translation cache of a virtual address or all TLB's
  * @cpumask: mask of all cpu's in which the address is to be removed
  * @mm: mm_struct containing virtual address range
- * @va: virtual address to be removed (or TLB_FLUSH_ALL for all TLB's on cpu)
+ * @start: start virtual address to be removed from TLB
+ * @end: end virtual address to be remove from TLB
  * @cpu: the current cpu
  *
  * This is the entry point for initiating any UV global TLB shootdown.
@@ -1113,7 +1114,8 @@ const struct cpumask *uv_flush_tlb_others(const struct cpumask *cpumask,
 
 	record_send_statistics(stat, locals, hubs, remotes, bau_desc);
 
-	bau_desc->payload.address = start;
+	bau_desc->payload.start = start;
+	bau_desc->payload.end	= end;
 	bau_desc->payload.sending_cpu = cpu;
 	/*
 	 * uv_flush_send_and_wait returns 0 if all cpu's were messaged,
-- 
1.7.5.4

Re: [PATCH] UV: fix incorrect tlb flush all issue

[Andrew Morton]

On Fri, 24 Aug 2012 16:57:35 +0800
Alex Shi <alex.shi@intel.com> wrote:

> The flush tlb optimization code has logical issue on UV platform.
> It doesn't flush the full range at all, since it simply
> ignores its 'end' parameter (and hence also the "all" indicator)
> in uv_flush_tlb_others() function.
> 
> This patch fixed this issue, but untested due to hardware leaking.

Well, it doesn't really come very close to being compilable:

arch/x86/platform/uv/tlb_uv.c: In function 'bau_process_message':
arch/x86/platform/uv/tlb_uv.c:283: error: 'struct bau_pq_entry' has no member named 'end'
arch/x86/platform/uv/tlb_uv.c:284: error: 'struct bau_pq_entry' has no member named 'start'

Cliff, could you please help out here?

Re: [PATCH] UV: fix incorrect tlb flush all issue

[Alex Shi]

On 09/07/2012 07:11 AM, Andrew Morton wrote:

> On Fri, 24 Aug 2012 16:57:35 +0800
> Alex Shi <alex.shi@intel.com> wrote:
> 
>> The flush tlb optimization code has logical issue on UV platform.
>> It doesn't flush the full range at all, since it simply
>> ignores its 'end' parameter (and hence also the "all" indicator)
>> in uv_flush_tlb_others() function.
>>
>> This patch fixed this issue, but untested due to hardware leaking.
> 
> Well, it doesn't really come very close to being compilable:
> 
> arch/x86/platform/uv/tlb_uv.c: In function 'bau_process_message':
> arch/x86/platform/uv/tlb_uv.c:283: error: 'struct bau_pq_entry' has no member named 'end'
> arch/x86/platform/uv/tlb_uv.c:284: error: 'struct bau_pq_entry' has no member named 'start'
> 
> Cliff, could you please help out here?
> 




Sorry, I had found the problem yesterday, and in trying to fix this issue.

here I use the simplest fixing, that just flush single page or flush all tlb.
It will great if UV experts like to enable flush_tlb_range() finally.

------
>From 1b5e470466d719109770a39b230c074f9d2a1490 Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@intel.com>
Date: Fri, 7 Sep 2012 10:38:36 +0800
Subject: [PATCH] x86/uv: fix incorrect tlb flush issue

The flush tlb optimization code has logical issue on UV platform.
It doesn't flush the full range at all, since it simply
ignores its 'end' parameter (and hence also the "all" indicator)
in uv_flush_tlb_others() function.

I have tried to embed the start/end on UV code, but finally find the
payload struct embedded hardware filled vector. So be lacking
the enough knowledge of the hardware, I just check the start/end fair
in uv_flush_tlb_others(): except single page flush(end==0), will flush
all TLB entries for any other situations.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Alex Shi <alex.shi@intel.com>
Cc: Jack Steiner <steiner@sgi.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/platform/uv/tlb_uv.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c
index b8b3a37..1b2d49a 100644
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -1034,7 +1034,8 @@ static int set_distrib_bits(struct cpumask *flush_mask, struct bau_control *bcp,
  * globally purge translation cache of a virtual address or all TLB's
  * @cpumask: mask of all cpu's in which the address is to be removed
  * @mm: mm_struct containing virtual address range
- * @va: virtual address to be removed (or TLB_FLUSH_ALL for all TLB's on cpu)
+ * @start: start virtual address to be removed from TLB
+ * @end: end virtual address to be remove from TLB
  * @cpu: the current cpu
  *
  * This is the entry point for initiating any UV global TLB shootdown.
@@ -1113,7 +1114,10 @@ const struct cpumask *uv_flush_tlb_others(const struct cpumask *cpumask,
 
 	record_send_statistics(stat, locals, hubs, remotes, bau_desc);
 
-	bau_desc->payload.address = start;
+	if (!end)
+		bau_desc->payload.address = TLB_FLUSH_ALL;
+	else
+		bau_desc->payload.address = start;
 	bau_desc->payload.sending_cpu = cpu;
 	/*
 	 * uv_flush_send_and_wait returns 0 if all cpu's were messaged,
-- 
1.7.5.4

Re: [PATCH] UV: fix incorrect tlb flush all issue

[Jan Beulich]

>>> On 07.09.12 at 07:37, Alex Shi <alex.shi@intel.com> wrote:
> @@ -1113,7 +1114,10 @@ const struct cpumask *uv_flush_tlb_others(const struct 
> cpumask *cpumask,
>  
>  	record_send_statistics(stat, locals, hubs, remotes, bau_desc);
>  
> -	bau_desc->payload.address = start;
> +	if (!end)

So despite having made a very similar mistake on the Xen side,
you're again not following your own code adjustments here:
The indicator for a multi-page flush is not 'end' being zero, nor
would it be okay to use the proper indicator (TLB_FLUSH_ALL)
here. Instead, to yield correct code, you need to check for the
range covering more than one page. Improvements to do finer
grained flushing (if worthwhile on UV) could then be done on
top of this.

Jan

> +		bau_desc->payload.address = TLB_FLUSH_ALL;
> +	else
> +		bau_desc->payload.address = start;
>  	bau_desc->payload.sending_cpu = cpu;
>  	/*
>  	 * uv_flush_send_and_wait returns 0 if all cpu's were messaged,

Re: [PATCH] UV: fix incorrect tlb flush all issue

[Alex Shi]

On 09/07/2012 03:10 PM, Jan Beulich wrote:

>>>> On 07.09.12 at 07:37, Alex Shi <alex.shi@intel.com> wrote:
>> @@ -1113,7 +1114,10 @@ const struct cpumask *uv_flush_tlb_others(const struct 
>> cpumask *cpumask,
>>  
>>  	record_send_statistics(stat, locals, hubs, remotes, bau_desc);
>>  
>> -	bau_desc->payload.address = start;
>> +	if (!end)
> 
> So despite having made a very similar mistake on the Xen side,
> you're again not following your own code adjustments here:
> The indicator for a multi-page flush is not 'end' being zero, nor
> would it be okay to use the proper indicator (TLB_FLUSH_ALL)
> here. Instead, to yield correct code, you need to check for the
> range covering more than one page. Improvements to do finer
> grained flushing (if worthwhile on UV) could then be done on
> top of this.
> 




Thanks for catching this typo. The patch was updated:
==========

>From bccb5f34834986fbd36c069640237296a9b1fe37 Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@intel.com>
Date: Fri, 7 Sep 2012 10:38:36 +0800
Subject: [PATCH] x86/uv: fix incorrect tlb flush issue

The flush tlb optimization code has logical issue on UV platform.
It doesn't flush the full range at all, since it simply
ignores its 'end' parameter (and hence also the "all" indicator)
in uv_flush_tlb_others() function.

I have tried to embed the start/end on UV code, but finally find the
'payload' message format embedded hardware filled vector. So be lacking
the enough knowledge of the hardware, I just check the start/end fair
in uv_flush_tlb_others(): except single page flush, will flush
all TLB entries for other situations.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Alex Shi <alex.shi@intel.com>
Cc: Jack Steiner <steiner@sgi.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/platform/uv/tlb_uv.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c
index b8b3a37..582836e 100644
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -1034,7 +1034,8 @@ static int set_distrib_bits(struct cpumask *flush_mask, struct bau_control *bcp,
  * globally purge translation cache of a virtual address or all TLB's
  * @cpumask: mask of all cpu's in which the address is to be removed
  * @mm: mm_struct containing virtual address range
- * @va: virtual address to be removed (or TLB_FLUSH_ALL for all TLB's on cpu)
+ * @start: start virtual address to be removed from TLB
+ * @end: end virtual address to be remove from TLB
  * @cpu: the current cpu
  *
  * This is the entry point for initiating any UV global TLB shootdown.
@@ -1113,7 +1114,10 @@ const struct cpumask *uv_flush_tlb_others(const struct cpumask *cpumask,
 
 	record_send_statistics(stat, locals, hubs, remotes, bau_desc);
 
-	bau_desc->payload.address = start;
+	if (!end || (end - start) <= PAGE_SIZE)
+		bau_desc->payload.address = start;
+	else
+		bau_desc->payload.address = TLB_FLUSH_ALL;
 	bau_desc->payload.sending_cpu = cpu;
 	/*
 	 * uv_flush_send_and_wait returns 0 if all cpu's were messaged,
-- 
1.7.5.4

[PATCH] UV: fix incorrect tlb flush all issue

[Cliff Wickman]

From: Cliff Wickman <cpw@sgi.com>

(this was sent as an ack on 9/13, but with incorrect title and sign-off)

Ack.
But with the adjustment below.  The 'end' argument was not declared long.

I tested the patch on a UV.
It has the effect of either clearing 1 or all TLBs in a cpu.
I added some debugging to test for the cases when clearing all TLBs is
overkill, and in practice it happens very seldom.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Alex Shi <alex.shi@intel.com>
Signed-off-by: Cliff Wickman <cpw@sgi.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/include/asm/uv/uv.h  |    2 +-
 arch/x86/platform/uv/tlb_uv.c |   10 +++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

Index: linux/arch/x86/platform/uv/tlb_uv.c
===================================================================
--- linux.orig/arch/x86/platform/uv/tlb_uv.c
+++ linux/arch/x86/platform/uv/tlb_uv.c
@@ -1034,7 +1034,8 @@ static int set_distrib_bits(struct cpuma
  * globally purge translation cache of a virtual address or all TLB's
  * @cpumask: mask of all cpu's in which the address is to be removed
  * @mm: mm_struct containing virtual address range
- * @va: virtual address to be removed (or TLB_FLUSH_ALL for all TLB's on cpu)
+ * @start: start virtual address to be removed from TLB
+ * @end: end virtual address to be remove from TLB
  * @cpu: the current cpu
  *
  * This is the entry point for initiating any UV global TLB shootdown.
@@ -1056,7 +1057,7 @@ static int set_distrib_bits(struct cpuma
  */
 const struct cpumask *uv_flush_tlb_others(const struct cpumask *cpumask,
 				struct mm_struct *mm, unsigned long start,
-				unsigned end, unsigned int cpu)
+				unsigned long end, unsigned int cpu)
 {
 	int locals = 0;
 	int remotes = 0;
@@ -1113,7 +1114,10 @@ const struct cpumask *uv_flush_tlb_other
 
 	record_send_statistics(stat, locals, hubs, remotes, bau_desc);
 
-	bau_desc->payload.address = start;
+	if (!end || (end - start) <= PAGE_SIZE)
+		bau_desc->payload.address = start;
+	else
+		bau_desc->payload.address = TLB_FLUSH_ALL;
 	bau_desc->payload.sending_cpu = cpu;
 	/*
 	 * uv_flush_send_and_wait returns 0 if all cpu's were messaged,
Index: linux/arch/x86/include/asm/uv/uv.h
===================================================================
--- linux.orig/arch/x86/include/asm/uv/uv.h
+++ linux/arch/x86/include/asm/uv/uv.h
@@ -16,7 +16,7 @@ extern void uv_system_init(void);
 extern const struct cpumask *uv_flush_tlb_others(const struct cpumask *cpumask,
 						 struct mm_struct *mm,
 						 unsigned long start,
-						 unsigned end,
+						 unsigned long end,
 						 unsigned int cpu);
 
 #else	/* X86_UV */

Re: [PATCH] UV: fix incorrect tlb flush all issue

[Ingo Molnar]

* Cliff Wickman <cpw@sgi.com> wrote:

> From: Cliff Wickman <cpw@sgi.com>
> 
> (this was sent as an ack on 9/13, but with incorrect title and sign-off)
> 
> Ack.
> But with the adjustment below.  The 'end' argument was not declared long.
> 
> I tested the patch on a UV.
> It has the effect of either clearing 1 or all TLBs in a cpu.
> I added some debugging to test for the cases when clearing all TLBs is
> overkill, and in practice it happens very seldom.
> 
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Alex Shi <alex.shi@intel.com>
> Signed-off-by: Cliff Wickman <cpw@sgi.com>

If this is still substantially the original patch then I suspect 
this should be From: Alex Shi, and the small fix you made can be 
added as:

 Signed-off-by: Alex Shi <alex.shi@intel.com>
 [ fixed the 'end' argument to be declared 'long'. ]
 Signed-off-by: Cliff Wickman <cpw@sgi.com>

Thanks,

	Ingo

Re: [PATCH] UV: fix incorrect tlb flush all issue

[Andrew Morton]

On Tue, 25 Sep 2012 07:34:52 -0500
Cliff Wickman <cpw@sgi.com> wrote:

> From: Cliff Wickman <cpw@sgi.com>
> 
> (this was sent as an ack on 9/13, but with incorrect title and sign-off)
> 
> Ack.
> But with the adjustment below.  The 'end' argument was not declared long.
> 
> I tested the patch on a UV.
> It has the effect of either clearing 1 or all TLBs in a cpu.
> I added some debugging to test for the cases when clearing all TLBs is
> overkill, and in practice it happens very seldom.

- The initial (and rather wrong) patch was prepared by Alex.  Was it
  intentional that you claimed authorship?

  This patch is very different from the original, so that would be a
  reasonable thing to do.  But people often rewrite authorship by
  mistake.  Please clarify.

- If this is indeed considered a quite different patch then a
  Reported-by:Alex would be appropriate.

- You forgot to cc Alex on this email!

- Alex's original changelog contained the all-important bug
  description and that has got lost.


Here is what I have pieced together from this little mess.  It might be
wrong.  I added it to -mm's to-send-to-Ingo queue.


From: Alex Shi <alex.shi@intel.com>
Subject: arch/x86/platform/uv: fix incorrect tlb flush all issue

The flush tlb optimization code has logical issue on UV platform.  It
doesn't flush the full range at all, since it simply ignores its 'end'
parameter (and hence also the "all" indicator) in uv_flush_tlb_others()
function.

Cliff's notes:

: I tested the patch on a UV.  It has the effect of either clearing 1 or all
: TLBs in a cpu.  I added some debugging to test for the cases when clearing
: all TLBs is overkill, and in practice it happens very seldom.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Alex Shi <alex.shi@intel.com>
Signed-off-by: Cliff Wickman <cpw@sgi.com>
Tested-by: Cliff Wickman <cpw@sgi.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/x86/include/asm/uv/uv.h  |    2 +-
 arch/x86/platform/uv/tlb_uv.c |   10 +++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff -puN arch/x86/include/asm/uv/uv.h~uv-fix-incorrect-tlb-flush-all-issue arch/x86/include/asm/uv/uv.h
--- a/arch/x86/include/asm/uv/uv.h~uv-fix-incorrect-tlb-flush-all-issue
+++ a/arch/x86/include/asm/uv/uv.h
@@ -16,7 +16,7 @@ extern void uv_system_init(void);
 extern const struct cpumask *uv_flush_tlb_others(const struct cpumask *cpumask,
 						 struct mm_struct *mm,
 						 unsigned long start,
-						 unsigned end,
+						 unsigned long end,
 						 unsigned int cpu);
 
 #else	/* X86_UV */
diff -puN arch/x86/platform/uv/tlb_uv.c~uv-fix-incorrect-tlb-flush-all-issue arch/x86/platform/uv/tlb_uv.c
--- a/arch/x86/platform/uv/tlb_uv.c~uv-fix-incorrect-tlb-flush-all-issue
+++ a/arch/x86/platform/uv/tlb_uv.c
@@ -1034,7 +1034,8 @@ static int set_distrib_bits(struct cpuma
  * globally purge translation cache of a virtual address or all TLB's
  * @cpumask: mask of all cpu's in which the address is to be removed
  * @mm: mm_struct containing virtual address range
- * @va: virtual address to be removed (or TLB_FLUSH_ALL for all TLB's on cpu)
+ * @start: start virtual address to be removed from TLB
+ * @end: end virtual address to be remove from TLB
  * @cpu: the current cpu
  *
  * This is the entry point for initiating any UV global TLB shootdown.
@@ -1056,7 +1057,7 @@ static int set_distrib_bits(struct cpuma
  */
 const struct cpumask *uv_flush_tlb_others(const struct cpumask *cpumask,
 				struct mm_struct *mm, unsigned long start,
-				unsigned end, unsigned int cpu)
+				unsigned long end, unsigned int cpu)
 {
 	int locals = 0;
 	int remotes = 0;
@@ -1113,7 +1114,10 @@ const struct cpumask *uv_flush_tlb_other
 
 	record_send_statistics(stat, locals, hubs, remotes, bau_desc);
 
-	bau_desc->payload.address = start;
+	if (!end || (end - start) <= PAGE_SIZE)
+		bau_desc->payload.address = start;
+	else
+		bau_desc->payload.address = TLB_FLUSH_ALL;
 	bau_desc->payload.sending_cpu = cpu;
 	/*
 	 * uv_flush_send_and_wait returns 0 if all cpu's were messaged,
_