[PATCH net-next 0/2] sched: improve behaviour of offloaded RED stats

[PATCH net-next 0/2] sched: improve behaviour of offloaded RED stats

[Jakub Kicinski]

Hi!

This set attempts to improve the kernel API for TC qdisc offloads.
The cumulative stats are handled nicely today, but the momentary
stats like backlog and qlen are behaving a little less cleanly.

v1:
 - reset the stats on destroy (incl. replace with unsupported params).

Jakub Kicinski (2):
  net: sched: add qstats.qlen to qlen
  net: sched: red: don't reset the backlog on every stat dump

 .../net/ethernet/mellanox/mlxsw/spectrum_qdisc.c   | 35 ++++++++++++++--------
 include/net/pkt_cls.h                              |  5 +++-
 include/net/sch_generic.h                          |  4 +--
 net/sched/sch_red.c                                |  3 +-
 4 files changed, 31 insertions(+), 16 deletions(-)

-- 
2.15.1

[PATCH net-next 1/2] net: sched: add qstats.qlen to qlen

[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

[PATCH net-next 2/2] net: sched: red: don't reset the backlog on every stat dump

[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.
As we add support for offload of other qdiscs this dependency on a
second copy of backlog length for resetting to pre-offload values
will become cumbersome.

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 other stats drivers also need to remove their adjustment
on destroy (or replace with unsupported parameters).  Note that
new_hw_stat < prev_hw_stat can be true for momentary stats.

Adjust mlxsw.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum_qdisc.c   | 35 ++++++++++++++--------
 include/net/pkt_cls.h                              |  5 +++-
 net/sched/sch_red.c                                |  3 +-
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
index 273300b75a68..50031ab484af 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_qdisc.c
@@ -54,7 +54,8 @@ struct mlxsw_sp_qdisc_ops {
 	int (*replace)(struct mlxsw_sp_port *mlxsw_sp_port,
 		       struct mlxsw_sp_qdisc *mlxsw_sp_qdisc, void *params);
 	int (*destroy)(struct mlxsw_sp_port *mlxsw_sp_port,
-		       struct mlxsw_sp_qdisc *mlxsw_sp_qdisc);
+		       struct mlxsw_sp_qdisc *mlxsw_sp_qdisc,
+		       struct gnet_stats_queue *qstats);
 	int (*get_stats)(struct mlxsw_sp_port *mlxsw_sp_port,
 			 struct mlxsw_sp_qdisc *mlxsw_sp_qdisc,
 			 struct tc_qopt_offload_stats *stats_ptr);
@@ -76,6 +77,7 @@ struct mlxsw_sp_qdisc {
 		u64 tx_packets;
 		u64 drops;
 		u64 overlimits;
+		u64 backlog;
 	} stats_base;
 
 	struct mlxsw_sp_qdisc_ops *ops;
@@ -92,7 +94,8 @@ mlxsw_sp_qdisc_compare(struct mlxsw_sp_qdisc *mlxsw_sp_qdisc, u32 handle,
 
 static int
 mlxsw_sp_qdisc_destroy(struct mlxsw_sp_port *mlxsw_sp_port,
-		       struct mlxsw_sp_qdisc *mlxsw_sp_qdisc)
+		       struct mlxsw_sp_qdisc *mlxsw_sp_qdisc,
+		       struct gnet_stats_queue *qstats)
 {
 	int err = 0;
 
@@ -101,7 +104,8 @@ mlxsw_sp_qdisc_destroy(struct mlxsw_sp_port *mlxsw_sp_port,
 
 	if (mlxsw_sp_qdisc->ops && mlxsw_sp_qdisc->ops->destroy)
 		err = mlxsw_sp_qdisc->ops->destroy(mlxsw_sp_port,
-						   mlxsw_sp_qdisc);
+						   mlxsw_sp_qdisc,
+						   qstats);
 
 	mlxsw_sp_qdisc->handle = TC_H_UNSPEC;
 	mlxsw_sp_qdisc->ops = NULL;
@@ -111,7 +115,8 @@ mlxsw_sp_qdisc_destroy(struct mlxsw_sp_port *mlxsw_sp_port,
 static int
 mlxsw_sp_qdisc_replace(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle,
 		       struct mlxsw_sp_qdisc *mlxsw_sp_qdisc,
-		       struct mlxsw_sp_qdisc_ops *ops, void *params)
+		       struct mlxsw_sp_qdisc_ops *ops, void *params,
+		       struct gnet_stats_queue *qstats)
 {
 	int err;
 
@@ -121,7 +126,7 @@ mlxsw_sp_qdisc_replace(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle,
 		 * Otherwise, we need to remove the old qdisc before setting the
 		 * new one.
 		 */
-		mlxsw_sp_qdisc_destroy(mlxsw_sp_port, mlxsw_sp_qdisc);
+		mlxsw_sp_qdisc_destroy(mlxsw_sp_port, mlxsw_sp_qdisc, qstats);
 	err = ops->check_params(mlxsw_sp_port, mlxsw_sp_qdisc, params);
 	if (err)
 		goto err_bad_param;
@@ -141,7 +146,7 @@ mlxsw_sp_qdisc_replace(struct mlxsw_sp_port *mlxsw_sp_port, u32 handle,
 
 err_bad_param:
 err_config:
-	mlxsw_sp_qdisc_destroy(mlxsw_sp_port, mlxsw_sp_qdisc);
+	mlxsw_sp_qdisc_destroy(mlxsw_sp_port, mlxsw_sp_qdisc, qstats);
 	return err;
 }
 
@@ -238,8 +243,10 @@ mlxsw_sp_setup_tc_qdisc_red_clean_stats(struct mlxsw_sp_port *mlxsw_sp_port,
 
 static int
 mlxsw_sp_qdisc_red_destroy(struct mlxsw_sp_port *mlxsw_sp_port,
-			   struct mlxsw_sp_qdisc *mlxsw_sp_qdisc)
+			   struct mlxsw_sp_qdisc *mlxsw_sp_qdisc,
+			   struct gnet_stats_queue *qstats)
 {
+	qstats->backlog -= mlxsw_sp_qdisc->stats_base.backlog;
 	return mlxsw_sp_tclass_congestion_disable(mlxsw_sp_port,
 						  mlxsw_sp_qdisc->tclass_num);
 }
@@ -330,6 +337,7 @@ mlxsw_sp_qdisc_get_red_stats(struct mlxsw_sp_port *mlxsw_sp_port,
 	struct mlxsw_sp_qdisc_stats *stats_base;
 	struct mlxsw_sp_port_xstats *xstats;
 	struct rtnl_link_stats64 *stats;
+	s64 backlog;
 
 	xstats = &mlxsw_sp_port->periodic_hw_stats.xstats;
 	stats = &mlxsw_sp_port->periodic_hw_stats.stats;
@@ -341,18 +349,20 @@ mlxsw_sp_qdisc_get_red_stats(struct mlxsw_sp_port *mlxsw_sp_port,
 		     stats_base->overlimits;
 	drops = xstats->wred_drop[tclass_num] + xstats->tail_drop[tclass_num] -
 		stats_base->drops;
+	backlog = mlxsw_sp_cells_bytes(mlxsw_sp_port->mlxsw_sp,
+				       xstats->backlog[tclass_num]) -
+		stats_base->backlog;
 
 	_bstats_update(stats_ptr->bstats, tx_bytes, tx_packets);
 	stats_ptr->qstats->overlimits += overlimits;
 	stats_ptr->qstats->drops += drops;
-	stats_ptr->qstats->backlog +=
-			mlxsw_sp_cells_bytes(mlxsw_sp_port->mlxsw_sp,
-					     xstats->backlog[tclass_num]);
+	stats_ptr->qstats->backlog += backlog;
 
 	stats_base->drops +=  drops;
 	stats_base->overlimits += overlimits;
 	stats_base->tx_bytes += tx_bytes;
 	stats_base->tx_packets += tx_packets;
+	stats_base->backlog += backlog;
 	return 0;
 }
 
@@ -382,7 +392,7 @@ int mlxsw_sp_setup_tc_red(struct mlxsw_sp_port *mlxsw_sp_port,
 		return mlxsw_sp_qdisc_replace(mlxsw_sp_port, p->handle,
 					      mlxsw_sp_qdisc,
 					      &mlxsw_sp_qdisc_ops_red,
-					      &p->set);
+					      &p->set, p->qstats);
 
 	if (!mlxsw_sp_qdisc_compare(mlxsw_sp_qdisc, p->handle,
 				    MLXSW_SP_QDISC_RED))
@@ -390,7 +400,8 @@ int mlxsw_sp_setup_tc_red(struct mlxsw_sp_port *mlxsw_sp_port,
 
 	switch (p->command) {
 	case TC_RED_DESTROY:
-		return mlxsw_sp_qdisc_destroy(mlxsw_sp_port, mlxsw_sp_qdisc);
+		return mlxsw_sp_qdisc_destroy(mlxsw_sp_port, mlxsw_sp_qdisc,
+					      p->qstats);
 	case TC_RED_XSTATS:
 		return mlxsw_sp_qdisc_get_xstats(mlxsw_sp_port, mlxsw_sp_qdisc,
 						 p->xstats);
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 0d1343cba84c..d64db7777d69 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -755,7 +755,10 @@ struct tc_red_qopt_offload {
 	u32 handle;
 	u32 parent;
 	union {
-		struct tc_red_qopt_offload_params set;
+		struct {
+			struct tc_red_qopt_offload_params set;
+			struct gnet_stats_queue *qstats;
+		};
 		struct tc_qopt_offload_stats stats;
 		struct red_stats *xstats;
 	};
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 0af1c1254e0b..8845654ef84a 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -167,8 +167,10 @@ static int red_offload(struct Qdisc *sch, bool enable)
 		opt.set.max = q->parms.qth_max >> q->parms.Wlog;
 		opt.set.probability = q->parms.max_P;
 		opt.set.is_ecn = red_use_ecn(q);
+		opt.qstats = &sch->qstats;
 	} else {
 		opt.command = TC_RED_DESTROY;
+		opt.qstats = &sch->qstats;
 	}
 
 	return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_RED, &opt);
@@ -322,7 +324,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

Re: [PATCH net-next 0/2] sched: improve behaviour of offloaded RED stats

[Jakub Kicinski]

On Wed, Jan 10, 2018 at 9:38 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> Hi!
>
> This set attempts to improve the kernel API for TC qdisc offloads.
> The cumulative stats are handled nicely today, but the momentary
> stats like backlog and qlen are behaving a little less cleanly.
>
> v1:
>  - reset the stats on destroy (incl. replace with unsupported params).

I would like to withdraw this series, I will realign to fit better
with Nogah and Yuval's work and repost.

Re: [PATCH net-next 0/2] sched: improve behaviour of offloaded RED stats

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Fri, 12 Jan 2018 00:40:14 -0800

> On Wed, Jan 10, 2018 at 9:38 PM, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
>> Hi!
>>
>> This set attempts to improve the kernel API for TC qdisc offloads.
>> The cumulative stats are handled nicely today, but the momentary
>> stats like backlog and qlen are behaving a little less cleanly.
>>
>> v1:
>>  - reset the stats on destroy (incl. replace with unsupported params).
> 
> I would like to withdraw this series, I will realign to fit better
> with Nogah and Yuval's work and repost.

Ok.