linux-kernel - Re: [PATCH] IPMI: Add parameter to limit CPU usage in kipmid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <4B2A79B7.9040301@acm.org>
Date:	Thu, 17 Dec 2009 12:34:31 -0600
From:	Corey Minyard <minyard@....org>
To:	Jean Delvare <jdelvare@...e.de>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Martin Wilck <martin.wilck@...fujitsu.com>,
	OpenIPMI Developers <openipmi-developer@...ts.sourceforge.net>
Subject: Re: [PATCH] IPMI: Add parameter to limit CPU usage in kipmid

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 <minyard@....org> wrote:
>>
>>     
>>> From: Martin Wilck <martin.wilck@...fujitsu.com>
>>>
>>> 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 <martin.wilck@...fujitsu.com>
>>> Cc: Jean Delvare <jdelvare@...e.de>
>>> Signed-off-by: Corey Minyard <cminyard@...sta.com>
>>>
>>> --- 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/