diff mbox series

[net-next,3/4] sch_htb: Stats for offloaded HTB

Message ID 20201209160251.19054-4-maximmi@mellanox.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series HTB offload | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 35 this patch: 35
netdev/kdoc success Errors and warnings before: 35 this patch: 35
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 35 this patch: 35
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Maxim Mikityanskiy Dec. 9, 2020, 4:02 p.m. UTC 
This commit adds support for statistics of offloaded HTB. Bytes and
packets counters for leaf and inner nodes are supported, the values are
taken from per-queue qdiscs, and the numbers that the user sees should
have the same behavior as the software (non-offloaded) HTB.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
 net/sched/sch_htb.c | 49 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

Dan Carpenter Dec. 10, 2020, 8:28 a.m. UTC | #1 
Hi Maxim,


url:    https://github.com/0day-ci/linux/commits/Maxim-Mikityanskiy/HTB-offload/20201210-000703
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git afae3cc2da100ead3cd6ef4bb1fb8bc9d4b817c5
config: i386-randconfig-m021-20201209 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
net/sched/sch_htb.c:1310 htb_dump_class_stats() error: we previously assumed 'cl->leaf.q' could be null (see line 1300)

vim +1310 net/sched/sch_htb.c

^1da177e4c3f415 Linus Torvalds        2005-04-16  1289  static int
87990467d387f92 Stephen Hemminger     2006-08-10  1290  htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
^1da177e4c3f415 Linus Torvalds        2005-04-16  1291  {
^1da177e4c3f415 Linus Torvalds        2005-04-16  1292  	struct htb_class *cl = (struct htb_class *)arg;
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1293  	struct htb_sched *q = qdisc_priv(sch);
338ed9b4de57c4b Eric Dumazet          2016-06-21  1294  	struct gnet_stats_queue qs = {
338ed9b4de57c4b Eric Dumazet          2016-06-21  1295  		.drops = cl->drops,
3c75f6ee139d464 Eric Dumazet          2017-09-18  1296  		.overlimits = cl->overlimits,
338ed9b4de57c4b Eric Dumazet          2016-06-21  1297  	};
6401585366326fc John Fastabend        2014-09-28  1298  	__u32 qlen = 0;
^1da177e4c3f415 Linus Torvalds        2005-04-16  1299  
5dd431b6b92c0db Paolo Abeni           2019-03-28 @1300  	if (!cl->level && cl->leaf.q)
                                                                                  ^^^^^^^^^^
Check for NULL

5dd431b6b92c0db Paolo Abeni           2019-03-28  1301  		qdisc_qstats_qlen_backlog(cl->leaf.q, &qlen, &qs.backlog);
5dd431b6b92c0db Paolo Abeni           2019-03-28  1302  
0564bf0afae443d Konstantin Khlebnikov 2016-07-16  1303  	cl->xstats.tokens = clamp_t(s64, PSCHED_NS2TICKS(cl->tokens),
0564bf0afae443d Konstantin Khlebnikov 2016-07-16  1304  				    INT_MIN, INT_MAX);
0564bf0afae443d Konstantin Khlebnikov 2016-07-16  1305  	cl->xstats.ctokens = clamp_t(s64, PSCHED_NS2TICKS(cl->ctokens),
0564bf0afae443d Konstantin Khlebnikov 2016-07-16  1306  				     INT_MIN, INT_MAX);
^1da177e4c3f415 Linus Torvalds        2005-04-16  1307  
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1308  	if (q->offload) {
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1309  		if (!cl->level) {
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09 @1310  			cl->bstats = cl->leaf.q->bstats;
                                                                                             ^^^^^^^^^^^^
Unchecked dereference

1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1311  			cl->bstats.bytes += cl->bstats_bias.bytes;
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1312  			cl->bstats.packets += cl->bstats_bias.packets;
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1313  		} else {
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1314  			htb_offload_aggregate_stats(q, cl);
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1315  		}
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1316  	}
1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1317  
edb09eb17ed89ea Eric Dumazet          2016-06-06  1318  	if (gnet_stats_copy_basic(qdisc_root_sleeping_running(sch),
edb09eb17ed89ea Eric Dumazet          2016-06-06  1319  				  d, NULL, &cl->bstats) < 0 ||
1c0d32fde5bdf11 Eric Dumazet          2016-12-04  1320  	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
338ed9b4de57c4b Eric Dumazet          2016-06-21  1321  	    gnet_stats_copy_queue(d, NULL, &qs, qlen) < 0)
^1da177e4c3f415 Linus Torvalds        2005-04-16  1322  		return -1;
^1da177e4c3f415 Linus Torvalds        2005-04-16  1323  
^1da177e4c3f415 Linus Torvalds        2005-04-16  1324  	return gnet_stats_copy_app(d, &cl->xstats, sizeof(cl->xstats));
^1da177e4c3f415 Linus Torvalds        2005-04-16  1325  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Maxim Mikityanskiy Dec. 10, 2020, 3:07 p.m. UTC | #2 
On 2020-12-10 10:28, Dan Carpenter wrote:
> Hi Maxim,
> 
> 
> url:    https://github.com/0day-ci/linux/commits/Maxim-Mikityanskiy/HTB-offload/20201210-000703
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git afae3cc2da100ead3cd6ef4bb1fb8bc9d4b817c5
> config: i386-randconfig-m021-20201209 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> net/sched/sch_htb.c:1310 htb_dump_class_stats() error: we previously assumed 'cl->leaf.q' could be null (see line 1300)
> 
> vim +1310 net/sched/sch_htb.c
> 
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1289  static int
> 87990467d387f92 Stephen Hemminger     2006-08-10  1290  htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1291  {
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1292  	struct htb_class *cl = (struct htb_class *)arg;
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1293  	struct htb_sched *q = qdisc_priv(sch);
> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1294  	struct gnet_stats_queue qs = {
> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1295  		.drops = cl->drops,
> 3c75f6ee139d464 Eric Dumazet          2017-09-18  1296  		.overlimits = cl->overlimits,
> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1297  	};
> 6401585366326fc John Fastabend        2014-09-28  1298  	__u32 qlen = 0;
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1299
> 5dd431b6b92c0db Paolo Abeni           2019-03-28 @1300  	if (!cl->level && cl->leaf.q)
>                                                                                    ^^^^^^^^^^
> Check for NULL

Well, I don't think this is real... I don't see any possibility how 
cl->leaf.q can be NULL for a leaf class. However, I'll add a similar 
check below anyway.

Also, I fixed the sparse warnings from the other email (sorry for them!)

I will wait for some time to collect more comments (hopefully from 
people, not only from static checkers) and respin with the fixes.

> 5dd431b6b92c0db Paolo Abeni           2019-03-28  1301  		qdisc_qstats_qlen_backlog(cl->leaf.q, &qlen, &qs.backlog);
> 5dd431b6b92c0db Paolo Abeni           2019-03-28  1302
> 0564bf0afae443d Konstantin Khlebnikov 2016-07-16  1303  	cl->xstats.tokens = clamp_t(s64, PSCHED_NS2TICKS(cl->tokens),
> 0564bf0afae443d Konstantin Khlebnikov 2016-07-16  1304  				    INT_MIN, INT_MAX);
> 0564bf0afae443d Konstantin Khlebnikov 2016-07-16  1305  	cl->xstats.ctokens = clamp_t(s64, PSCHED_NS2TICKS(cl->ctokens),
> 0564bf0afae443d Konstantin Khlebnikov 2016-07-16  1306  				     INT_MIN, INT_MAX);
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1307
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1308  	if (q->offload) {
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1309  		if (!cl->level) {
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09 @1310  			cl->bstats = cl->leaf.q->bstats;
>                                                                                               ^^^^^^^^^^^^
> Unchecked dereference
> 
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1311  			cl->bstats.bytes += cl->bstats_bias.bytes;
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1312  			cl->bstats.packets += cl->bstats_bias.packets;
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1313  		} else {
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1314  			htb_offload_aggregate_stats(q, cl);
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1315  		}
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1316  	}
> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1317
> edb09eb17ed89ea Eric Dumazet          2016-06-06  1318  	if (gnet_stats_copy_basic(qdisc_root_sleeping_running(sch),
> edb09eb17ed89ea Eric Dumazet          2016-06-06  1319  				  d, NULL, &cl->bstats) < 0 ||
> 1c0d32fde5bdf11 Eric Dumazet          2016-12-04  1320  	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1321  	    gnet_stats_copy_queue(d, NULL, &qs, qlen) < 0)
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1322  		return -1;
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1323
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1324  	return gnet_stats_copy_app(d, &cl->xstats, sizeof(cl->xstats));
> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1325  }
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
Dan Carpenter Dec. 11, 2020, 8:41 a.m. UTC | #3 
On Thu, Dec 10, 2020 at 05:07:28PM +0200, Maxim Mikityanskiy wrote:
> On 2020-12-10 10:28, Dan Carpenter wrote:
> > Hi Maxim,
> > 
> > 
> > url:    https://github.com/0day-ci/linux/commits/Maxim-Mikityanskiy/HTB-offload/20201210-000703
> > base:
> > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
> > afae3cc2da100ead3cd6ef4bb1fb8bc9d4b817c5
> > config: i386-randconfig-m021-20201209 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > smatch warnings:
> > net/sched/sch_htb.c:1310 htb_dump_class_stats() error: we previously assumed 'cl->leaf.q' could be null (see line 1300)
> > 
> > vim +1310 net/sched/sch_htb.c
> > 
> > ^1da177e4c3f415 Linus Torvalds        2005-04-16  1289  static int
> > 87990467d387f92 Stephen Hemminger     2006-08-10  1290  htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
> > ^1da177e4c3f415 Linus Torvalds        2005-04-16  1291  {
> > ^1da177e4c3f415 Linus Torvalds        2005-04-16  1292  	struct htb_class *cl = (struct htb_class *)arg;
> > 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1293  	struct htb_sched *q = qdisc_priv(sch);
> > 338ed9b4de57c4b Eric Dumazet          2016-06-21  1294  	struct gnet_stats_queue qs = {
> > 338ed9b4de57c4b Eric Dumazet          2016-06-21  1295  		.drops = cl->drops,
> > 3c75f6ee139d464 Eric Dumazet          2017-09-18  1296  		.overlimits = cl->overlimits,
> > 338ed9b4de57c4b Eric Dumazet          2016-06-21  1297  	};
> > 6401585366326fc John Fastabend        2014-09-28  1298  	__u32 qlen = 0;
> > ^1da177e4c3f415 Linus Torvalds        2005-04-16  1299
> > 5dd431b6b92c0db Paolo Abeni           2019-03-28 @1300  	if (!cl->level && cl->leaf.q)
> >                                                                                    ^^^^^^^^^^
> > Check for NULL
> 
> Well, I don't think this is real... I don't see any possibility how
> cl->leaf.q can be NULL for a leaf class. However, I'll add a similar check
> below anyway.
> 

Another option is to remove this check if it's really impossible.

regards,
dan carpenter
Maxim Mikityanskiy Dec. 11, 2020, 3:25 p.m. UTC | #4 
On 2020-12-11 10:41, Dan Carpenter wrote:
> On Thu, Dec 10, 2020 at 05:07:28PM +0200, Maxim Mikityanskiy wrote:
>> On 2020-12-10 10:28, Dan Carpenter wrote:
>>> Hi Maxim,
>>>
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Maxim-Mikityanskiy/HTB-offload/20201210-000703
>>> base:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
>>> afae3cc2da100ead3cd6ef4bb1fb8bc9d4b817c5
>>> config: i386-randconfig-m021-20201209 (attached as .config)
>>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>>>
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> smatch warnings:
>>> net/sched/sch_htb.c:1310 htb_dump_class_stats() error: we previously assumed 'cl->leaf.q' could be null (see line 1300)
>>>
>>> vim +1310 net/sched/sch_htb.c
>>>
>>> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1289  static int
>>> 87990467d387f92 Stephen Hemminger     2006-08-10  1290  htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
>>> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1291  {
>>> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1292  	struct htb_class *cl = (struct htb_class *)arg;
>>> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1293  	struct htb_sched *q = qdisc_priv(sch);
>>> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1294  	struct gnet_stats_queue qs = {
>>> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1295  		.drops = cl->drops,
>>> 3c75f6ee139d464 Eric Dumazet          2017-09-18  1296  		.overlimits = cl->overlimits,
>>> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1297  	};
>>> 6401585366326fc John Fastabend        2014-09-28  1298  	__u32 qlen = 0;
>>> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1299
>>> 5dd431b6b92c0db Paolo Abeni           2019-03-28 @1300  	if (!cl->level && cl->leaf.q)
>>>                                                                                     ^^^^^^^^^^
>>> Check for NULL
>>
>> Well, I don't think this is real... I don't see any possibility how
>> cl->leaf.q can be NULL for a leaf class. However, I'll add a similar check
>> below anyway.
>>
> 
> Another option is to remove this check if it's really impossible.

Yes, thanks, I see this option, but between these two options I'd pick 
the one that for sure doesn't make any change to the non-offloaded HTB 
logic. Even though to the best of my knowledge this check isn't needed, 
I might miss something, because I tried tracking down the origin of this 
code, and it was already there in the initial commit of 2005.

Respinning now with the CI issues fixed.

> regards,
> dan carpenter
>
diff mbox series

Patch

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 7aa56a5e1e94..c2e7efc2af88 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -114,6 +114,7 @@  struct htb_class {
 	 * Written often fields
 	 */
 	struct gnet_stats_basic_packed bstats;
+	struct gnet_stats_basic_packed bstats_bias;
 	struct tc_htb_xstats	xstats;	/* our special stats */
 
 	/* token bucket parameters */
@@ -1213,6 +1214,7 @@  static int htb_dump_class(struct Qdisc *sch, unsigned long arg,
 			  struct sk_buff *skb, struct tcmsg *tcm)
 {
 	struct htb_class *cl = (struct htb_class *)arg;
+	struct htb_sched *q = qdisc_priv(sch);
 	struct nlattr *nest;
 	struct tc_htb_opt opt;
 
@@ -1239,6 +1241,8 @@  static int htb_dump_class(struct Qdisc *sch, unsigned long arg,
 	opt.level = cl->level;
 	if (nla_put(skb, TCA_HTB_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
+	if (q->offload && nla_put_flag(skb, TCA_HTB_OFFLOAD))
+		goto nla_put_failure;
 	if ((cl->rate.rate_bytes_ps >= (1ULL << 32)) &&
 	    nla_put_u64_64bit(skb, TCA_HTB_RATE64, cl->rate.rate_bytes_ps,
 			      TCA_HTB_PAD))
@@ -1255,10 +1259,38 @@  static int htb_dump_class(struct Qdisc *sch, unsigned long arg,
 	return -1;
 }
 
+static void htb_offload_aggregate_stats(struct htb_sched *q, struct htb_class *cl)
+{
+	struct htb_class *c;
+	unsigned int i;
+
+	memset(&cl->bstats, 0, sizeof(cl->bstats));
+
+	for (i = 0; i < q->clhash.hashsize; i++) {
+		hlist_for_each_entry(c, &q->clhash.hash[i], common.hnode) {
+			struct htb_class *p = c;
+
+			while (p && p->level < cl->level)
+				p = p->parent;
+
+			if (p != cl)
+				continue;
+
+			cl->bstats.bytes += c->bstats_bias.bytes;
+			cl->bstats.packets += c->bstats_bias.packets;
+			if (c->level == 0) {
+				cl->bstats.bytes += c->leaf.q->bstats.bytes;
+				cl->bstats.packets += c->leaf.q->bstats.packets;
+			}
+		}
+	}
+}
+
 static int
 htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 {
 	struct htb_class *cl = (struct htb_class *)arg;
+	struct htb_sched *q = qdisc_priv(sch);
 	struct gnet_stats_queue qs = {
 		.drops = cl->drops,
 		.overlimits = cl->overlimits,
@@ -1273,6 +1305,16 @@  htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 	cl->xstats.ctokens = clamp_t(s64, PSCHED_NS2TICKS(cl->ctokens),
 				     INT_MIN, INT_MAX);
 
+	if (q->offload) {
+		if (!cl->level) {
+			cl->bstats = cl->leaf.q->bstats;
+			cl->bstats.bytes += cl->bstats_bias.bytes;
+			cl->bstats.packets += cl->bstats_bias.packets;
+		} else {
+			htb_offload_aggregate_stats(q, cl);
+		}
+	}
+
 	if (gnet_stats_copy_basic(qdisc_root_sleeping_running(sch),
 				  d, NULL, &cl->bstats) < 0 ||
 	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
@@ -1452,6 +1494,11 @@  static void htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
 		WARN_ON(old != q);
 	}
 
+	if (cl->parent) {
+		cl->parent->bstats_bias.bytes += q->bstats.bytes;
+		cl->parent->bstats_bias.packets += q->bstats.packets;
+	}
+
 	offload_opt = (struct tc_htb_qopt_offload) {
 		.command = last_child ? TC_HTB_LEAF_DEL_LAST : TC_HTB_LEAF_DEL,
 		.classid = cl->common.classid,
@@ -1766,6 +1813,8 @@  static int htb_change_class(struct Qdisc *sch, u32 classid,
 				htb_graft_helper(dev_queue, old_q);
 				goto err_kill_estimator;
 			}
+			parent->bstats_bias.bytes += old_q->bstats.bytes;
+			parent->bstats_bias.packets += old_q->bstats.packets;
 			qdisc_put(old_q);
 		}
 		new_q = qdisc_create_dflt(dev_queue, &pfifo_qdisc_ops,