Re: [PATCH] net/mlx5: reduce stack usage in FW tracer

[Saeed Mahameed]

On Fri, 2019-09-06 at 17:11 +0200, Arnd Bergmann wrote:
> It's generally not ok to put a 512 byte buffer on the stack, as
> kernel
> stack is a scarce resource:
> 
> drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c:660:13:
> error: stack frame size of 1032 bytes in function
> 'mlx5_fw_tracer_handle_traces' [-Werror,-Wframe-larger-than=]
> 
> This is done in a context that is allowed to sleep, so using
> dynamic allocation is ok as well. I'm not too worried about
> runtime overhead, as this already contains an snprintf() and
> other expensive functions.
> 
> Fixes: 70dd6fdb8987 ("net/mlx5: FW tracer, parse traces and kernel
> tracing support")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>  .../mellanox/mlx5/core/diag/fw_tracer.c       | 21 ++++++++++-------
> --
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> index 2011eaf15cc5..d81e78060f9f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> @@ -557,16 +557,16 @@ static void mlx5_tracer_print_trace(struct
> tracer_string_format *str_frmt,
>  				    struct mlx5_core_dev *dev,
>  				    u64 trace_timestamp)
>  {
> -	char	tmp[512];
> -

Hi Arnd, thanks for the patch, 
this function is very perfomance critical when fw traces are activated
to pull some fw content on error situations, using kmalloc here might
become a problem and stall the system further more if the problem was
initially due to lack of memory.

since this function only needs 512 bytes maybe we should mark it as
noinline to avoid any extra stack usages on the caller function
mlx5_fw_tracer_handle_traces ?

> -	snprintf(tmp, sizeof(tmp), str_frmt->string,
> -		 str_frmt->params[0],
> -		 str_frmt->params[1],
> -		 str_frmt->params[2],
> -		 str_frmt->params[3],
> -		 str_frmt->params[4],
> -		 str_frmt->params[5],
> -		 str_frmt->params[6]);
> +	char *tmp = kasprintf(GFP_KERNEL, str_frmt->string,
> +			      str_frmt->params[0],
> +			      str_frmt->params[1],
> +			      str_frmt->params[2],
> +			      str_frmt->params[3],
> +			      str_frmt->params[4],
> +			      str_frmt->params[5],
> +			      str_frmt->params[6]);
> +	if (!tmp)
> +		return;
>  
>  	trace_mlx5_fw(dev->tracer, trace_timestamp, str_frmt->lost,
>  		      str_frmt->event_id, tmp);
> @@ -576,6 +576,7 @@ static void mlx5_tracer_print_trace(struct
> tracer_string_format *str_frmt,
>  
>  	/* remove it from hash */
>  	mlx5_tracer_clean_message(str_frmt);
> +	kfree(tmp);
>  }
>  
>  static int mlx5_tracer_handle_string_trace(struct mlx5_fw_tracer
> *tracer,