Re: [PATCH] net/mlx5e: reduce stack usage in mlx5_eswitch_termtbl_create

[Mark Bloch <markb@mellanox.com>]

On 6/17/19 11:02 AM, Saeed Mahameed wrote:
> On Mon, 2019-06-17 at 13:08 +0200, Arnd Bergmann wrote:
>> Putting an empty 'mlx5_flow_spec' structure on the stack is a bit
>> wasteful and causes a warning on 32-bit architectures when building
>> with clang -fsanitize-coverage:
>>
>> drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c:
>> In function 'mlx5_eswitch_termtbl_create':
>> drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c:90
>> :1: error: the frame size of 1032 bytes is larger than 1024 bytes [-
>> Werror=frame-larger-than=]
>>
>> Since the structure is never written to, we can statically allocate
>> it to avoid the stack usage. To be on the safe side, mark all
>> subsequent function arguments that we pass it into as 'const'
>> as well.
>>
>> Fixes: 10caabdaad5a ("net/mlx5e: Use termination table for VLAN push
>> actions")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  .../mlx5/core/eswitch_offloads_termtbl.c      |  2 +-
>>  .../net/ethernet/mellanox/mlx5/core/fs_core.c | 20 +++++++++------
>> ----
>>  include/linux/mlx5/fs.h                       |  2 +-
>>  3 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git
>> a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
>> index cb7d8ebe2c95..171f3d4ef9ac 100644
>> ---
>> a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
>> +++
>> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
>> @@ -50,7 +50,7 @@ mlx5_eswitch_termtbl_create(struct mlx5_core_dev
>> *dev,
>>  			    struct mlx5_flow_act *flow_act)
>>  {
>>  	struct mlx5_flow_namespace *root_ns;
>> -	struct mlx5_flow_spec spec = {};
>> +	static const struct mlx5_flow_spec spec = {};
> 
> LGTM, just make sure please to have a reverse xmas tree here.
> 
> Mark, please let me know if you are ok with such API constrain to flow
> steering (spec must be const).

I don't think the steering core layer should change the matching asked by the user.
Even without the frame size issue I like this change, thanks!

LGTM

Thanks,
Mark

> 
> Thanks,
> Saeed.
> 
>>  	int prio, flags;
>>  	int err;
>>  
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
>> index fe76c6fd6d80..739123e1363b 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
>> @@ -584,7 +584,7 @@ static int insert_fte(struct mlx5_flow_group *fg,
>> struct fs_fte *fte)
>>  }
>>  
>>  static struct fs_fte *alloc_fte(struct mlx5_flow_table *ft,
>> -				u32 *match_value,
>> +				const u32 *match_value,
>>  				struct mlx5_flow_act *flow_act)
>>  {
>>  	struct mlx5_flow_steering *steering = get_steering(&ft->node);
>> @@ -612,7 +612,7 @@ static void dealloc_flow_group(struct
>> mlx5_flow_steering *steering,
>>  
>>  static struct mlx5_flow_group *alloc_flow_group(struct
>> mlx5_flow_steering *steering,
>>  						u8
>> match_criteria_enable,
>> -						void *match_criteria,
>> +						const void
>> *match_criteria,
>>  						int start_index,
>>  						int end_index)
>>  {
>> @@ -642,7 +642,7 @@ static struct mlx5_flow_group
>> *alloc_flow_group(struct mlx5_flow_steering *steer
>>  
>>  static struct mlx5_flow_group *alloc_insert_flow_group(struct
>> mlx5_flow_table *ft,
>>  						       u8
>> match_criteria_enable,
>> -						       void
>> *match_criteria,
>> +						       const void
>> *match_criteria,
>>  						       int start_index,
>>  						       int end_index,
>>  						       struct list_head
>> *prev)
>> @@ -1285,7 +1285,7 @@ add_rule_fte(struct fs_fte *fte,
>>  }
>>  
>>  static struct mlx5_flow_group *alloc_auto_flow_group(struct
>> mlx5_flow_table  *ft,
>> -						     struct
>> mlx5_flow_spec *spec)
>> +						     const struct
>> mlx5_flow_spec *spec)
>>  {
>>  	struct list_head *prev = &ft->node.children;
>>  	struct mlx5_flow_group *fg;
>> @@ -1451,7 +1451,7 @@ static int check_conflicting_ftes(struct fs_fte
>> *fte, const struct mlx5_flow_act
>>  }
>>  
>>  static struct mlx5_flow_handle *add_rule_fg(struct mlx5_flow_group
>> *fg,
>> -					    u32 *match_value,
>> +					    const u32 *match_value,
>>  					    struct mlx5_flow_act
>> *flow_act,
>>  					    struct
>> mlx5_flow_destination *dest,
>>  					    int dest_num,
>> @@ -1536,7 +1536,7 @@ static void free_match_list(struct
>> match_list_head *head)
>>  
>>  static int build_match_list(struct match_list_head *match_head,
>>  			    struct mlx5_flow_table *ft,
>> -			    struct mlx5_flow_spec *spec)
>> +			    const struct mlx5_flow_spec *spec)
>>  {
>>  	struct rhlist_head *tmp, *list;
>>  	struct mlx5_flow_group *g;
>> @@ -1589,7 +1589,7 @@ static u64 matched_fgs_get_version(struct
>> list_head *match_head)
>>  
>>  static struct fs_fte *
>>  lookup_fte_locked(struct mlx5_flow_group *g,
>> -		  u32 *match_value,
>> +		  const u32 *match_value,
>>  		  bool take_write)
>>  {
>>  	struct fs_fte *fte_tmp;
>> @@ -1622,7 +1622,7 @@ lookup_fte_locked(struct mlx5_flow_group *g,
>>  static struct mlx5_flow_handle *
>>  try_add_to_existing_fg(struct mlx5_flow_table *ft,
>>  		       struct list_head *match_head,
>> -		       struct mlx5_flow_spec *spec,
>> +		       const struct mlx5_flow_spec *spec,
>>  		       struct mlx5_flow_act *flow_act,
>>  		       struct mlx5_flow_destination *dest,
>>  		       int dest_num,
>> @@ -1715,7 +1715,7 @@ try_add_to_existing_fg(struct mlx5_flow_table
>> *ft,
>>  
>>  static struct mlx5_flow_handle *
>>  _mlx5_add_flow_rules(struct mlx5_flow_table *ft,
>> -		     struct mlx5_flow_spec *spec,
>> +		     const struct mlx5_flow_spec *spec,
>>  		     struct mlx5_flow_act *flow_act,
>>  		     struct mlx5_flow_destination *dest,
>>  		     int dest_num)
>> @@ -1823,7 +1823,7 @@ static bool fwd_next_prio_supported(struct
>> mlx5_flow_table *ft)
>>  
>>  struct mlx5_flow_handle *
>>  mlx5_add_flow_rules(struct mlx5_flow_table *ft,
>> -		    struct mlx5_flow_spec *spec,
>> +		    const struct mlx5_flow_spec *spec,
>>  		    struct mlx5_flow_act *flow_act,
>>  		    struct mlx5_flow_destination *dest,
>>  		    int num_dest)
>> diff --git a/include/linux/mlx5/fs.h b/include/linux/mlx5/fs.h
>> index 2ddaa97f2179..c0c029664527 100644
>> --- a/include/linux/mlx5/fs.h
>> +++ b/include/linux/mlx5/fs.h
>> @@ -200,7 +200,7 @@ struct mlx5_flow_act {
>>   */
>>  struct mlx5_flow_handle *
>>  mlx5_add_flow_rules(struct mlx5_flow_table *ft,
>> -		    struct mlx5_flow_spec *spec,
>> +		    const struct mlx5_flow_spec *spec,
>>  		    struct mlx5_flow_act *flow_act,
>>  		    struct mlx5_flow_destination *dest,
>>  		    int num_dest);