| Message ID | 20160823202445.14368.4352.stgit@john-Precision-Tower-5810 |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote: > Enable dflt qdisc support for per cpu stats before this patch a > dflt qdisc was required to use the global statistics qstats and > bstats. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > net/sched/sch_generic.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > index 80544c2..910b4d15 100644 > --- a/net/sched/sch_generic.c > +++ b/net/sched/sch_generic.c > @@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue, > struct Qdisc *sch; > > if (!try_module_get(ops->owner)) > - goto errout; > + return NULL; > > sch = qdisc_alloc(dev_queue, ops); > if (IS_ERR(sch)) > - goto errout; > + return NULL; > sch->parent = parentid; > > - if (!ops->init || ops->init(sch, NULL) == 0) > + if (!ops->init) > return sch; > > - qdisc_destroy(sch); > + if (ops->init(sch, NULL)) > + goto errout; > + > + /* init() may have set percpu flags so init data structures */ > + if (qdisc_is_percpu_stats(sch)) { > + sch->cpu_bstats = > + netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu); > + if (!sch->cpu_bstats) > + goto errout; > + > + sch->cpu_qstats = alloc_percpu(struct gnet_stats_queue); > + if (!sch->cpu_qstats) > + goto errout; > + } > + > + return sch; > errout: > + qdisc_destroy(sch); > return NULL; > } > EXPORT_SYMBOL(qdisc_create_dflt); > Hmm... apparently we have bug here, added in 6da7c8fcbcbdb50ec ("qdisc: allow setting default queuing discipline") We do not undo the try_module_get() in case of an error. I will send a fix.
On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote: > Enable dflt qdisc support for per cpu stats before this patch a > dflt qdisc was required to use the global statistics qstats and > bstats. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > net/sched/sch_generic.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > index 80544c2..910b4d15 100644 > --- a/net/sched/sch_generic.c > +++ b/net/sched/sch_generic.c > @@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue, > struct Qdisc *sch; > > if (!try_module_get(ops->owner)) > - goto errout; > + return NULL; > > sch = qdisc_alloc(dev_queue, ops); > if (IS_ERR(sch)) > - goto errout; > + return NULL; > sch->parent = parentid; > > - if (!ops->init || ops->init(sch, NULL) == 0) > + if (!ops->init) > return sch; > > - qdisc_destroy(sch); > + if (ops->init(sch, NULL)) > + goto errout; > + > + /* init() may have set percpu flags so init data structures */ > + if (qdisc_is_percpu_stats(sch)) { > + sch->cpu_bstats = > + netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu); > + if (!sch->cpu_bstats) > + goto errout; > + > + sch->cpu_qstats = alloc_percpu(struct gnet_stats_queue); > + if (!sch->cpu_qstats) > + goto errout; > + } > + Why are you attempting these allocations here instead of qdisc_alloc() This looks weird, I would expect base qdisc being fully allocated before ops->init() is attempted.
On 16-08-24 09:41 AM, Eric Dumazet wrote: > On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote: >> Enable dflt qdisc support for per cpu stats before this patch a >> dflt qdisc was required to use the global statistics qstats and >> bstats. >> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >> --- >> net/sched/sch_generic.c | 24 ++++++++++++++++++++---- >> 1 file changed, 20 insertions(+), 4 deletions(-) >> >> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c >> index 80544c2..910b4d15 100644 >> --- a/net/sched/sch_generic.c >> +++ b/net/sched/sch_generic.c >> @@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue, >> struct Qdisc *sch; >> >> if (!try_module_get(ops->owner)) >> - goto errout; >> + return NULL; >> >> sch = qdisc_alloc(dev_queue, ops); >> if (IS_ERR(sch)) >> - goto errout; >> + return NULL; >> sch->parent = parentid; >> >> - if (!ops->init || ops->init(sch, NULL) == 0) >> + if (!ops->init) >> return sch; >> >> - qdisc_destroy(sch); >> + if (ops->init(sch, NULL)) >> + goto errout; >> + >> + /* init() may have set percpu flags so init data structures */ >> + if (qdisc_is_percpu_stats(sch)) { >> + sch->cpu_bstats = >> + netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu); >> + if (!sch->cpu_bstats) >> + goto errout; >> + >> + sch->cpu_qstats = alloc_percpu(struct gnet_stats_queue); >> + if (!sch->cpu_qstats) >> + goto errout; >> + } >> + > > Why are you attempting these allocations here instead of qdisc_alloc() > > This looks weird, I would expect base qdisc being fully allocated before > ops->init() is attempted. > > > I could fully allocate it in qdisc_alloc() but we don't know if the qdisc needs per cpu data structures until after the init call. So it would sit unused in those cases if done from qdisc_alloc(). It seems best to me at least to just avoid the allocation in qdisc_alloc() and do it after init like I did here. Perhaps it would be nice to pull these into a function call post_init_qdisc_alloc() that does all this allocation? .John
On Wed, 2016-08-24 at 10:13 -0700, John Fastabend wrote: > > > > I could fully allocate it in qdisc_alloc() but we don't know if the > qdisc needs per cpu data structures until after the init call Should not we have a flag to advertise the need of per spu stats on qdisc ? This is not clear why ->init() can know this, and not its caller. > . So it > would sit unused in those cases if done from qdisc_alloc(). It seems > best to me at least to just avoid the allocation in qdisc_alloc() and > do it after init like I did here. > > Perhaps it would be nice to pull these into a function call > post_init_qdisc_alloc() that does all this allocation? > > .John >
On 16-08-24 10:26 AM, Eric Dumazet wrote: > On Wed, 2016-08-24 at 10:13 -0700, John Fastabend wrote: > >>> >> >> I could fully allocate it in qdisc_alloc() but we don't know if the >> qdisc needs per cpu data structures until after the init call > > Should not we have a flag to advertise the need of per spu stats on > qdisc ? > > This is not clear why ->init() can know this, and not its caller. > sure we could be a static_flags field in the ops structure. What do you think about doing that? We would still need some flags to be set at init though like the bypass bit it looks like some qdiscs set that based on user input. >> . So it >> would sit unused in those cases if done from qdisc_alloc(). It seems >> best to me at least to just avoid the allocation in qdisc_alloc() and >> do it after init like I did here. >> >> Perhaps it would be nice to pull these into a function call >> post_init_qdisc_alloc() that does all this allocation? >> >> .John >> > >
On Wed, 2016-08-24 at 10:50 -0700, John Fastabend wrote: > On 16-08-24 10:26 AM, Eric Dumazet wrote: > > On Wed, 2016-08-24 at 10:13 -0700, John Fastabend wrote: > > > >>> > >> > >> I could fully allocate it in qdisc_alloc() but we don't know if the > >> qdisc needs per cpu data structures until after the init call > > > > Should not we have a flag to advertise the need of per spu stats on > > qdisc ? > > > > This is not clear why ->init() can know this, and not its caller. > > > > sure we could be a static_flags field in the ops structure. What do > you think about doing that? This is what I was suggesting yes. > > We would still need some flags to be set at init though like the bypass > bit it looks like some qdiscs set that based on user input. >
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 80544c2..910b4d15 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue, struct Qdisc *sch; if (!try_module_get(ops->owner)) - goto errout; + return NULL; sch = qdisc_alloc(dev_queue, ops); if (IS_ERR(sch)) - goto errout; + return NULL; sch->parent = parentid; - if (!ops->init || ops->init(sch, NULL) == 0) + if (!ops->init) return sch; - qdisc_destroy(sch); + if (ops->init(sch, NULL)) + goto errout; + + /* init() may have set percpu flags so init data structures */ + if (qdisc_is_percpu_stats(sch)) { + sch->cpu_bstats = + netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu); + if (!sch->cpu_bstats) + goto errout; + + sch->cpu_qstats = alloc_percpu(struct gnet_stats_queue); + if (!sch->cpu_qstats) + goto errout; + } + + return sch; errout: + qdisc_destroy(sch); return NULL; } EXPORT_SYMBOL(qdisc_create_dflt);
Enable dflt qdisc support for per cpu stats before this patch a dflt qdisc was required to use the global statistics qstats and bstats. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- net/sched/sch_generic.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)