[Linuxarm] Re: [PATCH net-next 2/3] net: sched: implement TCQ_F_CAN_BYPASS for lockless qdisc

On Mon, 31 May 2021 20:40:01 +0800 Yunsheng Lin wrote:

On 2021/5/31 9:10, Yunsheng Lin wrote:

...

On 2021/5/31 8:40, Yunsheng Lin wrote:

...

On 2021/5/31 4:21, Jakub Kicinski wrote:

[...] [...] [...] [...] [...] [...] [...]

...

...

When nolock_qdisc_is_empty() is not re-checking under q->seqlock, we may have:

    CPU1                                   CPU2

qdisc_run_begin(q) . . enqueue skb1 deuqueue skb1 and clear MISSED . . nolock_qdisc_is_empty() return true requeue skb . q->enqueue() . set MISSED . . . qdisc_run_end(q) . . qdisc_run_begin(q) . transmit skb2 directly . transmit the requeued skb1

The problem here is that skb1 and skb2 are from the same CPU, which means they are likely from the same flow, so we need to avoid this, right?

     CPU1                                   CPU2

qdisc_run_begin(q) . . enqueue skb1 dequeue skb1 . . . netdevice stopped and MISSED is clear . . nolock_qdisc_is_empty() return true requeue skb . . . . . . . qdisc_run_end(q) . . qdisc_run_begin(q) . transmit skb2 directly . transmit the requeued skb1

The above sequence diagram seems more correct, it is basically about how to avoid transmitting a packet directly bypassing the requeued packet.

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. + */

I had did some interesting testing to show how adjust a small number of code has some notiable performance degrade.

1. I used below patch to remove the nolock_qdisc_is_empty() testing under q->seqlock.

@@ -3763,17 +3763,6 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q, if (q->flags & TCQ_F_NOLOCK) { if (q->flags & TCQ_F_CAN_BYPASS && nolock_qdisc_is_empty(q) && qdisc_run_begin(q)) {

•                   /* Retest nolock_qdisc_is_empty() within the protection
  

•                    * of q->seqlock to ensure qdisc is indeed empty.
  

•                    */
  

•                   if (unlikely(!nolock_qdisc_is_empty(q))) {
  

•                           rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK;
  

•                           __qdisc_run(q);
  

•                           qdisc_run_end(q);
  

•                           goto no_lock_out;
  

•                   }
  

•                   qdisc_bstats_cpu_update(q, skb);
                    if (sch_direct_xmit(skb, q, dev, txq, NULL, true) &&
                        !nolock_qdisc_is_empty(q))
  

@@ -3786,7 +3775,6 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q, rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK; qdisc_run(q);

-no_lock_out: if (unlikely(to_free)) kfree_skb_list(to_free); return rc;

which has the below performance improvement:

threads v1 v1 + above patch delta 1 3.21Mpps 3.20Mpps -0.3% 2 5.56Mpps 5.94Mpps +4.9% 4 5.58Mpps 5.60Mpps +0.3% 8 2.76Mpps 2.77Mpps +0.3% 16 2.23Mpps 2.23Mpps +0.0%

v1 = this patchset.

2. After the above testing, it seems worthwhile to remove the nolock_qdisc_is_empty() testing under q->seqlock, so I used below patch to make sure nolock_qdisc_is_empty() always return false for netdev queue stopped case。

--- 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()?

which has the below performance data:

threads v1 v1 + above two patch delta 1 3.21Mpps 3.20Mpps -0.3% 2 5.56Mpps 5.94Mpps +4.9% 4 5.58Mpps 5.02Mpps -10% 8 2.76Mpps 2.77Mpps +0.3% 16 2.23Mpps 2.23Mpps +0.0%

So the adjustment in qdisc_maybe_clear_missed() seems to have caused about 10% performance degradation for 4 threads case.

And the cpu topdown perf data suggested that icache missed and bad Speculation play the main factor to those performance difference.

I tried to control the above factor by removing the inline function and add likely and unlikely tag for netif_xmit_frozen_or_stopped() in sch_generic.c.

And after removing the inline mark for function in sch_generic.c and add likely/unlikely tag for netif_xmit_frozen_or_stopped() checking in in sch_generic.c, we got notiable performance improvement for 1/2 threads case(some performance improvement for ip forwarding test too), but not for 4 threads case.

So it seems we need to ignore the performance degradation for 4 threads case? or any idea?

No ideas, are the threads pinned to CPUs in some particular way?