Re: [PATCH net v8 1/3] net: sched: fix packet stuck problem for lockless qdisc

[Cong Wang]

On Fri, May 14, 2021 at 4:39 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Fri, 14 May 2021 16:36:16 -0700 Cong Wang wrote:
> > > @@ -176,8 +202,15 @@ 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);
> > > +
> > > +               if (unlikely(test_bit(__QDISC_STATE_MISSED,
> > > +                                     &qdisc->state))) {
> > > +                       clear_bit(__QDISC_STATE_MISSED, &qdisc->state);
> >
> > We have test_and_clear_bit() which is atomic, test_bit()+clear_bit()
> > is not.
>
> It doesn't have to be atomic, right? I asked to split the test because
> test_and_clear is a locked op on x86, test by itself is not.

It depends on whether you expect the code under the true condition
to run once or multiple times, something like:

if (test_bit()) {
  clear_bit();
  // this code may run multiple times
}

With the atomic test_and_clear_bit(), it only runs once:

if (test_and_clear_bit()) {
  // this code runs once
}

This is why __netif_schedule() uses test_and_set_bit() instead of
test_bit()+set_bit().

Thanks.