| Message ID | 20191016082833.u4jxbiqg3oo6lyue@linutronix.de |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2] net: sched: Avoid using yield() in a busy waiting loop | expand |
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.
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
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!
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.
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 */