Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the dump
On Mon, 6 Mar 2017, David Miller wrote: > > Ah, right you are, thanks. The complete fix is not super trivial, as it > > needs some more surgery to tc_dump_qdisc_root(), tc_dump_tclass_root() and > > qdisc_match_from_root() (see 69012ae42 for some details). > > > > There are two options: > > > > - this gets fixed in two phases, in first everything *but* noop qdisc gets > > dumped (in the "give me everything" dump) and later we finalize it by > > teaching the above functions about noop_qdisc as well > > > > - I extend this patchset to handle noop qdisc from the very beginning; > > I am unlikely to find time for this during coming weeks though. But OTOH > > this whole thing is very low priority anyway > > > > What do you think? > > I'm not too hot on this whole idea because the only way you can emit > the noop_qdisc is to "dup" it by allocating a new qdisc so that you > can link it in. This has two downsides: > > 1) Extra overhead and memory usage > > 2) All of the simple checks against _qdisc might not be >so simply any more. Fully agreed. I'd be inclined to just live with the fact that noop qdisc would never ever be visible in the dump. I think that in this particular case this can be easily justified, as the behavior is consistent with what is seen in the dump ("nothing happens to the packets"). All other cases will be covered. If there are no objections to this, I'll send out v2 shortly. Thanks, -- Jiri Kosina SUSE Labs
Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the dump
From: Jiri KosinaDate: Mon, 6 Mar 2017 12:03:38 +0100 (CET) > Ah, right you are, thanks. The complete fix is not super trivial, as it > needs some more surgery to tc_dump_qdisc_root(), tc_dump_tclass_root() and > qdisc_match_from_root() (see 69012ae42 for some details). > > There are two options: > > - this gets fixed in two phases, in first everything *but* noop qdisc gets > dumped (in the "give me everything" dump) and later we finalize it by > teaching the above functions about noop_qdisc as well > > - I extend this patchset to handle noop qdisc from the very beginning; > I am unlikely to find time for this during coming weeks though. But OTOH > this whole thing is very low priority anyway > > What do you think? I'm not too hot on this whole idea because the only way you can emit the noop_qdisc is to "dup" it by allocating a new qdisc so that you can link it in. This has two downsides: 1) Extra overhead and memory usage 2) All of the simple checks against _qdisc might not be so simply any more.
Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the dump
On Wed, 1 Mar 2017, David Miller wrote: > > @@ -1066,6 +1066,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 > > parentid, > > _qdisc_ops, classid); > > if (cl->qdisc == NULL) > > cl->qdisc = _qdisc; > > + qdisc_hash_add(cl->qdisc, true); > > INIT_LIST_HEAD(>children); > > cl->vt_tree = RB_ROOT; > > cl->cf_tree = RB_ROOT; > > @@ -1425,6 +1426,7 @@ hfsc_init_qdisc(struct Qdisc *sch, struct nlattr *opt) > > sch->handle); > > if (q->root.qdisc == NULL) > > q->root.qdisc = _qdisc; > > + qdisc_hash_add(q->root.qdisc, true); > > INIT_LIST_HEAD(>root.children); > > q->root.vt_tree = RB_ROOT; > > q->root.cf_tree = RB_ROOT; > > I'm not so sure it is legal is potentially pass _qdisc into > qdisc_hash_add(). Ah, right you are, thanks. The complete fix is not super trivial, as it needs some more surgery to tc_dump_qdisc_root(), tc_dump_tclass_root() and qdisc_match_from_root() (see 69012ae42 for some details). There are two options: - this gets fixed in two phases, in first everything *but* noop qdisc gets dumped (in the "give me everything" dump) and later we finalize it by teaching the above functions about noop_qdisc as well - I extend this patchset to handle noop qdisc from the very beginning; I am unlikely to find time for this during coming weeks though. But OTOH this whole thing is very low priority anyway What do you think? Thanks, -- Jiri Kosina SUSE Labs
Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the dump
From: Jiri KosinaDate: Sat, 25 Feb 2017 22:29:09 +0100 (CET) > @@ -1066,6 +1066,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 > parentid, > _qdisc_ops, classid); > if (cl->qdisc == NULL) > cl->qdisc = _qdisc; > + qdisc_hash_add(cl->qdisc, true); > INIT_LIST_HEAD(>children); > cl->vt_tree = RB_ROOT; > cl->cf_tree = RB_ROOT; > @@ -1425,6 +1426,7 @@ hfsc_init_qdisc(struct Qdisc *sch, struct nlattr *opt) > sch->handle); > if (q->root.qdisc == NULL) > q->root.qdisc = _qdisc; > + qdisc_hash_add(q->root.qdisc, true); > INIT_LIST_HEAD(>root.children); > q->root.vt_tree = RB_ROOT; > q->root.cf_tree = RB_ROOT; I'm not so sure it is legal is potentially pass _qdisc into qdisc_hash_add().
[PATCH 1/2] net: sched: make default fifo qdiscs appear in the dump
From: Jiri KosinaThe original reason [1] for having hidden qdiscs (potential scalability issues in qdisc_match_from_root() with single linked list in case of large amount of qdiscs) has been invalidated by 59cc1f61f0 ("net: sched: convert qdisc linked list to hashtable"). This allows us for bringing more clarity and determinism into the dump by making default pfifo qdiscs visible. We're not turning this on by default though, at it was deemed [2] too intrusive / unnecessary change of default behavior towards userspace. Instead, TCA_DUMP_INVISIBLE netlink attribute is introduced, which allows applications to request complete qdisc hierarchy dump, including the ones that have always been implicit/invisible. [1] http://lkml.kernel.org/r/1460732328.10638.74.ca...@edumazet-glaptop3.roam.corp.google.com [2] http://lkml.kernel.org/r/20161021.105935.1907696543877061916.da...@davemloft.net Signed-off-by: Jiri Kosina --- include/net/pkt_sched.h| 2 +- include/net/sch_generic.h | 1 + include/uapi/linux/rtnetlink.h | 1 + net/sched/sch_api.c| 42 ++ net/sched/sch_cbq.c| 4 net/sched/sch_drr.c| 2 ++ net/sched/sch_dsmark.c | 1 + net/sched/sch_generic.c| 2 +- net/sched/sch_hfsc.c | 2 ++ net/sched/sch_htb.c| 1 + net/sched/sch_mq.c | 2 +- net/sched/sch_mqprio.c | 2 +- net/sched/sch_multiq.c | 1 + net/sched/sch_prio.c | 4 +++- net/sched/sch_qfq.c| 1 + net/sched/sch_red.c| 1 + net/sched/sch_sfb.c| 1 + net/sched/sch_tbf.c| 1 + 18 files changed, 54 insertions(+), 17 deletions(-) diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index cd334c9584e9..0625eac2c601 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -90,7 +90,7 @@ int unregister_qdisc(struct Qdisc_ops *qops); void qdisc_get_default(char *id, size_t len); int qdisc_set_default(const char *id); -void qdisc_hash_add(struct Qdisc *q); +void qdisc_hash_add(struct Qdisc *q, bool invisible); void qdisc_hash_del(struct Qdisc *q); struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle); struct Qdisc *qdisc_lookup_class(struct net_device *dev, u32 handle); diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index e6aa0a249672..e7dca250d115 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -66,6 +66,7 @@ struct Qdisc { #define TCQ_F_NOPARENT 0x40 /* root of its hierarchy : * qdisc_tree_decrease_qlen() should stop. */ +#define TCQ_F_INVISIBLE0x80 /* invisible by default in dump */ u32 limit; const struct Qdisc_ops *ops; struct qdisc_size_table __rcu *stab; diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index 262f0379d83a..c7de00e09797 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -542,6 +542,7 @@ enum { TCA_FCNT, TCA_STATS2, TCA_STAB, + TCA_DUMP_INVISIBLE, TCA_PAD, __TCA_MAX }; diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 206dc24add3a..8e4e6ab1847a 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -274,7 +274,7 @@ static struct Qdisc *qdisc_match_from_root(struct Qdisc *root, u32 handle) return NULL; } -void qdisc_hash_add(struct Qdisc *q) +void qdisc_hash_add(struct Qdisc *q, bool invisible) { if ((q->parent != TC_H_ROOT) && !(q->flags & TCQ_F_INGRESS)) { struct Qdisc *root = qdisc_dev(q)->qdisc; @@ -282,6 +282,8 @@ void qdisc_hash_add(struct Qdisc *q) WARN_ON_ONCE(root == _qdisc); ASSERT_RTNL(); hash_add_rcu(qdisc_dev(q)->qdisc_hash, >hash, q->handle); + if (invisible) + q->flags |= TCQ_F_INVISIBLE; } } EXPORT_SYMBOL(qdisc_hash_add); @@ -1004,7 +1006,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, goto err_out4; } - qdisc_hash_add(sch); + qdisc_hash_add(sch, false); return sch; } @@ -1400,9 +1402,14 @@ static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid, return -1; } -static bool tc_qdisc_dump_ignore(struct Qdisc *q) +static bool tc_qdisc_dump_ignore(struct Qdisc *q, bool dump_invisible) { - return (q->flags & TCQ_F_BUILTIN) ? true : false; + if (q->flags & TCQ_F_BUILTIN) + return true; + if ((q->flags & TCQ_F_INVISIBLE) && !dump_invisible) + return true; + + return false; } static int qdisc_notify(struct net *net, struct sk_buff *oskb, @@ -1416,12 +1423,12 @@ static int