Re: [PATCH] mlx5: reduce stack usage in mlx5_setup_tc

[Arnd Bergmann]

On Tue, Jan 17, 2023, at 18:46, Tariq Toukan wrote:
> On 17/01/2023 19:28, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@xxxxxxxx>
>> 
>> Clang warns about excessive stack usage on 32-bit targets:
>> 
>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:3597:12: error: stack frame size (1184) exceeds limit (1024) in 'mlx5e_setup_tc' [-Werror,-Wframe-larger-than]
>> static int mlx5e_setup_tc(struct net_device *dev, enum tc_setup_type type,
>> 
>> It turns out that both the mlx5e_setup_tc_mqprio_dcb() function and
>> the mlx5e_safe_switch_params() function it calls have a copy of
>> 'struct mlx5e_params' on the stack, and this structure is fairly
>> large.
>> 
>> Use dynamic allocation for both.
>> 

>>   
>> -	err = mlx5e_safe_switch_params(priv, &new_params,
>> +	err = mlx5e_safe_switch_params(priv, new_params,
>>   				       mlx5e_num_channels_changed_ctx, NULL, true);
>>   
>
> Is this change really required, even after new_chs are dynamically 
> allocated?
> As this code pattern of static local new_params repeats in all callers 
> of mlx5e_safe_switch_params, let's not change this one alone if not 
> necessary.

I'm not sure any more now, I actually did the patch a few weeks ago
and only now came across it while going through my backlog.

Generally speaking, the 'new_params' structure on the stack is
too large, but I no longer see warnings after my patch.

> Same for the noinline_for_stack. Are they really needed even after using 
> dynamic allocation for new_chs?

I've reverted both of those hunks now, let me try reproducing the
original randconfig reports and see what still happens.

   Arnd