Re: [RFC PATCH 2/3] powerpc/ftrace: Override ftrace_location_lookup() for MPROFILE_KERNEL

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Steven Rostedt wrote:
> On Thu, 10 Feb 2022 13:58:29 +0000
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index f9feb197b2daaf..68f20cf34b0c47 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -1510,6 +1510,7 @@ ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs)
>>  	}
>>  
>>  
>> +#ifndef ftrace_cmp_recs
>>  static int ftrace_cmp_recs(const void *a, const void *b)
>>  {
>>  	const struct dyn_ftrace *key = a;
>> @@ -1521,6 +1522,7 @@ static int ftrace_cmp_recs(const void *a, const void *b)
>>  		return 1;
>>  	return 0;
>>  }
>> +#endif
>>  
> 
> I don't really care for this part, as it seems somewhat ugly. But this
> patch does appear to solve your issue, and I can't think of a prettier way
> to do this.

Yes, this approach looks like it will solve a few different issues for 
us. We would also like to nop-out the instruction before the branch to 
_mcount(), so this helps ensure that kprobes won't also overwrite that 
instruction.

> 
> So, I will reluctantly ack it.

Since we don't want to change struct dyn_ftrace, we will need to contain 
changes within the architecture code, so I don't see a way to do this in 
a generic manner.

The other option is to mark ftrace_cmp_recs() as a __weak function, but 
I have a vague recollection of you suggesting #ifdef rather than a 
__weak function in the past. I might be mis-remembering, so if you think 
making this a __weak function is better, I can do that.

> 
> If anything, please add a comment above saying that architectures may need
> to override this function, and when doing so, they will define their own
> ftrace_cmp_recs.

Sure.


- Naveen