lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 3 Mar 2019 13:32:50 -0800 From: John Fastabend <john.fastabend@...il.com> To: Eric Dumazet <edumazet@...gle.com>, "David S . Miller" <davem@...emloft.net> Cc: netdev <netdev@...r.kernel.org>, Eric Dumazet <eric.dumazet@...il.com>, Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us> Subject: Re: [PATCH net] net: sched: put back q.qlen into a single location On 2/28/19 12:55 PM, Eric Dumazet wrote: > In the series fc8b81a5981f ("Merge branch 'lockless-qdisc-series'") > John made the assumption that the data path had no need to read > the qdisc qlen (number of packets in the qdisc). > > It is true when pfifo_fast is used as the root qdisc, or as direct MQ/MQPRIO > children. > > But pfifo_fast can be used as leaf in class full qdiscs, and existing > logic needs to access the child qlen in an efficient way. > > HTB breaks badly, since it uses cl->leaf.q->q.qlen in : > htb_activate() -> WARN_ON() > htb_dequeue_tree() to decide if a class can be htb_deactivated > when it has no more packets. > > HFSC, DRR, CBQ, QFQ have similar issues, and some calls to > qdisc_tree_reduce_backlog() also read q.qlen directly. > > Using qdisc_qlen_sum() (which iterates over all possible cpus) > in the data path is a non starter. > Yep. > It seems we have to put back qlen in a central location, > at least for stable kernels. > I can't think of anything better either unfortunately. > For all qdisc but pfifo_fast, qlen is guarded by the qdisc lock, > so the existing q.qlen{++|--} are correct. > > For 'lockless' qdisc (pfifo_fast so far), we need to use atomic_{inc|dec}() > because the spinlock might be not held (for example from > pfifo_fast_enqueue() and pfifo_fast_dequeue()) > > This patch adds atomic_qlen (in the same location than qlen) > and renames the following helpers, since we want to express > they can be used without qdisc lock, and that qlen is no longer percpu. > > - qdisc_qstats_cpu_qlen_dec -> qdisc_qstats_atomic_qlen_dec() > - qdisc_qstats_cpu_qlen_inc -> qdisc_qstats_atomic_qlen_inc() > > Later (net-next) we might revert this patch by tracking all these > qlen uses and replace them by a more efficient method (not having > to access a precise qlen, but an empty/non_empty status that might > be less expensive to maintain/track). Right I think this is the next step. > > Another possibility is to have a legacy pfifo_fast version that would > be used when used a a child qdisc, since the parent qdisc needs > a spinlock anyway. But then, future lockless qdiscs would also > have the same problem. > I would prefer to optimize the qlen checks out for each qdisc but haven't checked its possible yet in all cases. > Fixes: 7e66016f2c65 ("net: sched: helpers to sum qlen and qlen for per cpu logic") > Signed-off-by: Eric Dumazet <edumazet@...gle.com> > Cc: John Fastabend <john.fastabend@...il.com> > Cc: Jamal Hadi Salim <jhs@...atatu.com> > Cc: Cong Wang <xiyou.wangcong@...il.com> > Cc: Jiri Pirko <jiri@...nulli.us> > --- > include/net/sch_generic.h | 31 +++++++++++++------------------ > net/core/gen_stats.c | 2 -- > net/sched/sch_generic.c | 13 ++++++------- > 3 files changed, 19 insertions(+), 27 deletions(-) > Thanks! Acked-by: John Fastabend <john.fastabend@...il.com>
Powered by blists - more mailing lists