Re: [Openipmi-developer] [PATCH] IPMI: use schedule in kthread

Re: [Openipmi-developer] [PATCH] IPMI: use schedule in kthread

[Matt Domsch]

On Mon, Jun 26, 2006 at 09:08:19AM -0500, @ wrote:
> The kthread used to speed up polling for IPMI was using udelay
> when the lower-level state machine told it to do a short delay.
> This just used CPU and didn't help scheduling, thus causing bad
> problems with other tasks.  Call schedule() instead.
> 
> Signed-off-by: Corey Minyard <minyard@acm.org>

Acked-by: Matt Domsch <Matt_Domsch@dell.com>

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [PATCH] IPMI: use schedule in kthread

[Andrew Morton]

On Mon, 26 Jun 2006 09:08:19 -0500
MAILER-DAEMON@osdl.org wrote:

> The kthread used to speed up polling for IPMI was using udelay
> when the lower-level state machine told it to do a short delay.
> This just used CPU and didn't help scheduling, thus causing bad
> problems with other tasks.  Call schedule() instead.
> 
> Signed-off-by: Corey Minyard <minyard@acm.org>
> 
> Index: linux-2.6.17/drivers/char/ipmi/ipmi_si_intf.c
> ===================================================================
> --- linux-2.6.17.orig/drivers/char/ipmi/ipmi_si_intf.c
> +++ linux-2.6.17/drivers/char/ipmi/ipmi_si_intf.c
> @@ -809,7 +809,7 @@ static int ipmi_thread(void *data)
>  			/* do nothing */
>  		}
>  		else if (smi_result == SI_SM_CALL_WITH_DELAY)
> -			udelay(1);
> +			schedule();
>  		else
>  			schedule_timeout_interruptible(1);
>  	}

calling schedule() isn't a lot of use either.

If CONFIG_PREEMPT it's of no benefit and will just chew CPU.

If !CONFIG_PREEMPT && !need_resched() then it's a no-op and will chew CPU.

If !CONFIG_PREEMPT && need_resched() then yes, it'll schedule away.  This
is pretty much the only time that a simple schedule() is useful.



What are we actually trying to do in here?

Re: [PATCH] IPMI: use schedule in kthread

[Matt Domsch]

On Mon, Jun 26, 2006 at 12:00:48PM -0700, Andrew Morton wrote:
> On Mon, 26 Jun 2006 09:08:19 -0500
> MAILER-DAEMON@osdl.org wrote:
> 
> > The kthread used to speed up polling for IPMI was using udelay
> > when the lower-level state machine told it to do a short delay.
> > This just used CPU and didn't help scheduling, thus causing bad
> > problems with other tasks.  Call schedule() instead.
> > 
> > Signed-off-by: Corey Minyard <minyard@acm.org>
> > 
> > Index: linux-2.6.17/drivers/char/ipmi/ipmi_si_intf.c
> > ===================================================================
> > --- linux-2.6.17.orig/drivers/char/ipmi/ipmi_si_intf.c
> > +++ linux-2.6.17/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -809,7 +809,7 @@ static int ipmi_thread(void *data)
> >  			/* do nothing */
> >  		}
> >  		else if (smi_result == SI_SM_CALL_WITH_DELAY)
> > -			udelay(1);
> > +			schedule();
> >  		else
> >  			schedule_timeout_interruptible(1);
> >  	}
> 
> calling schedule() isn't a lot of use either.
> 
> If CONFIG_PREEMPT it's of no benefit and will just chew CPU.
> 
> If !CONFIG_PREEMPT && !need_resched() then it's a no-op and will chew CPU.
> 
> If !CONFIG_PREEMPT && need_resched() then yes, it'll schedule away.  This
> is pretty much the only time that a simple schedule() is useful.
> 
> 
> What are we actually trying to do in here?

Make up for a lack of poor hardware design.  Most IPMI controllers
that use the standard-specified KCS interface don't implement
interrupts to signal data availability, and its a character-at-a-time
interface.

Without this kernel thread, ordinary operations (ipmitool sensor list)
would take 30 seconds, essentially one character transfer per timer
tick.  Starting applications that would read a lot of IPMI data would
take 5 minutes.  Doing a firmware flash would take 15 minutes.

With the kernel thread, these ops drop to 5-7 seconds, <1 minute, and
~1.5 minutes respectively.  It does this by polling at a rate faster
than the timer tick, in a low-priority kernel thread, any time a
command is outstanding to the controller.  If no commands are
outstanding, it goes to sleep for a tick, then wakes up (in case the
controller itself generates an event so we can capture that).

The trouble is, with udelay(1), it could consume the CPU while waiting
for a command to complete on the controller, without letting other
tasks run.  This exhibited itself as jittery mouse movement as well as
soft lockup messages.

We need to be able to poll at faster-than-timer-tick rates, yet let
other higher-priority apps run.  Hence the schedule().  We're open to
other suggestions, but this seemed minimally intrusive while
accomplishing the goal.  No more soft lockups or jittery mice.

Thanks,
Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [PATCH] IPMI: use schedule in kthread

[Arjan van de Ven]

On Mon, 2006-06-26 at 14:49 -0500, Matt Domsch wrote:
> We need to be able to poll at faster-than-timer-tick rates, yet let
> other higher-priority apps run.  Hence the schedule().  We're open to
> other suggestions, but this seemed minimally intrusive while
> accomplishing the goal.  No more soft lockups or jittery mice.

