| Message ID | 20190617110855.2085326-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net/mlx5e: reduce stack usage in mlx5_eswitch_termtbl_create | expand |
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). 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);
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);
From: Arnd Bergmann <arnd@arndb.de> Date: Mon, 17 Jun 2019 13:08:22 +0200 > 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> Saeed, once Arnd fixes the reverse christmas tree issue, I assume you will take this in via your tree? Thanks.
On Mon, 2019-06-17 at 14:02 -0700, David Miller wrote: > From: Arnd Bergmann <arnd@arndb.de> > Date: Mon, 17 Jun 2019 13:08:22 +0200 > > > 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> > > Saeed, once Arnd fixes the reverse christmas tree issue, I assume you > will take > this in via your tree? Yes, this is a core change that might conflict with some ongoing stuff. Thank you Dave ! Saeed.
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 = {}; 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);
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(-)