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 Mon,  7 Feb 2022 12:37:21 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> --- a/arch/powerpc/kernel/trace/ftrace.c
>> +++ b/arch/powerpc/kernel/trace/ftrace.c
>> @@ -1137,3 +1137,14 @@ char *arch_ftrace_match_adjust(char *str, const char *search)
>>  		return str;
>>  }
>>  #endif /* PPC64_ELF_ABI_v1 */
>> +
>> +#ifdef CONFIG_MPROFILE_KERNEL
>> +unsigned long ftrace_location_lookup(unsigned long ip)
>> +{
>> +	/*
>> +	 * Per livepatch.h, ftrace location is always within the first
>> +	 * 16 bytes of a function on powerpc with -mprofile-kernel.
>> +	 */
>> +	return ftrace_location_range(ip, ip + 16);
> 
> I think this is the wrong approach for the implementation and error prone.
> 
>> +}
>> +#endif
>> -- 
> 
> What I believe is a safer approach is to use the record address and add the
> range to it.
> 
> That is, you know that the ftrace modification site is a range (multiple
> instructions), where in the ftrace infrastructure, only one ip represents
> that range. What you want is if you pass in an ip, and that ip is within
> that range, you return the ip that represents that range to ftrace.
> 
> It looks like we need to change the compare function in the bsearch.
> 
> Perhaps add a new macro to define the size of the range to be searched,
> instead of just using MCOUNT_INSN_SIZE? Then we may not even need this new
> lookup function?
> 
> static int ftrace_cmp_recs(const void *a, const void *b)
> {
> 	const struct dyn_ftrace *key = a;
> 	const struct dyn_ftrace *rec = b;
> 
> 	if (key->flags < rec->ip)
> 		return -1;
> 	if (key->ip >= rec->ip + ARCH_IP_SIZE)
> 		return 1;
> 	return 0;
> }
> 
> Where ARCH_IP_SIZE is defined to MCOUNT_INSN_SIZE by default, but an arch
> could define it to something else, like 16.
> 
> Would that work for you, or am I missing something?

Yes, I hadn't realized that [un]register_ftrace_direct() and 
modify_ftrace_direct() internally lookup the correct ftrace location, 
and act on that. So, changing ftrace_cmp_recs() does look like it will 
work well for powerpc. Thanks for this suggestion.

However, I think we will not be able to use a fixed range.  I would like 
to reserve instructions from function entry till the branch to 
_mcount(), and it can be two or four instructions depending on whether a 
function has a global entry point. For this, I am considering adding a 
field in 'struct dyn_arch_ftrace', and a hook in ftrace_process_locs() 
to initialize the same. I may need to override ftrace_cmp_recs().


Thanks,
- Naveen