* [PATCH net-next] net: get rid of spin_trylock() in net_tx_action()
@ 2016-06-05 3:02 Eric Dumazet
2016-06-07 15:40 ` Tom Herbert
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Eric Dumazet @ 2016-06-05 3:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Tom Herbert
From: Eric Dumazet <edumazet@google.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@google.com>
Cc: Tom Herbert <tom@herbertland.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);
}
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: get rid of spin_trylock() in net_tx_action()
2016-06-05 3:02 [PATCH net-next] net: get rid of spin_trylock() in net_tx_action() Eric Dumazet
@ 2016-06-07 15:40 ` Tom Herbert
2016-06-07 15:59 ` Eric Dumazet
2016-06-07 16:04 ` Tom Herbert
2016-06-07 22:38 ` David Miller
2 siblings, 1 reply; 5+ messages in thread
From: Tom Herbert @ 2016-06-07 15:40 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Sat, Jun 4, 2016 at 8:02 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.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@google.com>
> Cc: Tom Herbert <tom@herbertland.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)) {
This check no longer needed?
> - __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);
> }
> }
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: get rid of spin_trylock() in net_tx_action()
2016-06-07 15:40 ` Tom Herbert
@ 2016-06-07 15:59 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2016-06-07 15:59 UTC (permalink / raw)
To: Tom Herbert; +Cc: David Miller, netdev
On Tue, 2016-06-07 at 08:40 -0700, Tom Herbert wrote:
> On Sat, Jun 4, 2016 at 8:02 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.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@google.com>
> > Cc: Tom Herbert <tom@herbertland.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)) {
>
> This check no longer needed?
It is not needed, since we always take the spinlock.
This section is really dead code now.
>
> > - __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);
> > }
> > }
> > }
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: get rid of spin_trylock() in net_tx_action()
2016-06-05 3:02 [PATCH net-next] net: get rid of spin_trylock() in net_tx_action() Eric Dumazet
2016-06-07 15:40 ` Tom Herbert
@ 2016-06-07 16:04 ` Tom Herbert
2016-06-07 22:38 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: Tom Herbert @ 2016-06-07 16:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Sat, Jun 4, 2016 at 8:02 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.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@google.com>
> Cc: Tom Herbert <tom@herbertland.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@herbertland.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: get rid of spin_trylock() in net_tx_action()
2016-06-05 3:02 [PATCH net-next] net: get rid of spin_trylock() in net_tx_action() Eric Dumazet
2016-06-07 15:40 ` Tom Herbert
2016-06-07 16:04 ` Tom Herbert
@ 2016-06-07 22:38 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2016-06-07 22:38 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, tom
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 04 Jun 2016 20:02:28 -0700
> From: Eric Dumazet <edumazet@google.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@google.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-07 22:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-05 3:02 [PATCH net-next] net: get rid of spin_trylock() in net_tx_action() Eric Dumazet
2016-06-07 15:40 ` Tom Herbert
2016-06-07 15:59 ` Eric Dumazet
2016-06-07 16:04 ` Tom Herbert
2016-06-07 22:38 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).