* [RFC -next 0/2] sched: red offload and TC stats @ 2018-01-04 20:18 Jakub Kicinski 2018-01-04 20:18 ` [RFC -next 1/2] net: sched: add qstats.qlen to qlen Jakub Kicinski 2018-01-04 20:18 ` [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat dump Jakub Kicinski 0 siblings, 2 replies; 5+ messages in thread From: Jakub Kicinski @ 2018-01-04 20:18 UTC (permalink / raw) To: john.fastabend, jiri, xiyou.wangcong, Nogah Frankel, Yuval Mintz Cc: netdev, oss-drivers, edumazet, Jakub Kicinski Hi! I've been playing with TC qdisc offloads. I came to a little snag with the momentary stats (qlen and backlog). It seems there is a little bit of legacy magic around the backlog while qlen needs changes to be reported. I would appreciate any comments and guidance. Jakub Kicinski (2): net: sched: add qstats.qlen to qlen net: sched: red: don't reset the backlog on every stat dump drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 1 + drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c | 9 +++++++-- include/net/sch_generic.h | 4 ++-- net/sched/sch_red.c | 1 - 4 files changed, 10 insertions(+), 5 deletions(-) -- 2.15.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC -next 1/2] net: sched: add qstats.qlen to qlen 2018-01-04 20:18 [RFC -next 0/2] sched: red offload and TC stats Jakub Kicinski @ 2018-01-04 20:18 ` Jakub Kicinski 2018-01-04 20:18 ` [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat dump Jakub Kicinski 1 sibling, 0 replies; 5+ messages in thread From: Jakub Kicinski @ 2018-01-04 20:18 UTC (permalink / raw) To: john.fastabend, jiri, xiyou.wangcong, Nogah Frankel, Yuval Mintz Cc: netdev, oss-drivers, edumazet, Jakub Kicinski AFAICT struct gnet_stats_queue.qlen is not used in Qdiscs. It may, however, be useful for offloads to report HW queue length there. Add that value to the result of qdisc_qlen_sum(). Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- include/net/sch_generic.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index ac029d5d88e4..5d8735c0af11 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -310,14 +310,14 @@ static inline int qdisc_qlen(const struct Qdisc *q) static inline int qdisc_qlen_sum(const struct Qdisc *q) { - __u32 qlen = 0; + __u32 qlen = q->qstats.qlen; int i; if (q->flags & TCQ_F_NOLOCK) { for_each_possible_cpu(i) qlen += per_cpu_ptr(q->cpu_qstats, i)->qlen; } else { - qlen = q->q.qlen; + qlen += q->q.qlen; } return qlen; -- 2.15.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat dump 2018-01-04 20:18 [RFC -next 0/2] sched: red offload and TC stats Jakub Kicinski 2018-01-04 20:18 ` [RFC -next 1/2] net: sched: add qstats.qlen to qlen Jakub Kicinski @ 2018-01-04 20:18 ` Jakub Kicinski 2018-01-08 9:18 ` Nogah Frankel 1 sibling, 1 reply; 5+ messages in thread From: Jakub Kicinski @ 2018-01-04 20:18 UTC (permalink / raw) To: john.fastabend, jiri, xiyou.wangcong, Nogah Frankel, Yuval Mintz Cc: netdev, oss-drivers, edumazet, Jakub Kicinski Commit 0dfb33a0d7e2 ("sch_red: report backlog information") copied child's backlog into RED's backlog. Back then RED did not maintain its own backlog counts. This has changed after commit 2ccccf5fb43f ("net_sched: update hierarchical backlog too") and commit d7f4f332f082 ("sch_red: update backlog as well"). Copying is no longer necessary. Tested: $ tc -s qdisc show dev veth0 qdisc red 1: root refcnt 2 limit 400000b min 30000b max 30000b ecn Sent 20942 bytes 221 pkt (dropped 0, overlimits 0 requeues 0) backlog 1260b 14p requeues 14 marked 0 early 0 pdrop 0 other 0 qdisc tbf 2: parent 1: rate 1Kbit burst 15000b lat 3585.0s Sent 20942 bytes 221 pkt (dropped 0, overlimits 138 requeues 0) backlog 1260b 14p requeues 14 Recently RED offload was added. We need to make sure drivers don't depend on resetting the stats. This means backlog should be treated like any other statistic: total_stat = new_hw_stat - prev_hw_stat; Unlike for other statistics new_hw_stat < prev_hw_stat can be true. Adjust mlxsw. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 1 + drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c | 9 +++++++-- net/sched/sch_red.c | 1 - 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h index ff8d32bc852c..6755050e4ee0 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h @@ -237,6 +237,7 @@ struct mlxsw_sp_qdisc { u64 tx_packets; u64 drops; u64 overlimits; + u64 backlog; }; /* No need an internal lock; At worse - miss a single periodic iteration */ diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c index c33beac5def0..d5091740bd40 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c @@ -212,6 +212,7 @@ mlxsw_sp_qdisc_get_red_stats(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle, u64 tx_bytes, tx_packets, overlimits, drops; struct mlxsw_sp_port_xstats *xstats; struct rtnl_link_stats64 *stats; + s64 backlog; if (mlxsw_sp_qdisc->handle != handle || mlxsw_sp_qdisc->type != MLXSW_SP_QDISC_RED) @@ -226,17 +227,21 @@ mlxsw_sp_qdisc_get_red_stats(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle, mlxsw_sp_qdisc->overlimits; drops = xstats->wred_drop[tclass_num] + xstats->tail_drop[tclass_num] - mlxsw_sp_qdisc->drops; + backlog = mlxsw_sp_cells_bytes(mlxsw_sp_port->mlxsw_sp, + xstats->backlog[tclass_num]) - + mlxsw_sp_qdisc->backlog; _bstats_update(res->bstats, tx_bytes, tx_packets); res->qstats->overlimits += overlimits; res->qstats->drops += drops; - res->qstats->backlog += mlxsw_sp_cells_bytes(mlxsw_sp_port->mlxsw_sp, - xstats->backlog[tclass_num]); + res->qstats->backlog += backlog; mlxsw_sp_qdisc->drops += drops; mlxsw_sp_qdisc->overlimits += overlimits; mlxsw_sp_qdisc->tx_bytes += tx_bytes; mlxsw_sp_qdisc->tx_packets += tx_packets; + mlxsw_sp_qdisc->backlog += backlog; + return 0; } diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c index a392eaa4a0b4..caebb7e37551 100644 --- a/net/sched/sch_red.c +++ b/net/sched/sch_red.c @@ -322,7 +322,6 @@ static int red_dump(struct Qdisc *sch, struct sk_buff *skb) }; int err; - sch->qstats.backlog = q->qdisc->qstats.backlog; err = red_dump_offload_stats(sch, &opt); if (err) goto nla_put_failure; -- 2.15.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat dump 2018-01-04 20:18 ` [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat dump Jakub Kicinski @ 2018-01-08 9:18 ` Nogah Frankel 2018-01-08 18:23 ` Jakub Kicinski 0 siblings, 1 reply; 5+ messages in thread From: Nogah Frankel @ 2018-01-08 9:18 UTC (permalink / raw) To: Jakub Kicinski, john.fastabend@gmail.com, jiri@resnulli.us, xiyou.wangcong@gmail.com, Yuval Mintz Cc: netdev@vger.kernel.org, oss-drivers@netronome.com, edumazet@google.com > -----Original Message----- > From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com] > Sent: Thursday, January 04, 2018 10:19 PM > To: john.fastabend@gmail.com; jiri@resnulli.us; xiyou.wangcong@gmail.com; > Nogah Frankel <nogahf@mellanox.com>; Yuval Mintz > <yuvalm@mellanox.com> > Cc: netdev@vger.kernel.org; oss-drivers@netronome.com; > edumazet@google.com; Jakub Kicinski <jakub.kicinski@netronome.com> > Subject: [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat > dump > > Commit 0dfb33a0d7e2 ("sch_red: report backlog information") copied > child's backlog into RED's backlog. Back then RED did not maintain > its own backlog counts. This has changed after commit 2ccccf5fb43f > ("net_sched: update hierarchical backlog too") and commit d7f4f332f082 > ("sch_red: update backlog as well"). Copying is no longer necessary. > > Tested: > > $ tc -s qdisc show dev veth0 > qdisc red 1: root refcnt 2 limit 400000b min 30000b max 30000b ecn > Sent 20942 bytes 221 pkt (dropped 0, overlimits 0 requeues 0) > backlog 1260b 14p requeues 14 > marked 0 early 0 pdrop 0 other 0 > qdisc tbf 2: parent 1: rate 1Kbit burst 15000b lat 3585.0s > Sent 20942 bytes 221 pkt (dropped 0, overlimits 138 requeues 0) > backlog 1260b 14p requeues 14 > > Recently RED offload was added. We need to make sure drivers don't > depend on resetting the stats. This means backlog should be treated > like any other statistic: > > total_stat = new_hw_stat - prev_hw_stat; > > Unlike for other statistics new_hw_stat < prev_hw_stat can be true. > Adjust mlxsw. There is one problem with this patch, and that is that we can fail in changing RED that is offloaded. In this case, we delete RED from the driver but the backlog will still include the hardware backlog. The solution is to send in the offload-replace command a pointer to the backlog, so failure in updating the hardware can be follow by backlog update, if needed. Thanks Nogah > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 1 + > drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c | 9 +++++++-- > net/sched/sch_red.c | 1 - > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > index ff8d32bc852c..6755050e4ee0 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > @@ -237,6 +237,7 @@ struct mlxsw_sp_qdisc { > u64 tx_packets; > u64 drops; > u64 overlimits; > + u64 backlog; > }; > > /* No need an internal lock; At worse - miss a single periodic iteration */ > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c > b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c > index c33beac5def0..d5091740bd40 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c > @@ -212,6 +212,7 @@ mlxsw_sp_qdisc_get_red_stats(struct mlxsw_sp_port > *mlxsw_sp_port, u32 handle, > u64 tx_bytes, tx_packets, overlimits, drops; > struct mlxsw_sp_port_xstats *xstats; > struct rtnl_link_stats64 *stats; > + s64 backlog; > > if (mlxsw_sp_qdisc->handle != handle || > mlxsw_sp_qdisc->type != MLXSW_SP_QDISC_RED) > @@ -226,17 +227,21 @@ mlxsw_sp_qdisc_get_red_stats(struct > mlxsw_sp_port *mlxsw_sp_port, u32 handle, > mlxsw_sp_qdisc->overlimits; > drops = xstats->wred_drop[tclass_num] + xstats- > >tail_drop[tclass_num] - > mlxsw_sp_qdisc->drops; > + backlog = mlxsw_sp_cells_bytes(mlxsw_sp_port->mlxsw_sp, > + xstats->backlog[tclass_num]) - > + mlxsw_sp_qdisc->backlog; > > _bstats_update(res->bstats, tx_bytes, tx_packets); > res->qstats->overlimits += overlimits; > res->qstats->drops += drops; > - res->qstats->backlog += mlxsw_sp_cells_bytes(mlxsw_sp_port- > >mlxsw_sp, > - xstats->backlog[tclass_num]); > + res->qstats->backlog += backlog; > > mlxsw_sp_qdisc->drops += drops; > mlxsw_sp_qdisc->overlimits += overlimits; > mlxsw_sp_qdisc->tx_bytes += tx_bytes; > mlxsw_sp_qdisc->tx_packets += tx_packets; > + mlxsw_sp_qdisc->backlog += backlog; > + > return 0; > } > > diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c > index a392eaa4a0b4..caebb7e37551 100644 > --- a/net/sched/sch_red.c > +++ b/net/sched/sch_red.c > @@ -322,7 +322,6 @@ static int red_dump(struct Qdisc *sch, struct sk_buff > *skb) > }; > int err; > > - sch->qstats.backlog = q->qdisc->qstats.backlog; > err = red_dump_offload_stats(sch, &opt); > if (err) > goto nla_put_failure; > -- > 2.15.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat dump 2018-01-08 9:18 ` Nogah Frankel @ 2018-01-08 18:23 ` Jakub Kicinski 0 siblings, 0 replies; 5+ messages in thread From: Jakub Kicinski @ 2018-01-08 18:23 UTC (permalink / raw) To: Nogah Frankel Cc: john.fastabend@gmail.com, jiri@resnulli.us, xiyou.wangcong@gmail.com, Yuval Mintz, netdev@vger.kernel.org, oss-drivers@netronome.com, edumazet@google.com On Mon, 8 Jan 2018 09:18:08 +0000, Nogah Frankel wrote: > > Unlike for other statistics new_hw_stat < prev_hw_stat can be true. > > Adjust mlxsw. > > There is one problem with this patch, and that is that we can fail in > changing RED that is offloaded. In this case, we delete RED from the driver > but the backlog will still include the hardware backlog. > The solution is to send in the offload-replace command a pointer to the > backlog, so failure in updating the hardware can be follow by backlog update, > if needed. Thanks, will fix. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-08 18:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-04 20:18 [RFC -next 0/2] sched: red offload and TC stats Jakub Kicinski 2018-01-04 20:18 ` [RFC -next 1/2] net: sched: add qstats.qlen to qlen Jakub Kicinski 2018-01-04 20:18 ` [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat dump Jakub Kicinski 2018-01-08 9:18 ` Nogah Frankel 2018-01-08 18:23 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).