netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <jakub.kicinski@netronome.com>
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
Date: Thu,  4 Jan 2018 12:18:51 -0800	[thread overview]
Message-ID: <20180104201851.6455-3-jakub.kicinski@netronome.com> (raw)
In-Reply-To: <20180104201851.6455-1-jakub.kicinski@netronome.com>

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

  parent reply	other threads:[~2018-01-04 20:19 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2018-01-08  9:18   ` [RFC -next 2/2] net: sched: red: don't reset the backlog on every stat dump Nogah Frankel
2018-01-08 18:23     ` Jakub Kicinski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180104201851.6455-3-jakub.kicinski@netronome.com \
    --to=jakub.kicinski@netronome.com \
    --cc=edumazet@google.com \
    --cc=jiri@resnulli.us \
    --cc=john.fastabend@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=nogahf@mellanox.com \
    --cc=oss-drivers@netronome.com \
    --cc=xiyou.wangcong@gmail.com \
    --cc=yuvalm@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).