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

[Jakub Kicinski]

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?