IPMI driver version 19 release
This is a new release of the IPMI driver that fixes some performance problems. Some vendors implement firmware updates over IPMI, and this speeds up that process quite a bit.
Linus, please apply.
Alan, I will send you the 2.4 patch, so you don't have to fetch it from SF.
This release:
* Improves the "send - wait for response - send -wait for
response - etc" performance when using high-res timers. Before, an ~10ms delay would be added to each message, because it didn't restart the timer if nothing was happing when a new message was started.
* Adds some checking for leaked messages.
The patch is a 2.5 version against what is current in Bitkeeper (well, what is current in bk-CVS, thanks for the tool, Larry). The 2.5.66 and 2.4.20 versions are at:
http://sourceforge.net/projects/openipmi/
-Corey
Only in linux-2.5/drivers/char/ipmi: CVS
home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_kcs_intf.c renamed from drivers/char/ipmi/ipmi_kcs_intf.c +57 -12
@@ -61,6 +61,14 @@/* Measure times between events in the driver. */ #undef DEBUG_TIMING +/* Timing parameters. Call every 10 ms when not doing anything, +otherwise call every KCS_SHORT_TIMEOUT_USEC microseconds. */ +#define KCS_TIMEOUT_TIME_USEC 10000 +#define KCS_USEC_PER_JIFFY (1000000/HZ) +#define KCS_TIMEOUT_JIFFIES (KCS_TIMEOUT_TIME_USEC/KCS_USEC_PER_JIFFY) +#define KCS_SHORT_TIMEOUT_USEC 250/* .25ms when the SM request a +short timeout */ +#ifdef CONFIG_IPMI_KCS /* This forces a dependency to the config file for this option. */ #endif
@@ -132,6 +140,8 @@int interrupt_disabled; }; +static void kcs_restart_short_timer (struct kcs_info *kcs_info); +static void deliver_recv_msg (struct kcs_info *kcs_info,struct ipmi_smi_msg *msg) {/* Deliver the message to the upper layer with the lock
@@ -309,6 +319,9 @@#endif switch (kcs_info->kcs_state) {case KCS_NORMAL : +if (!kcs_info->curr_msg) +break ; + kcs_info->curr_msg->rsp_size = kcs_get_result(kcs_info->kcs_sm, kcs_info->curr_msg->rsp,
@@ -563,8 +576,9 @@spin_lock_irqsave (&(kcs_info->kcs_lock), flags); result =kcs_event_handler (kcs_info,0 );while (result !=KCS_SM_IDLE ) { -udelay (500 ); - result =kcs_event_handler (kcs_info,500 ); +udelay (KCS_SHORT_TIMEOUT_USEC ); + result =kcs_event_handler (kcs_info, +KCS_SHORT_TIMEOUT_USEC ); }spin_unlock_irqrestore (&(kcs_info->kcs_lock), flags);return ;
@@ -582,6 +596,7 @@ && (kcs_info->curr_msg == NULL)) {start_next_msg (kcs_info); +kcs_restart_short_timer (kcs_info); }spin_unlock_irqrestore (&(kcs_info->kcs_lock), flags); }
@@ -598,8 +613,9 @@if (i_run_to_completion) { result =kcs_event_handler (kcs_info,0 );while (result !=KCS_SM_IDLE ) { -udelay (500 ); - result =kcs_event_handler (kcs_info,500 ); +udelay (KCS_SHORT_TIMEOUT_USEC ); + result =kcs_event_handler (kcs_info, +KCS_SHORT_TIMEOUT_USEC ); } }
@@ -613,14 +629,42 @@atomic_set (&kcs_info->req_events,1 ); } -/* Call every 10 ms. */ -#define KCS_TIMEOUT_TIME_USEC 10000 -#define KCS_USEC_PER_JIFFY (1000000/HZ) -#define KCS_TIMEOUT_JIFFIES (KCS_TIMEOUT_TIME_USEC/KCS_USEC_PER_JIFFY) -#define KCS_SHORT_TIMEOUT_USEC 500/* .5ms when the SM request a -short timeout */ static int initialized =0 ; +/* Must be called with interrupts off and with the kcs_lock held. */ +static void kcs_restart_short_timer (struct kcs_info *kcs_info) +{ +if (del_timer (&(kcs_info->kcs_timer))) { +#ifdef CONFIG_HIGH_RES_TIMERS +unsigned long jiffies_now; + +/* If we don't delete the timer, then it will go off +immediately, anyway. So we only process if we +actually delete the timer. */ + +/* We already have irqsave on, so no need for it +here. */ +read_lock (&xtime_lock); + jiffies_now = jiffies; + kcs_info->kcs_timer.expires = jiffies_now; + + kcs_info->kcs_timer.sub_expires + =quick_update_jiffies_sub (jiffies_now); +read_unlock (&xtime_lock); + + kcs_info->kcs_timer.sub_expires + +=usec_to_arch_cycles (KCS_SHORT_TIMEOUT_USEC ); +while (kcs_info->kcs_timer.sub_expires >= cycles_per_jiffies) { + kcs_info->kcs_timer.expires++; + kcs_info->kcs_timer.sub_expires -= cycles_per_jiffies; + } +#else + kcs_info->kcs_timer.expires = jiffies +1 ; +#endif +add_timer (&(kcs_info->kcs_timer)); + } +} +static void kcs_timeout (unsigned long data) {struct kcs_info *kcs_info = (struct kcs_info *) data;
@@ -643,12 +687,11 @@printk ("**Timer: %d.%9.9d\n" , t.tv_sec, t.tv_usec);#endif jiffies_now = jiffies; + time_diff = ((jiffies_now - kcs_info->last_timeout_jiffies) *KCS_USEC_PER_JIFFY ); kcs_result =kcs_event_handler (kcs_info, time_diff); -spin_unlock_irqrestore (&(kcs_info->kcs_lock), flags); - kcs_info->last_timeout_jiffies = jiffies_now;if ((kcs_info->irq) && (! kcs_info->interrupt_disabled)) {
@@ -669,6 +712,7 @@ } }else { kcs_info->kcs_timer.expires = jiffies +KCS_TIMEOUT_JIFFIES ; + kcs_info->kcs_timer.sub_expires =0 ; }#else /* If requested, take the shortest delay possible */
@@ -681,6 +725,7 @@ do_add_timer:add_timer (&(kcs_info->kcs_timer)); +spin_unlock_irqrestore (&(kcs_info->kcs_lock), flags); }static void kcs_irq_handler (int irq,void *data,struct pt_regs *regs)
home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_msghandler.c renamed from drivers/char/ipmi/ipmi_msghandler.c +23 -2
@@ -1765,9 +1765,13 @@ } +static atomic_t smi_msg_inuse_count =ATOMIC_INIT (0 ); +static atomic_t recv_msg_inuse_count =ATOMIC_INIT (0 ); +/* FIXME - convert these to slabs. */ static void free_smi_msg (struct ipmi_smi_msg *msg) { +atomic_dec (&smi_msg_inuse_count);kfree (msg); }
@@ -1775,13 +1779,16 @@ {struct ipmi_smi_msg *rv; rv =kmalloc (sizeof (struct ipmi_smi_msg ),GFP_ATOMIC ); -if (rv) +if (rv) { rv->done = free_smi_msg; +atomic_inc (&smi_msg_inuse_count); + }return rv; }static void free_recv_msg (struct ipmi_recv_msg *msg) { +atomic_dec (&recv_msg_inuse_count);kfree (msg); }
@@ -1790,8 +1797,10 @@struct ipmi_recv_msg *rv; rv =kmalloc (sizeof (struct ipmi_recv_msg ),GFP_ATOMIC ); -if (rv) +if (rv) { rv->done = free_recv_msg; +atomic_inc (&recv_msg_inuse_count); + }return rv; }
@@ -1924,6 +1933,8 @@ static __exit void cleanup_ipmi(void) { +int count; +if (!initialized)return ;
@@ -1940,6 +1951,16 @@ } initialized =0 ; + +/* Check for buffer leaks. */ + count =atomic_read (&smi_msg_inuse_count); +if (count !=0 ) +printk ("ipmi_msghandler: SMI message count %d at exit\n" , + count); + count =atomic_read (&recv_msg_inuse_count); +if (count !=0 ) +printk ("ipmi_msghandler: recv message count %d at exit\n" , + count); }module_exit (cleanup_ipmi);
home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_kcs_intf.c line 584
Quoted 12 lines · Corey Minyard
home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_kcs_intf.c renamed from drivers/char/ipmi/ipmi_kcs_intf.c +3
-2@@ -563,8 +576,9 @@spin_lock_irqsave (&(kcs_info->kcs_lock), flags); result =kcs_event_handler (kcs_info,0 );while (result !=KCS_SM_IDLE ) { -udelay (500 ); - result =kcs_event_handler (kcs_info,500 ); +udelay (KCS_SHORT_TIMEOUT_USEC ); + result =kcs_event_handler (kcs_info, +KCS_SHORT_TIMEOUT_USEC ); }spin_unlock_irqrestore (&(kcs_info->kcs_lock), flags);return ;
Do you really want to udelay this long with interrupts disabled? Certainly comments in kcs_event[_handler] indicate you're aware of the issue, but the code does not belie this fact :)
Not only is the udelay itself "long" relatively speaking, but it's in a loop. Which also calls a function that contains a loop that is potentially infinite is hardware is being wonky.
Jeff
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
This only occurs in "run to completion" mode, which is a special mode the driver goes into after a panic. This allows the driver to get messages out during a panic to do things like extend the watchdog timer and send panic information.
- -Corey
Quoted 21 lines · Jeff Garzik
home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_kcs_intf.c line 584
Quoted 12 lines · Corey Minyard
home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_kcs_intf.c renamed from drivers/char/ipmi/ipmi_kcs_intf.c +3
-2@@ -563,8 +576,9 @@spin_lock_irqsave (&(kcs_info->kcs_lock), flags); result =kcs_event_handler (kcs_info,0 );while (result !=KCS_SM_IDLE ) { -udelay (500 ); - result =kcs_event_handler (kcs_info,500 ); +udelay (KCS_SHORT_TIMEOUT_USEC ); + result =kcs_event_handler (kcs_info, +KCS_SHORT_TIMEOUT_USEC ); }spin_unlock_irqrestore (&(kcs_info->kcs_lock), flags);return ;Do you really want to udelay this long with interrupts disabled? Certainly comments in kcs_event[_handler] indicate you're aware of the issue, but the code does not belie this fact :)
Quoted 5 lines
Not only is the udelay itself "long" relatively speaking, but it's in a loop. Which also calls a function that contains a loop that is potentially infinite is hardware is being wonky.
Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQE+jGUhmUvlb4BhfF4RAp2ZAJ40cXa1O1tv1wITiFzMsTuaTDejEgCfeOzM Uw0PgpEhlmDkyF9yejO/r4A=
=l5ia
-----END PGP SIGNATURE-----