Message ID | 20211019191544.3063872-1-arnd@kernel.org (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [v2] net: sched: gred: dynamically allocate tc_gred_qopt_offload | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | success | Single patches do not need cover letters |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Guessed tree name to be net-next |
netdev/subject_prefix | warning | Target tree name not specified in the subject |
netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | Fixes tag looks correct |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 96 lines checked |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
netdev/header_inline | success | No static functions without inline keyword in header files |
On Tue, 19 Oct 2021 21:15:29 +0200 Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The tc_gred_qopt_offload structure has grown too big to be on the > stack for 32-bit architectures after recent changes. > > net/sched/sch_gred.c:903:13: error: stack frame size (1180) exceeds limit (1024) in 'gred_destroy' [-Werror,-Wframe-larger-than] > net/sched/sch_gred.c:310:13: error: stack frame size (1212) exceeds limit (1024) in 'gred_offload' [-Werror,-Wframe-larger-than] > > Use dynamic allocation per qdisc to avoid this. > > Fixes: 50dc9a8572aa ("net: sched: Merge Qdisc::bstats and Qdisc::cpu_bstats data types") > Fixes: 67c9e6270f30 ("net: sched: Protect Qdisc::bstats with u64_stats") > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Hi Jakub, > > Not sure if this is what you had in mind, if not it might be easier > if you do it yourself. In particular, adding tc_gred_qopt_offload > to gred_sched directly rather than as a pointer may be easier here, > but that may have other downsides. This is exactly what I had in mind, thanks! Two minor nits if you're willing to respin, if you feel like you've spent enough time on this already we can marge as is :) > - opt.set.qstats = &sch->qstats; > + opt->set.qstats = &sch->qstats; > } > > - dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, &opt); > + dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, opt); > + > + return; return no longer needed > } > > static int gred_offload_dump_stats(struct Qdisc *sch) > @@ -754,6 +759,10 @@ static int gred_init(struct Qdisc *sch, struct nlattr *opt, > sch->limit = qdisc_dev(sch)->tx_queue_len > * psched_mtu(qdisc_dev(sch)); The ops should not change, so I think we can do if (qdisc_dev(sch)->netdev_ops->ndo_setup_tc) { > + table->opt = kzalloc(sizeof(table->opt), GFP_KERNEL); > + if (!table->opt) > + return -ENOMEM; > + > return gred_change_table_def(sch, tb[TCA_GRED_DPS], extack); > } >
On 10/19/21 12:15 PM, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > > @@ -754,6 +759,10 @@ static int gred_init(struct Qdisc *sch, struct nlattr *opt, > sch->limit = qdisc_dev(sch)->tx_queue_len > * psched_mtu(qdisc_dev(sch)); > > + table->opt = kzalloc(sizeof(table->opt), GFP_KERNEL); sizeof(*table->opt) ? > + if (!table->opt) > + return -ENOMEM; > + > return gred_change_table_def(sch, tb[TCA_GRED_DPS], extack); > } >
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c index 72de08ef8335..d44958f3411a 100644 --- a/net/sched/sch_gred.c +++ b/net/sched/sch_gred.c @@ -56,6 +56,7 @@ struct gred_sched { u32 DPs; u32 def; struct red_vars wred_set; + struct tc_gred_qopt_offload *opt; }; static inline int gred_wred_mode(struct gred_sched *table) @@ -311,42 +312,45 @@ static void gred_offload(struct Qdisc *sch, enum tc_gred_command command) { struct gred_sched *table = qdisc_priv(sch); struct net_device *dev = qdisc_dev(sch); - struct tc_gred_qopt_offload opt = { - .command = command, - .handle = sch->handle, - .parent = sch->parent, - }; + struct tc_gred_qopt_offload *opt = table->opt; if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc) return; + memset(opt, 0, sizeof(*opt)); + opt->command = command; + opt->handle = sch->handle; + opt->parent = sch->parent; + if (command == TC_GRED_REPLACE) { unsigned int i; - opt.set.grio_on = gred_rio_mode(table); - opt.set.wred_on = gred_wred_mode(table); - opt.set.dp_cnt = table->DPs; - opt.set.dp_def = table->def; + opt->set.grio_on = gred_rio_mode(table); + opt->set.wred_on = gred_wred_mode(table); + opt->set.dp_cnt = table->DPs; + opt->set.dp_def = table->def; for (i = 0; i < table->DPs; i++) { struct gred_sched_data *q = table->tab[i]; if (!q) continue; - opt.set.tab[i].present = true; - opt.set.tab[i].limit = q->limit; - opt.set.tab[i].prio = q->prio; - opt.set.tab[i].min = q->parms.qth_min >> q->parms.Wlog; - opt.set.tab[i].max = q->parms.qth_max >> q->parms.Wlog; - opt.set.tab[i].is_ecn = gred_use_ecn(q); - opt.set.tab[i].is_harddrop = gred_use_harddrop(q); - opt.set.tab[i].probability = q->parms.max_P; - opt.set.tab[i].backlog = &q->backlog; + opt->set.tab[i].present = true; + opt->set.tab[i].limit = q->limit; + opt->set.tab[i].prio = q->prio; + opt->set.tab[i].min = q->parms.qth_min >> q->parms.Wlog; + opt->set.tab[i].max = q->parms.qth_max >> q->parms.Wlog; + opt->set.tab[i].is_ecn = gred_use_ecn(q); + opt->set.tab[i].is_harddrop = gred_use_harddrop(q); + opt->set.tab[i].probability = q->parms.max_P; + opt->set.tab[i].backlog = &q->backlog; } - opt.set.qstats = &sch->qstats; + opt->set.qstats = &sch->qstats; } - dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, &opt); + dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, opt); + + return; } static int gred_offload_dump_stats(struct Qdisc *sch) @@ -731,6 +735,7 @@ static int gred_change(struct Qdisc *sch, struct nlattr *opt, static int gred_init(struct Qdisc *sch, struct nlattr *opt, struct netlink_ext_ack *extack) { + struct gred_sched *table = qdisc_priv(sch); struct nlattr *tb[TCA_GRED_MAX + 1]; int err; @@ -754,6 +759,10 @@ static int gred_init(struct Qdisc *sch, struct nlattr *opt, sch->limit = qdisc_dev(sch)->tx_queue_len * psched_mtu(qdisc_dev(sch)); + table->opt = kzalloc(sizeof(table->opt), GFP_KERNEL); + if (!table->opt) + return -ENOMEM; + return gred_change_table_def(sch, tb[TCA_GRED_DPS], extack); } @@ -910,6 +919,7 @@ static void gred_destroy(struct Qdisc *sch) gred_destroy_vq(table->tab[i]); } gred_offload(sch, TC_GRED_DESTROY); + kfree(table->opt); } static struct Qdisc_ops gred_qdisc_ops __read_mostly = {