at least put a whole bunch of cpu_relax() in that loop, or your cpu
will.. get hot. Would also be nice if cpufreq had a "run this one slow"
option.... 

Re: [PATCH] IPMI: use schedule in kthread

[Jeremy Fitzhardinge]

Arjan van de Ven wrote:
> at least put a whole bunch of cpu_relax() in that loop, or your cpu
> will.. get hot. Would also be nice if cpufreq had a "run this one slow"
> option.... 
The "conservative" and "ondemand" governors have an "ignore niced 
processes" setting, which looks like it would help here.

    J

Re: [PATCH] IPMI: use schedule in kthread

[Andrew Morton]

On Mon, 26 Jun 2006 14:49:37 -0500
Matt Domsch <Matt_Domsch@dell.com> wrote:

> On Mon, Jun 26, 2006 at 12:00:48PM -0700, Andrew Morton wrote:
> > On Mon, 26 Jun 2006 09:08:19 -0500
> > MAILER-DAEMON@osdl.org wrote:
> > 
> > > The kthread used to speed up polling for IPMI was using udelay
> > > when the lower-level state machine told it to do a short delay.
> > > This just used CPU and didn't help scheduling, thus causing bad
> > > problems with other tasks.  Call schedule() instead.
> > > 
> > > Signed-off-by: Corey Minyard <minyard@acm.org>
> > > 
> > > Index: linux-2.6.17/drivers/char/ipmi/ipmi_si_intf.c
> > > ===================================================================
> > > --- linux-2.6.17.orig/drivers/char/ipmi/ipmi_si_intf.c
> > > +++ linux-2.6.17/drivers/char/ipmi/ipmi_si_intf.c
> > > @@ -809,7 +809,7 @@ static int ipmi_thread(void *data)
> > >  			/* do nothing */
> > >  		}
> > >  		else if (smi_result == SI_SM_CALL_WITH_DELAY)
> > > -			udelay(1);
> > > +			schedule();
> > >  		else
> > >  			schedule_timeout_interruptible(1);
> > >  	}
> > 
> > calling schedule() isn't a lot of use either.
> > 
> > If CONFIG_PREEMPT it's of no benefit and will just chew CPU.
> > 
> > If !CONFIG_PREEMPT && !need_resched() then it's a no-op and will chew CPU.
> > 
> > If !CONFIG_PREEMPT && need_resched() then yes, it'll schedule away.  This
> > is pretty much the only time that a simple schedule() is useful.
> > 
> > 
> > What are we actually trying to do in here?
> 
> Make up for a lack of poor hardware design.  Most IPMI controllers
> that use the standard-specified KCS interface don't implement
> interrupts to signal data availability, and its a character-at-a-time
> interface.

ah, OK, you're screwed ;) There's no fix for that, so blowing CPU while
offering reschedule opportunities is better than just blowing CPU.

Re: [PATCH] IPMI: use schedule in kthread

[Corey Minyard]

Andrew Morton wrote:
> On Mon, 26 Jun 2006 09:08:19 -0500
> MAILER-DAEMON@osdl.org wrote:
>
>   
>> The kthread used to speed up polling for IPMI was using udelay
>> when the lower-level state machine told it to do a short delay.
>> This just used CPU and didn't help scheduling, thus causing bad
>> problems with other tasks.  Call schedule() instead.
>>
>> Signed-off-by: Corey Minyard <minyard@acm.org>
>>
>> Index: linux-2.6.17/drivers/char/ipmi/ipmi_si_intf.c
>> ===================================================================
>> --- linux-2.6.17.orig/drivers/char/ipmi/ipmi_si_intf.c
>> +++ linux-2.6.17/drivers/char/ipmi/ipmi_si_intf.c
>> @@ -809,7 +809,7 @@ static int ipmi_thread(void *data)
>>  			/* do nothing */
>>  		}
>>  		else if (smi_result == SI_SM_CALL_WITH_DELAY)
>> -			udelay(1);
>> +			schedule();
>>  		else
>>  			schedule_timeout_interruptible(1);
>>  	}
>>     
>
> calling schedule() isn't a lot of use either.
>
> If CONFIG_PREEMPT it's of no benefit and will just chew CPU.
>
> If !CONFIG_PREEMPT && !need_resched() then it's a no-op and will chew CPU.
>
> If !CONFIG_PREEMPT && need_resched() then yes, it'll schedule away.  This
> is pretty much the only time that a simple schedule() is useful.
>
>
>
> What are we actually trying to do in here?
>   
The IPMI physical interfaces in generally really suck.  The most common
are byte at a time interfaces without interrupts that generally take in
the 500 microsecond per byte range.

This thread is an attempt to improve the performance of these
interfaces.  It is very low priority and wakes up when the IPMI
interface is doing something.  It basically spins looking for IPMI
activity at nice level 19 to help improve the performance of the
interface.  So basically, it chews CPU, but should be preempted by
anything else that is scheduled to run.  However, just calling udelay(1)
caused scheduling problems; users were reporting soft lockups, jerky
mouse movement, and keyboard problems if the IPMI interface was very
busy.  Adding a schedule here seems to fix those problems, and I'm
assuming they are falling into your third scenario above.

Any suggestions on better ways to fix this?

Thanks,

-Corey