[PATCH net-next] net: sched: Avoid using yield() in a busy waiting loop

[PATCH net-next] net: sched: Avoid using yield() in a busy waiting loop

[Sebastian Andrzej Siewior]

From: Marc Kleine-Budde <mkl@pengutronix.de>

With threaded interrupts enabled, the interrupt thread runs as SCHED_RR
with priority 50. If a user application with a higher priority preempts
the interrupt thread and tries to shutdown the network interface then it
will loop forever. The kernel will spin in the loop waiting for the
device to become idle and the scheduler will never consider the
interrupt thread because its priority is lower.

Avoid the problem by using by sleeping for a jiffy giving other tasks,
including the interrupt thread, a chance to run and make progress.

In the original thread it has been suggested to use wait_event() and
properly waiting for the state to occur. DaveM explained that this would
require to add expensive checks in the fast paths of packet processing.

Link: https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
[bigeasy: Rewrite commit message, add comment, use
          schedule_timeout_uninterruptible()]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

The old thread also pointed anoth yield() loop which was resolved by
commit
   845704a535e9b ("tcp: avoid looping in tcp_send_fin()")

 net/sched/sch_generic.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 17bd8f539bc7f..b27574f2c6b47 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1217,8 +1217,13 @@ void dev_deactivate_many(struct list_head *head)
 
 	/* Wait for outstanding qdisc_run calls. */
 	list_for_each_entry(dev, head, close_list) {
-		while (some_qdisc_is_busy(dev))
-			yield();
+		while (some_qdisc_is_busy(dev)) {
+			/* wait_event() would avoid this sleep-loop but would
+			 * require expesive checks in the fast paths of packet
+			 * processing which isn't worth it.
+			 */
+			schedule_timeout_uninterruptible(1);
+		}
 		/* The new qdisc is assigned at this point so we can safely
 		 * unwind stale skb lists and qdisc statistics
 		 */
-- 
2.23.0

Re: [PATCH net-next] net: sched: Avoid using yield() in a busy waiting loop

[Sergei Shtylyov]

Hello!

On 10/11/2019 08:15 PM, Sebastian Andrzej Siewior wrote:

> From: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> With threaded interrupts enabled, the interrupt thread runs as SCHED_RR
> with priority 50. If a user application with a higher priority preempts
> the interrupt thread and tries to shutdown the network interface then it
> will loop forever. The kernel will spin in the loop waiting for the
> device to become idle and the scheduler will never consider the
> interrupt thread because its priority is lower.
> 
> Avoid the problem by using by sleeping for a jiffy giving other tasks,

   So "using" or "sleeping"? :-)

