On Thu, Sep 19, 2019 at 1:53 AM Vlad Buslov <vla...@mellanox.com> wrote: > > > 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?
I think they have to call qdisc_purge_queue() or whatever that calls qdisc_reset() to reset all queues including qdisc->gso_skb and qdisc->skb_bad_txq before releasing sch tree lock. qdisc_tree_flush_backlog() is not sufficient.
