Re: [PATCH v8 09/14] riscv: misaligned: move emulated access uniformity check in a function

["Clément Léger" <cleger@rivosinc.com>]

On 26/05/2025 10:41, Andrew Jones wrote:
> On Fri, May 23, 2025 at 09:21:51PM +0200, Clément Léger wrote:
>>
>>
>> On 23/05/2025 20:30, Charlie Jenkins wrote:
>>> On Fri, May 23, 2025 at 12:19:26PM +0200, Clément Léger wrote:
>>>> Split the code that check for the uniformity of misaligned accesses
>>>> performance on all cpus from check_unaligned_access_emulated_all_cpus()
>>>> to its own function which will be used for delegation check. No
>>>> functional changes intended.
>>>>
>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>>> ---
>>>>  arch/riscv/kernel/traps_misaligned.c | 20 ++++++++++++++------
>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
>>>> index f1b2af515592..7ecaa8103fe7 100644
>>>> --- a/arch/riscv/kernel/traps_misaligned.c
>>>> +++ b/arch/riscv/kernel/traps_misaligned.c
>>>> @@ -645,6 +645,18 @@ bool __init check_vector_unaligned_access_emulated_all_cpus(void)
>>>>  }
>>>>  #endif
>>>>  
>>>> +static bool all_cpus_unaligned_scalar_access_emulated(void)
>>>> +{
>>>> +	int cpu;
>>>> +
>>>> +	for_each_online_cpu(cpu)
>>>> +		if (per_cpu(misaligned_access_speed, cpu) !=
>>>> +		    RISCV_HWPROBE_MISALIGNED_SCALAR_EMULATED)
>>>> +			return false;
>>>> +
>>>> +	return true;
>>>> +}
>>>
>>> This ends up wasting time when !CONFIG_RISCV_SCALAR_MISALIGNED since it
>>> will always return false in that case. Maybe there is a way to simplify
>>> the ifdefs and still have performant code, but I don't think this is a
>>> big enough problem to prevent this patch from merging.
>>
>> Yeah I though of that as well but the amount of call to this function is
>> probably well below 10 times so I guess it does not really matters in
>> that case to justify yet another ifdef ?
> 
> Would it need an ifdef? Or can we just do
> 
>  if (!IS_ENABLED(CONFIG_RISCV_SCALAR_MISALIGNED))
>     return false;
> 
> at the top of the function?
> 
> While the function wouldn't waste much time since it's not called much and
> would return false on the first check done in the loop, since it's a
> static function, adding the IS_ENABLED() check would likely allow the
> compiler to completely remove it and all the branches depending on it.

Ah yeah indeed ! I'll do that

Thanks,

Clément

> 
> Thanks,
> drew
> 
>>
>>>
>>> Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
>>> Tested-by: Charlie Jenkins <charlie@rivosinc.com>
>>
>> Thanks,
>>
>> Clément
>>
>>>
>>>> +
>>>>  #ifdef CONFIG_RISCV_SCALAR_MISALIGNED
>>>>  
>>>>  static bool unaligned_ctl __read_mostly;
>>>> @@ -683,8 +695,6 @@ static int cpu_online_check_unaligned_access_emulated(unsigned int cpu)
>>>>  
>>>>  bool __init check_unaligned_access_emulated_all_cpus(void)
>>>>  {
>>>> -	int cpu;
>>>> -
>>>>  	/*
>>>>  	 * We can only support PR_UNALIGN controls if all CPUs have misaligned
>>>>  	 * accesses emulated since tasks requesting such control can run on any
>>>> @@ -692,10 +702,8 @@ bool __init check_unaligned_access_emulated_all_cpus(void)
>>>>  	 */
>>>>  	on_each_cpu(check_unaligned_access_emulated, NULL, 1);
>>>>  
>>>> -	for_each_online_cpu(cpu)
>>>> -		if (per_cpu(misaligned_access_speed, cpu)
>>>> -		    != RISCV_HWPROBE_MISALIGNED_SCALAR_EMULATED)
>>>> -			return false;
>>>> +	if (!all_cpus_unaligned_scalar_access_emulated())
>>>> +		return false;
>>>>  
>>>>  	unaligned_ctl = true;
>>>>  	return true;
>>>> -- 
>>>> 2.49.0
>>>>
>>