On Tue, Mar 23, 2021 at 7:24 PM Yunsheng Lin <linyunsheng@huawei.com> wrote:
@@ -176,8 +207,23 @@ static inline bool qdisc_run_begin(struct Qdisc *qdisc) static inline void qdisc_run_end(struct Qdisc *qdisc) { write_seqcount_end(&qdisc->running); - if (qdisc->flags & TCQ_F_NOLOCK) + if (qdisc->flags & TCQ_F_NOLOCK) { spin_unlock(&qdisc->seqlock); + + /* qdisc_run_end() is protected by RCU lock, and + * qdisc reset will do a synchronize_net() after + * setting __QDISC_STATE_DEACTIVATED, so testing + * the below two bits separately should be fine.
Hmm, why synchronize_net() after setting this bit is fine? It could still be flipped right after you test RESCHEDULE bit.
+ * For qdisc_run() in net_tx_action() case, we + * really should provide rcu protection explicitly + * for document purposes or PREEMPT_RCU. + */ + if (unlikely(test_bit(__QDISC_STATE_NEED_RESCHEDULE, + &qdisc->state) && + !test_bit(__QDISC_STATE_DEACTIVATED, + &qdisc->state)))
Why do you want to test __QDISC_STATE_DEACTIVATED bit at all? dev_deactivate_many() will wait for those scheduled but being deactivated, so what's the problem of scheduling it even with this bit? Thanks.