| Message ID | 20190116150210.12357-1-ivecera@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] net/sched: cls_flower: allocate mask dynamically in fl_change() | expand |
Wed, Jan 16, 2019 at 04:02:10PM CET, ivecera@redhat.com wrote: >Recent changes (especially 05cd271fd61a ("cls_flower: Support multiple >masks per priority")) in the fl_flow_mask structure grow it and its >current size e.g. on x86_64 with defconfig is 760 bytes and more than >1024 bytes with some debug options enabled. Prior the mentioned commit >its size was 176 bytes (using defconfig on x86_64). >With regard to this fact it's reasonable to allocate this structure >dynamically in fl_change() to reduce its stack size. > >Fixes: 05cd271fd61a ("cls_flower: Support multiple masks per priority") Not sure this is -net material. But I don't mind. >Cc: Jiri Pirko <jiri@resnulli.us> >Cc: Paul Blakey <paulb@mellanox.com> >Signed-off-by: Ivan Vecera <ivecera@redhat.com> >--- > net/sched/cls_flower.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > >diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c >index dad04e710493..47ac88cc34fb 100644 >--- a/net/sched/cls_flower.c >+++ b/net/sched/cls_flower.c >@@ -1291,16 +1291,22 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, > struct cls_fl_filter *fold = *arg; > struct cls_fl_filter *fnew; > struct nlattr **tb; >- struct fl_flow_mask mask = {}; >+ struct fl_flow_mask *mask; > int err; > > if (!tca[TCA_OPTIONS]) > return -EINVAL; > >- tb = kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL); >- if (!tb) >+ mask = kcalloc(1, sizeof(struct fl_flow_mask), GFP_KERNEL); kzalloc? Otherwise this looks fine. Acked-by: Jiri Pirko <jiri@mellanox.com> >+ if (!mask) > return -ENOBUFS; > >+ tb = kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL); >+ if (!tb) { >+ err = -ENOBUFS; >+ goto errout_mask_alloc; >+ } >+ > err = nla_parse_nested(tb, TCA_FLOWER_MAX, tca[TCA_OPTIONS], > fl_policy, NULL); > if (err < 0) >@@ -1343,12 +1349,12 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, > } > } > >- err = fl_set_parms(net, tp, fnew, &mask, base, tb, tca[TCA_RATE], ovr, >+ err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE], ovr, > tp->chain->tmplt_priv, extack); > if (err) > goto errout_idr; > >- err = fl_check_assign_mask(head, fnew, fold, &mask); >+ err = fl_check_assign_mask(head, fnew, fold, mask); > if (err) > goto errout_idr; > >@@ -1392,6 +1398,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, > } > > kfree(tb); >+ kfree(mask); > return 0; > > errout_mask: >@@ -1405,6 +1412,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, > kfree(fnew); > errout_tb: > kfree(tb); >+errout_mask_alloc: >+ kfree(mask); > return err; > } > >-- >2.19.2 >
On 16. 01. 19 16:24, Jiri Pirko wrote: > Wed, Jan 16, 2019 at 04:02:10PM CET, ivecera@redhat.com wrote: >> Recent changes (especially 05cd271fd61a ("cls_flower: Support multiple >> masks per priority")) in the fl_flow_mask structure grow it and its >> current size e.g. on x86_64 with defconfig is 760 bytes and more than >> 1024 bytes with some debug options enabled. Prior the mentioned commit >> its size was 176 bytes (using defconfig on x86_64). >> With regard to this fact it's reasonable to allocate this structure >> dynamically in fl_change() to reduce its stack size. >> >> Fixes: 05cd271fd61a ("cls_flower: Support multiple masks per priority") > > Not sure this is -net material. But I don't mind. > > >> Cc: Jiri Pirko <jiri@resnulli.us> >> Cc: Paul Blakey <paulb@mellanox.com> >> Signed-off-by: Ivan Vecera <ivecera@redhat.com> >> --- >> net/sched/cls_flower.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c >> index dad04e710493..47ac88cc34fb 100644 >> --- a/net/sched/cls_flower.c >> +++ b/net/sched/cls_flower.c >> @@ -1291,16 +1291,22 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, >> struct cls_fl_filter *fold = *arg; >> struct cls_fl_filter *fnew; >> struct nlattr **tb; >> - struct fl_flow_mask mask = {}; >> + struct fl_flow_mask *mask; >> int err; >> >> if (!tca[TCA_OPTIONS]) >> return -EINVAL; >> >> - tb = kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL); >> - if (!tb) >> + mask = kcalloc(1, sizeof(struct fl_flow_mask), GFP_KERNEL); > > kzalloc? Sure.. excuse. > Otherwise this looks fine. > Acked-by: Jiri Pirko <jiri@mellanox.com> Thx. > > > >> + if (!mask) >> return -ENOBUFS; >> >> + tb = kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL); >> + if (!tb) { >> + err = -ENOBUFS; >> + goto errout_mask_alloc; >> + } >> + >> err = nla_parse_nested(tb, TCA_FLOWER_MAX, tca[TCA_OPTIONS], >> fl_policy, NULL); >> if (err < 0) >> @@ -1343,12 +1349,12 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, >> } >> } >> >> - err = fl_set_parms(net, tp, fnew, &mask, base, tb, tca[TCA_RATE], ovr, >> + err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE], ovr, >> tp->chain->tmplt_priv, extack); >> if (err) >> goto errout_idr; >> >> - err = fl_check_assign_mask(head, fnew, fold, &mask); >> + err = fl_check_assign_mask(head, fnew, fold, mask); >> if (err) >> goto errout_idr; >> >> @@ -1392,6 +1398,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, >> } >> >> kfree(tb); >> + kfree(mask); >> return 0; >> >> errout_mask: >> @@ -1405,6 +1412,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, >> kfree(fnew); >> errout_tb: >> kfree(tb); >> +errout_mask_alloc: >> + kfree(mask); >> return err; >> } >> >> -- >> 2.19.2 >>
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index dad04e710493..47ac88cc34fb 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -1291,16 +1291,22 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, struct cls_fl_filter *fold = *arg; struct cls_fl_filter *fnew; struct nlattr **tb; - struct fl_flow_mask mask = {}; + struct fl_flow_mask *mask; int err; if (!tca[TCA_OPTIONS]) return -EINVAL; - tb = kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL); - if (!tb) + mask = kcalloc(1, sizeof(struct fl_flow_mask), GFP_KERNEL); + if (!mask) return -ENOBUFS; + tb = kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL); + if (!tb) { + err = -ENOBUFS; + goto errout_mask_alloc; + } + err = nla_parse_nested(tb, TCA_FLOWER_MAX, tca[TCA_OPTIONS], fl_policy, NULL); if (err < 0) @@ -1343,12 +1349,12 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, } } - err = fl_set_parms(net, tp, fnew, &mask, base, tb, tca[TCA_RATE], ovr, + err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE], ovr, tp->chain->tmplt_priv, extack); if (err) goto errout_idr; - err = fl_check_assign_mask(head, fnew, fold, &mask); + err = fl_check_assign_mask(head, fnew, fold, mask); if (err) goto errout_idr; @@ -1392,6 +1398,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, } kfree(tb); + kfree(mask); return 0; errout_mask: @@ -1405,6 +1412,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, kfree(fnew); errout_tb: kfree(tb); +errout_mask_alloc: + kfree(mask); return err; }
Recent changes (especially 05cd271fd61a ("cls_flower: Support multiple masks per priority")) in the fl_flow_mask structure grow it and its current size e.g. on x86_64 with defconfig is 760 bytes and more than 1024 bytes with some debug options enabled. Prior the mentioned commit its size was 176 bytes (using defconfig on x86_64). With regard to this fact it's reasonable to allocate this structure dynamically in fl_change() to reduce its stack size. Fixes: 05cd271fd61a ("cls_flower: Support multiple masks per priority") Cc: Jiri Pirko <jiri@resnulli.us> Cc: Paul Blakey <paulb@mellanox.com> Signed-off-by: Ivan Vecera <ivecera@redhat.com> --- net/sched/cls_flower.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)