On 2021/6/2 4:48, Jakub Kicinski wrote:
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.
CPU1 CPU2 CPU3 qdisc_run_begin(q) . . . MISSED is set . MISSED is cleared . . another dequeuing . . . . . . enqueue skb1 nolock_qdisc_is_empty() # true qdisc_run_end(q) . . . . qdisc_run_begin(q) # true . . send skb2 directly . MISSED is set . As qdisc is indeed empty at the point when MISSED is clear and another dequeue is retried by CPU1, MISSED setting is not under q->seqlock, so it seems retesting MISSED under q->seqlock does not seem to make any difference? and it seems like the case that does not need handling as we agreed previously?
.