[PATCH 0/2] MIPS: Cleanup and optimize tlbex.c

[PATCH 0/2] MIPS: Cleanup and optimize tlbex.c

[David Daney]

The main motivation for this patch set is to optimize the tlb refill
handler in the case where it would be split on an eret instruction.
However, it was brought to my attention that the existing code was a
bit cryptic, so I start by trying to clean up the existing code before
patching it.

The first patch should be purely cosmetic, and the second adds the
optimization.

I will reply with the two patches.

David Daney (2):
  MIPS: Replace some magic numbers with symbolic values in tlbex.c
  MIPS: Don't branch to eret in TLB refill.

 arch/mips/mm/tlbex.c |   80 +++++++++++++++++++++++++++++++++++---------------
 1 files changed, 56 insertions(+), 24 deletions(-)

[PATCH 1/2] MIPS: Replace some magic numbers with symbolic values in tlbex.c

[David Daney]

The logic used to split the r4000 refill handler is liberally
sprinkled with magic numbers.  We attempt to explain what they are and
normalize them against a new symbolic value (MIPS64_REFILL_INSNS).

CC: Paul Gortmaker <paul.gortmaker@windriver.com>
CC: David VomLehn <dvomlehn@cisco.com>
Signed-off-by: David Daney <ddaney@caviumnetworks.com>
---
 arch/mips/mm/tlbex.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index 4dc4f3e..d99ed78 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -649,6 +649,14 @@ static void __cpuinit build_update_entries(u32 **p, unsigned int tmp,
 #endif
 }
 
