On Tue, 1 Jun 2021 16:18:54 +0800 Yunsheng Lin wrote:
> > I see, thanks! That explains the need. Perhaps we can rephrase the
> > comment? Maybe:
> >
> > + /* Retest nolock_qdisc_is_empty() within the protection
> > + * of q->seqlock to protect from racing with requeuing.
> > + */
>
> Yes if we still decide to preserve the nolock_qdisc_is_empty() rechecking
> under q->seqlock.
Sounds good.
> >> --- a/net/sched/sch_generic.c
> >> +++ b/net/sched/sch_generic.c
> >> @@ -38,6 +38,15 @@ EXPORT_SYMBOL(default_qdisc_ops);
> >> static void qdisc_maybe_clear_missed(struct Qdisc *q,
> >> const struct netdev_queue *txq)
> >> {
> >> + set_bit(__QDISC_STATE_DRAINING, &q->state);
> >> +
> >> + /* Make sure DRAINING is set before clearing MISSED
> >> + * to make sure nolock_qdisc_is_empty() always return
> >> + * false for aoviding transmitting a packet directly
> >> + * bypassing the requeued packet.
> >> + */
> >> + smp_mb__after_atomic();
> >> +
> >> clear_bit(__QDISC_STATE_MISSED, &q->state);
> >>
> >> /* Make sure the below netif_xmit_frozen_or_stopped()
> >> @@ -52,8 +61,6 @@ static void qdisc_maybe_clear_missed(struct Qdisc *q,
> >> */
> >> if (!netif_xmit_frozen_or_stopped(txq))
> >> set_bit(__QDISC_STATE_MISSED, &q->state);
> >> - else
> >> - set_bit(__QDISC_STATE_DRAINING, &q->state);
> >> }
> >
> > But this would not be enough because we may also clear MISSING
> > in pfifo_fast_dequeue()?
>
> For the MISSING clearing in pfifo_fast_dequeue(), it seems it
> looks like the data race described in RFC v3 too?
>
> CPU1 CPU2 CPU3
> qdisc_run_begin(q) . .
> . MISSED is set .
> MISSED is cleared . .
> q->dequeue() . .
> . enqueue skb1 check MISSED # true
> qdisc_run_end(q) . .
> . . qdisc_run_begin(q) # true
> . MISSED is set send skb2 directly
Not sure what you mean.
