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

[David Daney <ddaney@caviumnetworks.com>]

David VomLehn wrote:
> On Tue, May 12, 2009 at 03:45:16PM -0700, David Daney wrote:
>> If the TLB refill handler is too bit 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 |   50 +++++++++++++++++++++++++++++++++-----------------
>>  1 files changed, 33 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
>> index 4dc4f3e..ffa7104 100644
>> --- a/arch/mips/mm/tlbex.c
>> +++ b/arch/mips/mm/tlbex.c
>> @@ -723,28 +731,36 @@ static void __cpuinit build_r4000_tlb_refill_handler(void)
>>  		uasm_copy_handler(relocs, labels, tlb_handler, p, f);
>>  		final_len = p - tlb_handler;
>>  	} else {
>> -		u32 *split = tlb_handler + 30;
>> -
>> -		/*
>> -		 * Find the split point.
>> -		 */
>> -		if (uasm_insn_has_bdelay(relocs, split - 1))
>> -			split--;
>> +		u32 *split;
>> +		if (split_on_eret) {
>> +			split = tlb_handler + 32;
>> +		} else {
>> +			split = tlb_handler + 30;
> 
> It would be a shame to pass up an opportunity to eliminate some of the
> pile of magic constants we have in the MIPS tree. Given that the MIPS
> documentation describes the size of the TLB Refill handler as 0x80 bytes,
> we could add something like:
> 

That would be a different patch according to the one patch per problem rule.

> /* Maximum # of instructions in exception vector for TLB Refill handler */
> #define MIPS64_TLB_REFILL_OPS	(0x80 / sizeof(union mips_instruction))
> 
> and could change the last few lines of the code above to, for example:
> 
> 		if (split_on_eret) {
> 			split = tlb_handler + MIPS64_TLB_REFILL_OPS;
> 		} else {
> 			split = tlb_handler + MIPS64_TLB_REFILL_OPS - 2;
> 
> (you'd need to include asm/inst.h to get union mips_instruction defined)

It is certainly possible to do something like that, but it isn't clear 
to me that it makes it any easier to understand.

> 
>> +
>> +			/*
>> +			 * Find the split point.
>> +			 */
>> +			if (uasm_insn_has_bdelay(relocs, split - 1))
>> +				split--;
>> +		}
> 
> The code itself makes sense. Does this case actually happen much, or was
> this just an itch?
> 

For my CPU it was happening 100% of the time when I add the soon to be 
submitted hugeTLBfs support patch.  Although I have not measured it, 
this code is so hot that keeping the normal case fitting on a single 
cache line should be a big win.

David Daney