+/*
+ * For a 64-bit kernel, we are using the 64-bit XTLB refill execption
+ * because EXL == 0.  If we wrap, we can also use the 32 instruction
+ * slots before the XTLB refill exception handler which belong to the
+ * unused TLB refill exception.
+ */
+#define MIPS64_REFILL_INSNS 32
+
 static void __cpuinit build_r4000_tlb_refill_handler(void)
 {
 	u32 *p = tlb_handler;
@@ -702,9 +710,10 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 	if ((p - tlb_handler) > 64)
 		panic("TLB refill handler space exceeded");
 #else
-	if (((p - tlb_handler) > 63)
-	    || (((p - tlb_handler) > 61)
-		&& uasm_insn_has_bdelay(relocs, tlb_handler + 29)))
+	if (((p - tlb_handler) > (MIPS64_REFILL_INSNS * 2) - 1)
+	    || (((p - tlb_handler) > (MIPS64_REFILL_INSNS * 2) - 3)
+		&& uasm_insn_has_bdelay(relocs,
+					tlb_handler + MIPS64_REFILL_INSNS - 3)))
 		panic("TLB refill handler space exceeded");
 #endif
 
@@ -717,16 +726,24 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 	uasm_copy_handler(relocs, labels, tlb_handler, p, f);
 	final_len = p - tlb_handler;
 #else /* CONFIG_64BIT */
-	f = final_handler + 32;
-	if ((p - tlb_handler) <= 32) {
+	f = final_handler + MIPS64_REFILL_INSNS;
+	if ((p - tlb_handler) <= MIPS64_REFILL_INSNS) {
 		/* Just copy the handler. */
 		uasm_copy_handler(relocs, labels, tlb_handler, p, f);
 		final_len = p - tlb_handler;
 	} else {
-		u32 *split = tlb_handler + 30;
+		/*
+		 * Split two instructions before the end.  One for the
+		 * branch and one for the instruction in the delay
+		 * slot.
+		 */
+		u32 *split = tlb_handler + MIPS64_REFILL_INSNS - 2;
 
 		/*
-		 * Find the split point.
+		 * Find the split point.  If the branch would fall in
+		 * a delay slot, we must back up an additional
+		 * instruction so that it is no longer in a delay
+		 * slot.
 		 */
 		if (uasm_insn_has_bdelay(relocs, split - 1))
 			split--;
@@ -749,7 +766,8 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 
 		/* Copy the rest of the handler. */
 		uasm_copy_handler(relocs, labels, split, p, final_handler);
-		final_len = (f - (final_handler + 32)) + (p - split);
+		final_len = (f - (final_handler + MIPS64_REFILL_INSNS)) +
+			    (p - split);
 	}
 #endif /* CONFIG_64BIT */
 
-- 
1.6.0.6

Re: [PATCH 1/2] MIPS: Replace some magic numbers with symbolic values in tlbex.c

[Paul Gortmaker]

[[PATCH 1/2] MIPS: Replace some magic numbers with symbolic values in tlbex.c] On 13/05/2009 (Wed 13:48) David Daney wrote:

> The logic used to split the r4000 refill handler is liberally
> sprinkled with magic numbers.  We attempt to explain what they are and
> normalize them against a new symbolic value (MIPS64_REFILL_INSNS).
> 
> CC: Paul Gortmaker <paul.gortmaker@windriver.com>
> CC: David VomLehn <dvomlehn@cisco.com>
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> ---
>  arch/mips/mm/tlbex.c |   34 ++++++++++++++++++++++++++--------
>  1 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index 4dc4f3e..d99ed78 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -649,6 +649,14 @@ static void __cpuinit build_update_entries(u32 **p, unsigned int tmp,
>  #endif
>  }
>  
> +/*
> + * For a 64-bit kernel, we are using the 64-bit XTLB refill execption

                                              "exception"

 -- aside from that, I've double checked that the changes should be inert
(excluding the improved readability).   You can add a:

Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com>

if you want, or given that it really isn't a complex patch, feel free
to reduce the clutter and no need to mention me at all.  Your choice.

Thanks for the cleanup,
Paul.

> + * because EXL == 0.  If we wrap, we can also use the 32 instruction
> + * slots before the XTLB refill exception handler which belong to the
> + * unused TLB refill exception.
> + */
> +#define MIPS64_REFILL_INSNS 32
> +
>  static void __cpuinit build_r4000_tlb_refill_handler(void)
>  {
>  	u32 *p = tlb_handler;
> @@ -702,9 +710,10 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
>  	if ((p - tlb_handler) > 64)
>  		panic("TLB refill handler space exceeded");
>  #else
> -	if (((p - tlb_handler) > 63)
> -	    || (((p - tlb_handler) > 61)
> -		&& uasm_insn_has_bdelay(relocs, tlb_handler + 29)))
> +	if (((p - tlb_handler) > (MIPS64_REFILL_INSNS * 2) - 1)
> +	    || (((p - tlb_handler) > (MIPS64_REFILL_INSNS * 2) - 3)
> +		&& uasm_insn_has_bdelay(relocs,
> +					tlb_handler + MIPS64_REFILL_INSNS - 3)))
>  		panic("TLB refill handler space exceeded");
>  #endif
>  
> @@ -717,16 +726,24 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
>  	uasm_copy_handler(relocs, labels, tlb_handler, p, f);
>  	final_len = p - tlb_handler;
>  #else /* CONFIG_64BIT */
> -	f = final_handler + 32;
> -	if ((p - tlb_handler) <= 32) {
> +	f = final_handler + MIPS64_REFILL_INSNS;
> +	if ((p - tlb_handler) <= MIPS64_REFILL_INSNS) {
>  		/* Just copy the handler. */
>  		uasm_copy_handler(relocs, labels, tlb_handler, p, f);
>  		final_len = p - tlb_handler;
>  	} else {
> -		u32 *split = tlb_handler + 30;
> +		/*
> +		 * Split two instructions before the end.  One for the
> +		 * branch and one for the instruction in the delay
> +		 * slot.
> +		 */
> +		u32 *split = tlb_handler + MIPS64_REFILL_INSNS - 2;
>  
>  		/*
> -		 * Find the split point.
> +		 * Find the split point.  If the branch would fall in
> +		 * a delay slot, we must back up an additional
> +		 * instruction so that it is no longer in a delay
> +		 * slot.
>  		 */
>  		if (uasm_insn_has_bdelay(relocs, split - 1))
>  			split--;
> @@ -749,7 +766,8 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
>  
>  		/* Copy the rest of the handler. */
>  		uasm_copy_handler(relocs, labels, split, p, final_handler);
> -		final_len = (f - (final_handler + 32)) + (p - split);
> +		final_len = (f - (final_handler + MIPS64_REFILL_INSNS)) +
> +			    (p - split);
>  	}
>  #endif /* CONFIG_64BIT */
>  
> -- 
> 1.6.0.6
> 

[PATCH 1/2] MIPS: Replace some magic numbers with symbolic values in tlbex.c (v2)

[David Daney]

The logic used to split the r4000 refill handler is liberally
sprinkled with magic numbers.  We attempt to explain what they are and
normalize them against a new symbolic value (MIPS64_REFILL_INSNS).

Changes from v1:  Corrected spelling in comment.

CC: David VomLehn <dvomlehn@cisco.com>
Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: David Daney <ddaney@caviumnetworks.com>
---

Please use this version instead of the original 1/2.

 arch/mips/mm/tlbex.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index 4dc4f3e..cbc09de 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -649,6 +649,14 @@ static void __cpuinit build_update_entries(u32 **p, unsigned int tmp,
 #endif
 }
 
+/*
+ * For a 64-bit kernel, we are using the 64-bit XTLB refill exception
+ * because EXL == 0.  If we wrap, we can also use the 32 instruction
+ * slots before the XTLB refill exception handler which belong to the
+ * unused TLB refill exception.
+ */
+#define MIPS64_REFILL_INSNS 32
+
 static void __cpuinit build_r4000_tlb_refill_handler(void)
 {
 	u32 *p = tlb_handler;
@@ -702,9 +710,10 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 	if ((p - tlb_handler) > 64)
 		panic("TLB refill handler space exceeded");
 #else
-	if (((p - tlb_handler) > 63)
-	    || (((p - tlb_handler) > 61)
-		&& uasm_insn_has_bdelay(relocs, tlb_handler + 29)))
+	if (((p - tlb_handler) > (MIPS64_REFILL_INSNS * 2) - 1)
+	    || (((p - tlb_handler) > (MIPS64_REFILL_INSNS * 2) - 3)
+		&& uasm_insn_has_bdelay(relocs,
+					tlb_handler + MIPS64_REFILL_INSNS - 3)))
 		panic("TLB refill handler space exceeded");
 #endif
 
@@ -717,16 +726,24 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 	uasm_copy_handler(relocs, labels, tlb_handler, p, f);
 	final_len = p - tlb_handler;
 #else /* CONFIG_64BIT */
-	f = final_handler + 32;
-	if ((p - tlb_handler) <= 32) {
+	f = final_handler + MIPS64_REFILL_INSNS;
+	if ((p - tlb_handler) <= MIPS64_REFILL_INSNS) {
 		/* Just copy the handler. */
 		uasm_copy_handler(relocs, labels, tlb_handler, p, f);
 		final_len = p - tlb_handler;
 	} else {
-		u32 *split = tlb_handler + 30;
+		/*
+		 * Split two instructions before the end.  One for the
+		 * branch and one for the instruction in the delay
+		 * slot.
+		 */
+		u32 *split = tlb_handler + MIPS64_REFILL_INSNS - 2;
 
 		/*
-		 * Find the split point.
+		 * Find the split point.  If the branch would fall in
+		 * a delay slot, we must back up an additional
+		 * instruction so that it is no longer in a delay
+		 * slot.
 		 */
 		if (uasm_insn_has_bdelay(relocs, split - 1))
 			split--;
@@ -749,7 +766,8 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 
 		/* Copy the rest of the handler. */
 		uasm_copy_handler(relocs, labels, split, p, final_handler);
-		final_len = (f - (final_handler + 32)) + (p - split);
+		final_len = (f - (final_handler + MIPS64_REFILL_INSNS)) +
+			    (p - split);
 	}
 #endif /* CONFIG_64BIT */
 
-- 
1.6.0.6

[PATCH 2/2] MIPS: Don't branch to eret in TLB refill.

[David Daney]

If the TLB refill handler is too big and needs to be split, there is
no need to branch around the split if the branch target would be an
eret.  Since the eret returns from the handler, control flow never
passes it.  A branch to an eret is equivalent to the eret itself.

Signed-off-by: David Daney <ddaney@caviumnetworks.com>
---
 arch/mips/mm/tlbex.c |   66 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index d99ed78..0a88383 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -664,6 +664,7 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 	struct uasm_reloc *r = relocs;
 	u32 *f;
 	unsigned int final_len;
+	int split_on_eret;
 
 	memset(tlb_handler, 0, sizeof(tlb_handler));
 	memset(labels, 0, sizeof(labels));
@@ -693,6 +694,12 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 	build_tlb_write_entry(&p, &l, &r, tlb_random);
 	uasm_l_leave(&l, p);
 	uasm_i_eret(&p); /* return from trap */
+	/*
+	 * Check to see if the eret was the last instruction before
+	 * the split.  If it was, there will be no need to branch
+	 * around the split, because we are returning.
+	 */
+	split_on_eret = (p - tlb_handler == MIPS64_REFILL_INSNS);
 
 #ifdef CONFIG_64BIT
 	build_get_pgd_vmalloc64(&p, &l, &r, K0, K1);
@@ -732,36 +739,43 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 		uasm_copy_handler(relocs, labels, tlb_handler, p, f);
 		final_len = p - tlb_handler;
 	} else {
-		/*
-		 * Split two instructions before the end.  One for the
-		 * branch and one for the instruction in the delay
-		 * slot.
-		 */
-		u32 *split = tlb_handler + MIPS64_REFILL_INSNS - 2;
-
-		/*
-		 * Find the split point.  If the branch would fall in
-		 * a delay slot, we must back up an additional
-		 * instruction so that it is no longer in a delay
-		 * slot.
-		 */
-		if (uasm_insn_has_bdelay(relocs, split - 1))
-			split--;
-
+		u32 *split;
+		if (split_on_eret) {
+			split = tlb_handler + MIPS64_REFILL_INSNS;
+		} else {
+			/*
+			 * Split two instructions before the end.  One
+			 * for the branch and one for the instruction
+			 * in the delay slot.
+			 */
+			u32 *split = tlb_handler + MIPS64_REFILL_INSNS - 2;
+
+			/*
+			 * Find the split point.  If the branch would
+			 * fall in a delay slot, we must back up an
+			 * additional instruction so that it is no
+			 * longer in a delay slot.
+			 */
+			if (uasm_insn_has_bdelay(relocs, split - 1))
+				split--;
+		}
 		/* Copy first part of the handler. */
 		uasm_copy_handler(relocs, labels, tlb_handler, split, f);
 		f += split - tlb_handler;
 
-		/* Insert branch. */
-		uasm_l_split(&l, final_handler);
-		uasm_il_b(&f, &r, label_split);
-		if (uasm_insn_has_bdelay(relocs, split))
-			uasm_i_nop(&f);
-		else {
-			uasm_copy_handler(relocs, labels, split, split + 1, f);
-			uasm_move_labels(labels, f, f + 1, -1);
-			f++;
-			split++;
+		if (!split_on_eret) {
+			/* Insert branch. */
+			uasm_l_split(&l, final_handler);
+			uasm_il_b(&f, &r, label_split);
+			if (uasm_insn_has_bdelay(relocs, split))
+				uasm_i_nop(&f);
+			else {
+				uasm_copy_handler(relocs, labels,
+						  split, split + 1, f);
+				uasm_move_labels(labels, f, f + 1, -1);
+				f++;
+				split++;
+			}
 		}
 
 		/* Copy the rest of the handler. */
-- 
1.6.0.6

[PATCH 2/2] MIPS: Replace some magic numbers with symbolic values in tlbex.c (v2)

[David Daney]

The logic used to split the r4000 refill handler is liberally
sprinkled with magic numbers.  We attempt to explain what they are and
normalize them against a new symbolic value (MIPS64_REFILL_INSNS).

Changes from v1:  Corrected spelling in comment.

CC: David VomLehn <dvomlehn@cisco.com>
Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: David Daney <ddaney@caviumnetworks.com>
---

Please use this version instead of the original 2/2.

 arch/mips/mm/tlbex.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index 4dc4f3e..cbc09de 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -649,6 +649,14 @@ static void __cpuinit build_update_entries(u32 **p, unsigned int tmp,
 #endif
 }
 
+/*
+ * For a 64-bit kernel, we are using the 64-bit XTLB refill exception
+ * because EXL == 0.  If we wrap, we can also use the 32 instruction
+ * slots before the XTLB refill exception handler which belong to the
+ * unused TLB refill exception.
+ */
+#define MIPS64_REFILL_INSNS 32
+
 static void __cpuinit build_r4000_tlb_refill_handler(void)
 {
 	u32 *p = tlb_handler;
@@ -702,9 +710,10 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 	if ((p - tlb_handler) > 64)
 		panic("TLB refill handler space exceeded");
 #else
-	if (((p - tlb_handler) > 63)
-	    || (((p - tlb_handler) > 61)
-		&& uasm_insn_has_bdelay(relocs, tlb_handler + 29)))
+	if (((p - tlb_handler) > (MIPS64_REFILL_INSNS * 2) - 1)
+	    || (((p - tlb_handler) > (MIPS64_REFILL_INSNS * 2) - 3)
+		&& uasm_insn_has_bdelay(relocs,
+					tlb_handler + MIPS64_REFILL_INSNS - 3)))
 		panic("TLB refill handler space exceeded");
 #endif
 
@@ -717,16 +726,24 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 	uasm_copy_handler(relocs, labels, tlb_handler, p, f);
 	final_len = p - tlb_handler;
 #else /* CONFIG_64BIT */
-	f = final_handler + 32;
-	if ((p - tlb_handler) <= 32) {
+	f = final_handler + MIPS64_REFILL_INSNS;
+	if ((p - tlb_handler) <= MIPS64_REFILL_INSNS) {
 		/* Just copy the handler. */
 		uasm_copy_handler(relocs, labels, tlb_handler, p, f);
 		final_len = p - tlb_handler;
 	} else {
-		u32 *split = tlb_handler + 30;
+		/*
+		 * Split two instructions before the end.  One for the
+		 * branch and one for the instruction in the delay
+		 * slot.
+		 */
+		u32 *split = tlb_handler + MIPS64_REFILL_INSNS - 2;
 
 		/*
-		 * Find the split point.
+		 * Find the split point.  If the branch would fall in
+		 * a delay slot, we must back up an additional
+		 * instruction so that it is no longer in a delay
+		 * slot.
 		 */
 		if (uasm_insn_has_bdelay(relocs, split - 1))
 			split--;
@@ -749,7 +766,8 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
 
 		/* Copy the rest of the handler. */
 		uasm_copy_handler(relocs, labels, split, p, final_handler);
-		final_len = (f - (final_handler + 32)) + (p - split);
+		final_len = (f - (final_handler + MIPS64_REFILL_INSNS)) +
+			    (p - split);
 	}
 #endif /* CONFIG_64BIT */
 
-- 
1.6.0.6

Re: [PATCH 2/2] MIPS: Replace some magic numbers with symbolic values in tlbex.c (v2)

[David Daney]

Well I replied to the wrong message and screwed up the subject.  So
Ignore this one and I will send it again properly

David Daney

David Daney wrote:
> The logic used to split the r4000 refill handler is liberally
> sprinkled with magic numbers.  We attempt to explain what they are and
> normalize them against a new symbolic value (MIPS64_REFILL_INSNS).
> 
> Changes from v1:  Corrected spelling in comment.
> 
> CC: David VomLehn <dvomlehn@cisco.com>
> Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> ---
> 
> Please use this version instead of the original 2/2.
> 
>  arch/mips/mm/tlbex.c |   34 ++++++++++++++++++++++++++--------
>  1 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index 4dc4f3e..cbc09de 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -649,6 +649,14 @@ static void __cpuinit build_update_entries(u32 **p, unsigned int tmp,
>  #endif