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?
>
> .
>
