Re: [PATCH net v2] net: sched: fix packet stuck problem for lockless qdisc

[Cong Wang]

On Tue, Mar 23, 2021 at 7:24 PM Yunsheng Lin <linyunsheng@xxxxxxxxxx> 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.