> including the interrupt thread, a chance to run and make progress.
> 
> In the original thread it has been suggested to use wait_event() and
> properly waiting for the state to occur. DaveM explained that this would
> require to add expensive checks in the fast paths of packet processing.
> 
> Link: https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> [bigeasy: Rewrite commit message, add comment, use
>           schedule_timeout_uninterruptible()]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> 
> The old thread also pointed anoth yield() loop which was resolved by
> commit
>    845704a535e9b ("tcp: avoid looping in tcp_send_fin()")
> 
>  net/sched/sch_generic.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 17bd8f539bc7f..b27574f2c6b47 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1217,8 +1217,13 @@ void dev_deactivate_many(struct list_head *head)
>  
>  	/* Wait for outstanding qdisc_run calls. */
>  	list_for_each_entry(dev, head, close_list) {
> -		while (some_qdisc_is_busy(dev))
> -			yield();
> +		while (some_qdisc_is_busy(dev)) {
> +			/* wait_event() would avoid this sleep-loop but would
> +			 * require expesive checks in the fast paths of packet

   Expensive?

> +			 * processing which isn't worth it.
> +			 */
> +			schedule_timeout_uninterruptible(1);
> +		}
>  		/* The new qdisc is assigned at this point so we can safely
>  		 * unwind stale skb lists and qdisc statistics
>  		 */

MBR, Sergei

[PATCH net-next v2] net: sched: Avoid using yield() in a busy waiting loop

[Sebastian Andrzej Siewior]

From: Marc Kleine-Budde <mkl@pengutronix.de>

With threaded interrupts enabled, the interrupt thread runs as SCHED_RR
with priority 50. If a user application with a higher priority preempts
the interrupt thread and tries to shutdown the network interface then it
will loop forever. The kernel will spin in the loop waiting for the
device to become idle and the scheduler will never consider the
interrupt thread because its priority is lower.

Avoid the problem by sleeping for a jiffy giving other tasks,
including the interrupt thread, a chance to run and make progress.

In the original thread it has been suggested to use wait_event() and
properly waiting for the state to occur. DaveM explained that this would
require to add expensive checks in the fast paths of packet processing.

Link: https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
[bigeasy: Rewrite commit message, add comment, use
          schedule_timeout_uninterruptible()]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Typo fixes, noticed by Sergei Shtylyov.

 net/sched/sch_generic.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 17bd8f539bc7f..974731b86c20c 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1217,8 +1217,13 @@ void dev_deactivate_many(struct list_head *head)
 
 	/* Wait for outstanding qdisc_run calls. */
 	list_for_each_entry(dev, head, close_list) {
-		while (some_qdisc_is_busy(dev))
-			yield();
+		while (some_qdisc_is_busy(dev)) {
+			/* wait_event() would avoid this sleep-loop but would
+			 * require expensive checks in the fast paths of packet
+			 * processing which isn't worth it.
+			 */
+			schedule_timeout_uninterruptible(1);
+		}
 		/* The new qdisc is assigned at this point so we can safely
 		 * unwind stale skb lists and qdisc statistics
 		 */
-- 
2.23.0

Re: [PATCH net-next v2] net: sched: Avoid using yield() in a busy waiting loop

[Cong Wang]

On Wed, Oct 16, 2019 at 1:28 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> From: Marc Kleine-Budde <mkl@pengutronix.de>
>
> With threaded interrupts enabled, the interrupt thread runs as SCHED_RR
> with priority 50. If a user application with a higher priority preempts
> the interrupt thread and tries to shutdown the network interface then it
> will loop forever. The kernel will spin in the loop waiting for the
> device to become idle and the scheduler will never consider the
> interrupt thread because its priority is lower.
>
> Avoid the problem by sleeping for a jiffy giving other tasks,
> including the interrupt thread, a chance to run and make progress.
>
> In the original thread it has been suggested to use wait_event() and
> properly waiting for the state to occur. DaveM explained that this would
> require to add expensive checks in the fast paths of packet processing.
>
> Link: https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de

BTW, this link doesn't work, 404 is returned.


> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> [bigeasy: Rewrite commit message, add comment, use
>           schedule_timeout_uninterruptible()]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2: Typo fixes, noticed by Sergei Shtylyov.
>
>  net/sched/sch_generic.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 17bd8f539bc7f..974731b86c20c 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1217,8 +1217,13 @@ void dev_deactivate_many(struct list_head *head)
>
>         /* Wait for outstanding qdisc_run calls. */
>         list_for_each_entry(dev, head, close_list) {
> -               while (some_qdisc_is_busy(dev))
> -                       yield();
> +               while (some_qdisc_is_busy(dev)) {
> +                       /* wait_event() would avoid this sleep-loop but would
> +                        * require expensive checks in the fast paths of packet
> +                        * processing which isn't worth it.
> +                        */
> +                       schedule_timeout_uninterruptible(1);

I am curious why this is uninterruptible?

Thanks.

Re: [PATCH net-next v2] net: sched: Avoid using yield() in a busy waiting loop

[Sebastian Andrzej Siewior]

On 2019-10-16 10:28:04 [-0700], Cong Wang wrote:
> > Link: https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de
> 
> BTW, this link doesn't work, 404 is returned.

here it returns 200:

|$ wget https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de
|--2019-10-16 20:37:05--  https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de
|Resolving lkml.kernel.org (lkml.kernel.org)... 54.69.74.255, 54.71.250.162
|Connecting to lkml.kernel.org (lkml.kernel.org)|54.69.74.255|:443... connected.
|HTTP request sent, awaiting response... 302 Found
|Location: https://lore.kernel.org/linux-rt-users/1393976987-23555-1-git-send-email-mkl@pengutronix.de/ [following]
|--2019-10-16 20:37:06--  https://lore.kernel.org/linux-rt-users/1393976987-23555-1-git-send-email-mkl@pengutronix.de/
|Resolving lore.kernel.org (lore.kernel.org)... 54.71.250.162, 54.69.74.255
|Connecting to lore.kernel.org (lore.kernel.org)|54.71.250.162|:443... connected.
|HTTP request sent, awaiting response... 200 OK
|Length: 10044 (9,8K) [text/html]
|Saving to: ‘1393976987-23555-1-git-send-email-mkl@pengutronix.de’


> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -1217,8 +1217,13 @@ void dev_deactivate_many(struct list_head *head)
> >
> >         /* Wait for outstanding qdisc_run calls. */
> >         list_for_each_entry(dev, head, close_list) {
> > -               while (some_qdisc_is_busy(dev))
> > -                       yield();
> > +               while (some_qdisc_is_busy(dev)) {
> > +                       /* wait_event() would avoid this sleep-loop but would
> > +                        * require expensive checks in the fast paths of packet
> > +                        * processing which isn't worth it.
> > +                        */
> > +                       schedule_timeout_uninterruptible(1);
> 
> I am curious why this is uninterruptible?

You don't want a signal to wake it too early. It has to chill for a
jiffy.

> Thanks.

Sebastian

Re: [PATCH net-next v2] net: sched: Avoid using yield() in a busy waiting loop

[Cong Wang]

On Wed, Oct 16, 2019 at 11:48 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2019-10-16 10:28:04 [-0700], Cong Wang wrote:
> > > Link: https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de
> >
> > BTW, this link doesn't work, 404 is returned.
>
> here it returns 200:

Must be some firewall rule on my side.


>
> > > --- a/net/sched/sch_generic.c
> > > +++ b/net/sched/sch_generic.c
> > > @@ -1217,8 +1217,13 @@ void dev_deactivate_many(struct list_head *head)
> > >
> > >         /* Wait for outstanding qdisc_run calls. */
> > >         list_for_each_entry(dev, head, close_list) {
> > > -               while (some_qdisc_is_busy(dev))
> > > -                       yield();
> > > +               while (some_qdisc_is_busy(dev)) {
> > > +                       /* wait_event() would avoid this sleep-loop but would
> > > +                        * require expensive checks in the fast paths of packet
> > > +                        * processing which isn't worth it.
> > > +                        */
> > > +                       schedule_timeout_uninterruptible(1);
> >
> > I am curious why this is uninterruptible?
>
> You don't want a signal to wake it too early. It has to chill for a
> jiffy.

Yeah, at least msleep() is uninterruptible too.

So,
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

Thanks!

Re: [PATCH net-next v2] net: sched: Avoid using yield() in a busy waiting loop

[David Miller]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Wed, 16 Oct 2019 10:28:33 +0200

> From: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> With threaded interrupts enabled, the interrupt thread runs as SCHED_RR
> with priority 50. If a user application with a higher priority preempts
> the interrupt thread and tries to shutdown the network interface then it
> will loop forever. The kernel will spin in the loop waiting for the
> device to become idle and the scheduler will never consider the
> interrupt thread because its priority is lower.
> 
> Avoid the problem by sleeping for a jiffy giving other tasks,
> including the interrupt thread, a chance to run and make progress.
> 
> In the original thread it has been suggested to use wait_event() and
> properly waiting for the state to occur. DaveM explained that this would
> require to add expensive checks in the fast paths of packet processing.
> 
> Link: https://lkml.kernel.org/r/1393976987-23555-1-git-send-email-mkl@pengutronix.de
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> [bigeasy: Rewrite commit message, add comment, use
>           schedule_timeout_uninterruptible()]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2: Typo fixes, noticed by Sergei Shtylyov.

Applied, thank you.