From: Martin Wilck <[email protected]>
In some cases kipmid can use a lot of CPU. This adds a way to tune
the CPU used by kipmid to help in those cases. By setting
kipmid_max_busy_us to a value between 100 and 500, it is possible to
bring down kipmid CPU load to practically 0 without loosing too much
ipmi throughput performance. Not setting the value, or setting the
value to zero, operation is unaffected.
Signed-off-by: Martin Wilck <[email protected]>
Cc: Jean Delvare <[email protected]>
Signed-off-by: Corey Minyard <[email protected]>
--- linux-2.6.29.4/drivers/char/ipmi/ipmi_si_intf.c 2009-05-19 01:52:34.000000000 +0200
+++ linux-2.6.29-rc8/drivers/char/ipmi/ipmi_si_intf.c 2009-06-04 15:30:34.855398091 +0200
@@ -297,6 +297,9 @@
static int force_kipmid[SI_MAX_PARMS];
static int num_force_kipmid;
+static unsigned int kipmid_max_busy_us[SI_MAX_PARMS];
+static int num_max_busy_us;
+
static int unload_when_empty = 1;
static int try_smi_init(struct smi_info *smi);
@@ -927,23 +930,56 @@
}
}
+#define ipmi_si_set_not_busy(timespec) \
+ do { (timespec)->tv_nsec = -1; } while (0)
+#define ipmi_si_is_busy(timespec) ((timespec)->tv_nsec != -1)
+
+static int ipmi_thread_busy_wait(enum si_sm_result smi_result,
+ const struct smi_info *smi_info,
+ struct timespec *busy_until)
+{
+ unsigned int max_busy_us = 0;
+
+ if (smi_info->intf_num < num_max_busy_us)
+ max_busy_us = kipmid_max_busy_us[smi_info->intf_num];
+ if (max_busy_us == 0 || smi_result != SI_SM_CALL_WITH_DELAY)
+ ipmi_si_set_not_busy(busy_until);
+ else if (!ipmi_si_is_busy(busy_until)) {
+ getnstimeofday(busy_until);
+ timespec_add_ns(busy_until, max_busy_us*NSEC_PER_USEC);
+ } else {
+ struct timespec now;
+ getnstimeofday(&now);
+ if (unlikely(timespec_compare(&now, busy_until) > 0)) {
+ ipmi_si_set_not_busy(busy_until);
+ return 0;
+ }
+ }
+ return 1;
+}
+
static int ipmi_thread(void *data)
{
struct smi_info *smi_info = data;
unsigned long flags;
enum si_sm_result smi_result;
+ struct timespec busy_until;
+ ipmi_si_set_not_busy(&busy_until);
set_user_nice(current, 19);
while (!kthread_should_stop()) {
+ int busy_wait;
spin_lock_irqsave(&(smi_info->si_lock), flags);
smi_result = smi_event_handler(smi_info, 0);
spin_unlock_irqrestore(&(smi_info->si_lock), flags);
+ busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
+ &busy_until);
if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
; /* do nothing */
- else if (smi_result == SI_SM_CALL_WITH_DELAY)
+ else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait)
schedule();
else
- schedule_timeout_interruptible(1);
+ schedule_timeout_interruptible(0);
}
return 0;
}
@@ -1213,6 +1249,11 @@
MODULE_PARM_DESC(unload_when_empty, "Unload the module if no interfaces are"
" specified or found, default is 1. Setting to 0"
" is useful for hot add of devices using hotmod.");
+module_param_array(kipmid_max_busy_us, uint, &num_max_busy_us, 0644);
+MODULE_PARM_DESC(kipmid_max_busy_us,
+ "Max time (in microseconds) to busy-wait for IPMI data before"
+ " sleeping. 0 (default) means to wait forever. Set to 100-500"
+ " if kipmid is using up a lot of CPU time.");
static void std_irq_cleanup(struct smi_info *info)
On Wed, 16 Dec 2009 15:23:54 -0600
Corey Minyard <[email protected]> wrote:
> From: Martin Wilck <[email protected]>
>
> In some cases kipmid can use a lot of CPU.
Why is that? Without this information it is hard for others to suggest
alternative implementations.
> This adds a way to tune
> the CPU used by kipmid to help in those cases. By setting
> kipmid_max_busy_us to a value between 100 and 500, it is possible to
> bring down kipmid CPU load to practically 0 without loosing too much
> ipmi throughput performance. Not setting the value, or setting the
> value to zero, operation is unaffected.
Requiring the addition of a module parameter is regrettable. It'd be
better if the code "just works".
> Signed-off-by: Martin Wilck <[email protected]>
> Cc: Jean Delvare <[email protected]>
> Signed-off-by: Corey Minyard <[email protected]>
>
> --- linux-2.6.29.4/drivers/char/ipmi/ipmi_si_intf.c 2009-05-19 01:52:34.000000000 +0200
> +++ linux-2.6.29-rc8/drivers/char/ipmi/ipmi_si_intf.c 2009-06-04 15:30:34.855398091 +0200
> @@ -297,6 +297,9 @@
> static int force_kipmid[SI_MAX_PARMS];
> static int num_force_kipmid;
>
> +static unsigned int kipmid_max_busy_us[SI_MAX_PARMS];
> +static int num_max_busy_us;
> +
> static int unload_when_empty = 1;
>
> static int try_smi_init(struct smi_info *smi);
> @@ -927,23 +930,56 @@
> }
> }
>
> +#define ipmi_si_set_not_busy(timespec) \
> + do { (timespec)->tv_nsec = -1; } while (0)
> +#define ipmi_si_is_busy(timespec) ((timespec)->tv_nsec != -1)
These could have been implemented in C. It's better that way.
> +static int ipmi_thread_busy_wait(enum si_sm_result smi_result,
> + const struct smi_info *smi_info,
> + struct timespec *busy_until)
> +{
> + unsigned int max_busy_us = 0;
> +
> + if (smi_info->intf_num < num_max_busy_us)
> + max_busy_us = kipmid_max_busy_us[smi_info->intf_num];
> + if (max_busy_us == 0 || smi_result != SI_SM_CALL_WITH_DELAY)
> + ipmi_si_set_not_busy(busy_until);
> + else if (!ipmi_si_is_busy(busy_until)) {
> + getnstimeofday(busy_until);
> + timespec_add_ns(busy_until, max_busy_us*NSEC_PER_USEC);
> + } else {
> + struct timespec now;
> + getnstimeofday(&now);
> + if (unlikely(timespec_compare(&now, busy_until) > 0)) {
> + ipmi_si_set_not_busy(busy_until);
> + return 0;
> + }
> + }
> + return 1;
> +}
This function would benefit from some documentation.
It's a bit opaque. The setting of timespec.tv_nsec to -1 appears to
have some magical meaning, but it's left to the reader to work out what
that meaning is.
It might be clearer if the return type was `bool', ditto local variable
`busy_wait', below.
> static int ipmi_thread(void *data)
> {
> struct smi_info *smi_info = data;
> unsigned long flags;
> enum si_sm_result smi_result;
> + struct timespec busy_until;
>
> + ipmi_si_set_not_busy(&busy_until);
> set_user_nice(current, 19);
> while (!kthread_should_stop()) {
> + int busy_wait;
> spin_lock_irqsave(&(smi_info->si_lock), flags);
> smi_result = smi_event_handler(smi_info, 0);
> spin_unlock_irqrestore(&(smi_info->si_lock), flags);
> + busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
> + &busy_until);
> if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
> ; /* do nothing */
> - else if (smi_result == SI_SM_CALL_WITH_DELAY)
> + else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait)
> schedule();
> else
> - schedule_timeout_interruptible(1);
> + schedule_timeout_interruptible(0);
> }
> return 0;
> }
hm, what does schedule_timeout(0) do? It sets the timer to go off at
`jiffies' which I suppose means that the timer implementation will run
the callback at the next tick.
If there _is_ a tick. What does it do on NOHZ kernels?
The behaviour depends on HZ (it always did). Has it been tested to
check that performance is acceptable with HZ=100?
Again, it's too hard (IMO) to work out why the code sometimes calls
schedule() and sometimes calls schedule_timeout(0). It's a design
decision which is best communicated with a comment, please.
> @@ -1213,6 +1249,11 @@
> MODULE_PARM_DESC(unload_when_empty, "Unload the module if no interfaces are"
> " specified or found, default is 1. Setting to 0"
> " is useful for hot add of devices using hotmod.");
> +module_param_array(kipmid_max_busy_us, uint, &num_max_busy_us, 0644);
> +MODULE_PARM_DESC(kipmid_max_busy_us,
> + "Max time (in microseconds) to busy-wait for IPMI data before"
> + " sleeping. 0 (default) means to wait forever. Set to 100-500"
> + " if kipmid is using up a lot of CPU time.");
>
Andrew Morton wrote:
> On Wed, 16 Dec 2009 15:23:54 -0600
> Corey Minyard <[email protected]> wrote:
>
>> From: Martin Wilck <[email protected]>
>>
>> In some cases kipmid can use a lot of CPU.
>
> Why is that? Without this information it is hard for others to suggest
> alternative implementations.
[picking up the thread from the middle]
I dunno why, but I can confirm that kipmid is one of the most
noticeable threads on all machines here running ipmid.
Here's an example from a machine with 20days uptime.
According to `ps -aflx', [kipmid] process took 30:34
sec of CPU time. Most close to that is [md6_raid1]
thread with 12:30, and next to that is [kjournald]
with 0:24. 30:34 is not exactly huge given 20days
uptime, but it is a clear "winner", far from the
most close competitor.
Different kernels, somewhat different hardware, but
the same behavour.
/mjt
Hi Andrew,
Thanks for your comments.
Le mercredi 16 d?cembre 2009 22:42, Andrew Morton a ?crit?:
> On Wed, 16 Dec 2009 15:23:54 -0600
> Corey Minyard <[email protected]> wrote:
>
> > From: Martin Wilck <[email protected]>
> >
> > In some cases kipmid can use a lot of CPU.
>
> Why is that? Without this information it is hard for others to suggest
> alternative implementations.
Quoting Greg KH as he was investigating this issue:
"This looks to be a difference in the way the hardware works from
different ipmi controllers. Some allow for sleeping in an
interruptable state, and others do not, and can not have their delays
interrupted. Because of this, the process is put into uninterruptable
sleep mode, which causes a 'fake' system load increase on those types
of hardware controllers."
> > This adds a way to tune
> > the CPU used by kipmid to help in those cases. By setting
> > kipmid_max_busy_us to a value between 100 and 500, it is possible to
> > bring down kipmid CPU load to practically 0 without loosing too much
> > ipmi throughput performance. Not setting the value, or setting the
> > value to zero, operation is unaffected.
>
> Requiring the addition of a module parameter is regrettable. It'd be
> better if the code "just works".
That's right, it'd be better. But my understanding is that there is
no way to figure out automatically when the parameter is needed nor
its optimal value other than by trial and error. I'd love to be
proven wrong though.
> > Signed-off-by: Martin Wilck <[email protected]>
> > Cc: Jean Delvare <[email protected]>
> > Signed-off-by: Corey Minyard <[email protected]>
> >
> > --- linux-2.6.29.4/drivers/char/ipmi/ipmi_si_intf.c 2009-05-19 01:52:34.000000000 +0200
> > +++ linux-2.6.29-rc8/drivers/char/ipmi/ipmi_si_intf.c 2009-06-04 15:30:34.855398091 +0200
> > @@ -297,6 +297,9 @@
> > static int force_kipmid[SI_MAX_PARMS];
> > static int num_force_kipmid;
> >
> > +static unsigned int kipmid_max_busy_us[SI_MAX_PARMS];
> > +static int num_max_busy_us;
> > +
> > static int unload_when_empty = 1;
> >
> > static int try_smi_init(struct smi_info *smi);
> > @@ -927,23 +930,56 @@
> > }
> > }
> >
> > +#define ipmi_si_set_not_busy(timespec) \
> > + do { (timespec)->tv_nsec = -1; } while (0)
> > +#define ipmi_si_is_busy(timespec) ((timespec)->tv_nsec != -1)
>
> These could have been implemented in C. It's better that way.
+1, inline functions would be more readable.
I'll let Corey and maybe Martin comment on the rest, as the code is
not mine and I am not familiar with it.
--
Jean Delvare
Suse L3
Jean Delvare wrote:
> Hi Andrew,
>
> Thanks for your comments.
>
> Le mercredi 16 d?cembre 2009 22:42, Andrew Morton a ?crit :
>
>> On Wed, 16 Dec 2009 15:23:54 -0600
>> Corey Minyard <[email protected]> wrote:
>>
>>
>>> From: Martin Wilck <[email protected]>
>>>
>>> In some cases kipmid can use a lot of CPU.
>>>
>> Why is that? Without this information it is hard for others to suggest
>> alternative implementations.
>>
>
> Quoting Greg KH as he was investigating this issue:
>
> "This looks to be a difference in the way the hardware works from
> different ipmi controllers. Some allow for sleeping in an
> interruptable state, and others do not, and can not have their delays
> interrupted. Because of this, the process is put into uninterruptable
> sleep mode, which causes a 'fake' system load increase on those types
> of hardware controllers."
>
Yes, the hardware sucks. Delays vary greatly depending on what else the
ipmi controller is doing and varies greatly between different systems.
And almost none of them have interrupts.
>
>>> This adds a way to tune
>>> the CPU used by kipmid to help in those cases. By setting
>>> kipmid_max_busy_us to a value between 100 and 500, it is possible to
>>> bring down kipmid CPU load to practically 0 without loosing too much
>>> ipmi throughput performance. Not setting the value, or setting the
>>> value to zero, operation is unaffected.
>>>
>> Requiring the addition of a module parameter is regrettable. It'd be
>> better if the code "just works".
>>
>
> That's right, it'd be better. But my understanding is that there is
> no way to figure out automatically when the parameter is needed nor
> its optimal value other than by trial and error. I'd love to be
> proven wrong though.
>
It would be possible to do this automatically, I think, but it would
require dynamic tuning. Basically, the driver would have to watch how
much CPU it is using and the message latency and dynamically set the
value of kipmid_max_busy_us based upon what it sees. That would require
this patch, I think, then another piece of work to do the dynamic
setting. That would be somewhat complicated, but workable. But
something like this patch would still be required.
>
>>> Signed-off-by: Martin Wilck <[email protected]>
>>> Cc: Jean Delvare <[email protected]>
>>> Signed-off-by: Corey Minyard <[email protected]>
>>>
>>> --- linux-2.6.29.4/drivers/char/ipmi/ipmi_si_intf.c 2009-05-19 01:52:34.000000000 +0200
>>> +++ linux-2.6.29-rc8/drivers/char/ipmi/ipmi_si_intf.c 2009-06-04 15:30:34.855398091 +0200
>>> @@ -297,6 +297,9 @@
>>> static int force_kipmid[SI_MAX_PARMS];
>>> static int num_force_kipmid;
>>>
>>> +static unsigned int kipmid_max_busy_us[SI_MAX_PARMS];
>>> +static int num_max_busy_us;
>>> +
>>> static int unload_when_empty = 1;
>>>
>>> static int try_smi_init(struct smi_info *smi);
>>> @@ -927,23 +930,56 @@
>>> }
>>> }
>>>
>>> +#define ipmi_si_set_not_busy(timespec) \
>>> + do { (timespec)->tv_nsec = -1; } while (0)
>>> +#define ipmi_si_is_busy(timespec) ((timespec)->tv_nsec != -1)
>>>
>> These could have been implemented in C. It's better that way.
>>
>
> +1, inline functions would be more readable.
>
> I'll let Corey and maybe Martin comment on the rest, as the code is
> not mine and I am not familiar with it.
>
I agree, these should be inlines. I should have caught that. I can
convert them and address adding comments as Andrew suggests.
-corey
Le jeudi 17 d?cembre 2009 19:34, Corey Minyard a ?crit?:
> I agree, these should be inlines. I should have caught that. I can
> convert them and address adding comments as Andrew suggests.
Yes please!
Thanks,
--
Jean Delvare
Suse L3
On Thu, Dec 17, 2009 at 09:07:45PM +0100, Jean Delvare wrote:
> Le jeudi 17 d?cembre 2009 19:34, Corey Minyard a ?crit?:
> > I agree, these should be inlines. I should have caught that. I can
> > convert them and address adding comments as Andrew suggests.
Jean,
I cleaned up the patch some. I added some state results to the list of
things to be busy waited for (should improve performance a bit) and
changed the schedule_timeout_interruptible back to 1, since that's
what it's supposed to be. And I added some comments.
I did some testing on my system here. On my system, kipmid uses almost
no CPU normally. If I set the kipmid_max_busy_us value to 500, the
interface was more than 5 times slower. I had to set the value up to
35000 for it to go back to the normal performance, and it was pretty
linear between the two values. So this is definiately not for all
systems.
Can you try this out to make sure its ok?
-corey
From: Martin Wilck <[email protected]>
In some cases kipmid can use a lot of CPU. This is generally due to
the bad design of the hardware, it doesn't have interrupts and must be
polled constantly. Different controllers run at different speeds and
have different latencies, so it is difficult to account for
automatically.
This adds a way to tune the CPU used by kipmid to help in those cases.
By setting kipmid_max_busy_us to a value between 100 and 500, it is
possible to bring down kipmid CPU load to practically 0. This will
cost some performance, and that will vary from system to system. Not
setting the value, or setting the value to zero, causes operation to
be unaffected.
Signed-off-by: Martin Wilck <[email protected]>
Cc: Jean Delvare <[email protected]>
Reworked to clean things up, add comments, do other stylistic things, and
enhance performance a bit.
Signed-off-by: Corey Minyard <[email protected]>
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index e58ea4c..b914249 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -297,6 +297,9 @@ struct smi_info {
static int force_kipmid[SI_MAX_PARMS];
static int num_force_kipmid;
+static unsigned int kipmid_max_busy_us[SI_MAX_PARMS];
+static int num_max_busy_us;
+
static int unload_when_empty = 1;
static int try_smi_init(struct smi_info *smi);
@@ -927,12 +930,88 @@ static void set_run_to_completion(void *send_info, int i_run_to_completion)
}
}
+/*
+ * Handle busy waiting flags in the timespec. We use a -1 in tv_nsec
+ * to mark that we are not currently busy waiting.
+ */
+static inline void ipmi_si_set_not_busy(struct timespec *ts)
+{
+ ts->tv_nsec = -1;
+}
+static inline int ipmi_si_is_busy(struct timespec *ts)
+{
+ return ts->tv_nsec != -1;
+}
+
+static inline int ipmi_result_allow_busy_wait(enum si_sm_result smi_result)
+{
+ /*
+ * In these states we allow a busy wait. SI_SM_CALL_WITHOUT_DELAY
+ * is caught before here, so that will not be handled here. In the
+ * other results besides the ones below and SI_SM_CALL_WITHOUT_DELAY,
+ * do a full tick delay before checking again in kipmid.
+ */
+ switch (smi_result) {
+ case SI_SM_CALL_WITH_DELAY:
+ case SI_SM_TRANSACTION_COMPLETE:
+ case SI_SM_ATTN:
+ return 1;
+
+ default:
+ return 0;
+ }
+}
+
+/*
+ * Return true if the kthread should busy wait, and false if not. This is
+ * used to tune the operation of the kthread to not use too much CPU.
+ */
+static int ipmi_thread_busy_wait(enum si_sm_result smi_result,
+ const struct smi_info *smi_info,
+ struct timespec *busy_until)
+{
+ unsigned int max_busy_us = 0;
+
+ if (!ipmi_result_allow_busy_wait(smi_result))
+ return 0;
+
+ if (smi_info->intf_num < num_max_busy_us)
+ max_busy_us = kipmid_max_busy_us[smi_info->intf_num];
+
+ if (max_busy_us <= 0)
+ /* Busy wait timing is disabled, just busy wait forever. */
+ ipmi_si_set_not_busy(busy_until);
+ else if (!ipmi_si_is_busy(busy_until)) {
+ /*
+ * Need to start busy waiting. Record the time to stop busy
+ * waiting and do a full delay.
+ */
+ getnstimeofday(busy_until);
+ timespec_add_ns(busy_until, max_busy_us * NSEC_PER_USEC);
+ } else {
+ struct timespec now;
+
+ /*
+ * We are busy waiting. If we have exceeded our time then
+ * return false to do a full delay.
+ */
+ getnstimeofday(&now);
+ if (unlikely(timespec_compare(&now, busy_until) > 0)) {
+ ipmi_si_set_not_busy(busy_until);
+ return 0;
+ }
+ }
+ return 1;
+}
+
static int ipmi_thread(void *data)
{
struct smi_info *smi_info = data;
unsigned long flags;
enum si_sm_result smi_result;
+ struct timespec busy_until;
+ ipmi_si_set_not_busy(&busy_until);
set_user_nice(current, 19);
while (!kthread_should_stop()) {
spin_lock_irqsave(&(smi_info->si_lock), flags);
@@ -940,7 +1019,8 @@ static int ipmi_thread(void *data)
spin_unlock_irqrestore(&(smi_info->si_lock), flags);
if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
; /* do nothing */
- else if (smi_result == SI_SM_CALL_WITH_DELAY)
+ else if (ipmi_thread_busy_wait(smi_result, smi_info,
+ &busy_until))
schedule();
else
schedule_timeout_interruptible(1);
@@ -1213,6 +1293,13 @@ module_param(unload_when_empty, int, 0);
MODULE_PARM_DESC(unload_when_empty, "Unload the module if no interfaces are"
" specified or found, default is 1. Setting to 0"
" is useful for hot add of devices using hotmod.");
+module_param_array(kipmid_max_busy_us, int, &num_max_busy_us, 0644);
+MODULE_PARM_DESC(kipmid_max_busy_us,
+ "Max time (in microseconds) for kipmid to busy-wait for"
+ " IPMI data before sleeping. 0 (default) means to wait"
+ " forever. Set to a positive value, generally in the 100"
+ " to 500 range, if kipmid is using up a lot of CPU time."
+ " This will reduce performace, so balance is required.");
static void std_irq_cleanup(struct smi_info *info)
Le jeudi 17 d?cembre 2009 23:08, Corey Minyard a ?crit?:
> I cleaned up the patch some. I added some state results to the list of
> things to be busy waited for (should improve performance a bit) and
> changed the schedule_timeout_interruptible back to 1, since that's
> what it's supposed to be. And I added some comments.
Thanks for doing this!
> I did some testing on my system here. On my system, kipmid uses almost
> no CPU normally. If I set the kipmid_max_busy_us value to 500, the
> interface was more than 5 times slower. I had to set the value up to
> 35000 for it to go back to the normal performance, and it was pretty
> linear between the two values. So this is definiately not for all
> systems.
>
> Can you try this out to make sure its ok?
I don't have any hardware where I can test this myself. But hopefully
Martin does?
Thanks,
--
Jean Delvare
Suse L3
Le samedi 19 d?cembre 2009 14:56, Jean Delvare a ?crit?:
> Le jeudi 17 d?cembre 2009 23:08, Corey Minyard a ?crit?:
> > I cleaned up the patch some. I added some state results to the list of
> > things to be busy waited for (should improve performance a bit) and
> > changed the schedule_timeout_interruptible back to 1, since that's
> > what it's supposed to be. And I added some comments.
>
> Thanks for doing this!
>
> > I did some testing on my system here. On my system, kipmid uses almost
> > no CPU normally. If I set the kipmid_max_busy_us value to 500, the
> > interface was more than 5 times slower. I had to set the value up to
> > 35000 for it to go back to the normal performance, and it was pretty
> > linear between the two values. So this is definiately not for all
> > systems.
> >
> > Can you try this out to make sure its ok?
>
> I don't have any hardware where I can test this myself. But hopefully
> Martin does?
I've had the latest version of this patch tested by one of my
colleagues who has access to the relevant hardware, results are OK.
So I think we're ready to go, can we finally have this patch pushed
upstream?
Thanks,
--
Jean Delvare
Suse L3