On Thu 19 Sep 2019 at 01:50, Cong Wang <xiyou.wangc...@gmail.com> wrote: > On Wed, Sep 18, 2019 at 12:32 AM Vlad Buslov <vla...@mellanox.com> wrote: >> >> TC filter API unlocking introduced several new fine-grained locks. The >> change caused sleeping-while-atomic BUGs in several Qdiscs that call cls >> APIs which need to obtain new mutex while holding sch tree spinlock. This >> series fixes affected Qdiscs by ensuring that cls API that became sleeping >> is only called outside of sch tree lock critical section. > > Sorry I just took a deeper look. It seems harder than just moving it > out of the critical section. > > qdisc_destroy() calls ops->reset() which usually purges queues, > I don't see how it is safe to move it out of tree spinlock without > respecting fast path. > > What do you think?
Hmm, maybe we can split qdisc destruction in two stage process for affected qdiscs? Rough sketch: 1. Call qdisc_reset() (or qdisc_purge_queue()) on qdisc that are being deleted under sch tree lock protection. 2. Call new qdisc_put_empty() function after releasing the lock. This function would implement same functionality as a regular qdisc_put() besides resetting the Qdisc and freeing skb in its queues (already done by qdisc_reset()) In fact, affected queues already do the same or something similar: - htb_change_class() calls qdisc_purge_queue() that calls qdisc_reset(), which makes reset inside qdisc_destroy() redundant. - multiq_tune() calls qdisc_tree_flush_backlog() that has the same implementation as qdisc_purge_queue() minus actually resetting the Qdisc. Can we substitute first function with the second one here? - sfb_change() - same as multiq_tune(). Do you think that would work? Also, I'm kind of surprised that it worked before. Even though syzbot complains about mutex that I added, cls API never honored expectation by such Qdiscs that tcf_block_put doesn't sleep. From the top of my head there are several places in the cls code where it has never been the case: chain head change for ingress/clsact Qdiscs calls mini_qdisc_pair_swap() which is a sleeping function, tcf_block_flush_all_chains() eventually calls driver callbacks for any offloaded filters which may sleep.
