Re: [PATCH 4.19 012/106] ipmi_si: Only schedule continuously in the thread in maintenance mode
On Tue, Oct 08, 2019 at 11:49:15AM +0200, Pavel Machek wrote:
> Hi!
>
> > @@ -1013,11 +1016,20 @@ static int ipmi_thread(void *data)
> > spin_unlock_irqrestore(&(smi_info->si_lock), flags);
> > busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
> > _until);
> > - if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
> > + if (smi_result == SI_SM_CALL_WITHOUT_DELAY) {
> > ; /* do nothing */
> > - else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait)
> > - schedule();
> > - else if (smi_result == SI_SM_IDLE) {
> > + } else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait) {
> > + /*
> > +* In maintenance mode we run as fast as
> > +* possible to allow firmware updates to
> > +* complete as fast as possible, but normally
> > +* don't bang on the scheduler.
> > +*/
> > + if (smi_info->in_maintenance_mode)
> > + schedule();
> > + else
> > + usleep_range(100, 200);
> > + } else if (smi_result == SI_SM_IDLE) {
>
> This is quite crazy code. usleep() will need to do magic with high
> resolution timers to provide 200usec sleep... when all you want to do
> is unload the scheduler.
>
> cond_resched() should be okay to call in a loop, can the code use that
> instead?
According to Tejun Heo, spinning in a loop sleeping was causing all
sorts of issues with banging on scheduler locks on systems with lots of
cores. I forgot to add him to the CC on the patch, adding him now
for comment.
If cond_resched() would work, though, I'd be happy with that, it's
certainly simpler.
-corey
>
> Best regards,
> Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 4.19 012/106] ipmi_si: Only schedule continuously in the thread in maintenance mode
Hi!
> @@ -1013,11 +1016,20 @@ static int ipmi_thread(void *data)
> spin_unlock_irqrestore(&(smi_info->si_lock), flags);
> busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
> _until);
> - if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
> + if (smi_result == SI_SM_CALL_WITHOUT_DELAY) {
> ; /* do nothing */
> - else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait)
> - schedule();
> - else if (smi_result == SI_SM_IDLE) {
> + } else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait) {
> + /*
> + * In maintenance mode we run as fast as
> + * possible to allow firmware updates to
> + * complete as fast as possible, but normally
> + * don't bang on the scheduler.
> + */
> + if (smi_info->in_maintenance_mode)
> + schedule();
> + else
> + usleep_range(100, 200);
> + } else if (smi_result == SI_SM_IDLE) {
This is quite crazy code. usleep() will need to do magic with high
resolution timers to provide 200usec sleep... when all you want to do
is unload the scheduler.
cond_resched() should be okay to call in a loop, can the code use that
instead?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
[PATCH 4.19 012/106] ipmi_si: Only schedule continuously in the thread in maintenance mode
From: Corey Minyard
[ Upstream commit 340ff31ab00bca5c15915e70ad9ada3030c98cf8 ]
ipmi_thread() uses back-to-back schedule() to poll for command
completion which, on some machines, can push up CPU consumption and
heavily tax the scheduler locks leading to noticeable overall
performance degradation.
This was originally added so firmware updates through IPMI would
complete in a timely manner. But we can't kill the scheduler
locks for that one use case.
Instead, only run schedule() continuously in maintenance mode,
where firmware updates should run.
Signed-off-by: Corey Minyard
Signed-off-by: Sasha Levin
---
drivers/char/ipmi/ipmi_si_intf.c | 24 +++-
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 75e5006f395a5..006d765256782 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -221,6 +221,9 @@ struct smi_info {
*/
bool irq_enable_broken;
+ /* Is the driver in maintenance mode? */
+ bool in_maintenance_mode;
+
/*
* Did we get an attention that we did not handle?
*/
@@ -1013,11 +1016,20 @@ static int ipmi_thread(void *data)
spin_unlock_irqrestore(&(smi_info->si_lock), flags);
busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
_until);
- if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
+ if (smi_result == SI_SM_CALL_WITHOUT_DELAY) {
; /* do nothing */
- else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait)
- schedule();
- else if (smi_result == SI_SM_IDLE) {
+ } else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait) {
+ /*
+* In maintenance mode we run as fast as
+* possible to allow firmware updates to
+* complete as fast as possible, but normally
+* don't bang on the scheduler.
+*/
+ if (smi_info->in_maintenance_mode)
+ schedule();
+ else
+ usleep_range(100, 200);
+ } else if (smi_result == SI_SM_IDLE) {
if (atomic_read(_info->need_watch)) {
schedule_timeout_interruptible(100);
} else {
@@ -1025,8 +1037,9 @@ static int ipmi_thread(void *data)
__set_current_state(TASK_INTERRUPTIBLE);
schedule();
}
- } else
+ } else {
schedule_timeout_interruptible(1);
+ }
}
return 0;
}
@@ -1201,6 +1214,7 @@ static void set_maintenance_mode(void *send_info, bool
enable)
if (!enable)
atomic_set(_info->req_events, 0);
+ smi_info->in_maintenance_mode = enable;
}
static void shutdown_smi(void *send_info);
--
2.20.1
