netdev - Re: [PATCH net-next] net: get rid of spin_trylock() in net_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALx6S36MxNK9VUQA9Y=djD6bEwY_uiAkf674OEB=c_GUo8Pefw@mail.gmail.com>
Date:	Tue, 7 Jun 2016 09:04:02 -0700
From:	Tom Herbert <tom@...bertland.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: get rid of spin_trylock() in net_tx_action()

On Sat, Jun 4, 2016 at 8:02 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> From: Eric Dumazet <edumazet@...gle.com>
>
> Note: Tom Herbert posted almost same patch 3 months back, but for
> different reasons.
>
> The reasons we want to get rid of this spin_trylock() are :
>
> 1) Under high qdisc pressure, the spin_trylock() has almost no
> chance to succeed.
>
> 2) We loop multiple times in softirq handler, eventually reaching
> the max retry count (10), and we schedule ksoftirqd.
>
> Since we want to adhere more strictly to ksoftirqd being waked up in
> the future (https://lwn.net/Articles/687617/), better avoid spurious
> wakeups.
>
> 3) calls to __netif_reschedule() dirty the cache line containing
> q->next_sched, slowing down the owner of qdisc.
>
> 4) RT kernels can not use the spin_trylock() here.
>
> With help of busylock, we get the qdisc spinlock fast enough, and
> the trylock trick brings only performance penalty.
>
> Depending on qdisc setup, I observed a gain of up to 19 % in qdisc
> performance (1016600 pps instead of 853400 pps, using prio+tbf+fq_codel)
>
> ("mpstat -I SCPU 1" is much happier now)
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Tom Herbert <tom@...bertland.com>
> ---
>  net/core/dev.c |   26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 904ff431d570e3cfc0e6255480bd25f3c9dec2f3..896b686d19669d61a04bdccf6b0b71c0537cca81 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2253,7 +2253,7 @@ int netif_get_num_default_rss_queues(void)
>  }
>  EXPORT_SYMBOL(netif_get_num_default_rss_queues);
>
> -static inline void __netif_reschedule(struct Qdisc *q)
> +static void __netif_reschedule(struct Qdisc *q)
>  {
>         struct softnet_data *sd;
>         unsigned long flags;
> @@ -3898,22 +3898,14 @@ static void net_tx_action(struct softirq_action *h)
>                         head = head->next_sched;
>
>                         root_lock = qdisc_lock(q);
> -                       if (spin_trylock(root_lock)) {
> -                               smp_mb__before_atomic();
> -                               clear_bit(__QDISC_STATE_SCHED,
> -                                         &q->state);
> -                               qdisc_run(q);
> -                               spin_unlock(root_lock);
> -                       } else {
> -                               if (!test_bit(__QDISC_STATE_DEACTIVATED,
> -                                             &q->state)) {
> -                                       __netif_reschedule(q);
> -                               } else {
> -                                       smp_mb__before_atomic();
> -                                       clear_bit(__QDISC_STATE_SCHED,
> -                                                 &q->state);
> -                               }
> -                       }
> +                       spin_lock(root_lock);
> +                       /* We need to make sure head->next_sched is read
> +                        * before clearing __QDISC_STATE_SCHED
> +                        */
> +                       smp_mb__before_atomic();
> +                       clear_bit(__QDISC_STATE_SCHED, &q->state);
> +                       qdisc_run(q);
> +                       spin_unlock(root_lock);
>                 }
>         }
>  }
>
>
Acked-by: Tom Herbert <tom@...bertland.com>