IPMI driver version 19 release

IPMI driver version 19 release

[Corey Minyard]

[-- Attachment #1.1: Type: text/plain, Size: 813 bytes --]

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

[-- Attachment #1.2: linux-2.5.67-pre-ipmi-diff --]
[-- Type: text/plain, Size: 6389 bytes --]

Only in linux-2.5/drivers/char/ipmi: CVS
diff -ur linux-2.5/drivers/char/ipmi/ipmi_kcs_intf.c /home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_kcs_intf.c
--- linux-2.5/drivers/char/ipmi/ipmi_kcs_intf.c	Thu Mar 27 23:14:18 2003
+++ /home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_kcs_intf.c	Thu Apr  3 08:46:26 2003
@@ -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)
diff -ur linux-2.5/drivers/char/ipmi/ipmi_msghandler.c /home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_msghandler.c
--- linux-2.5/drivers/char/ipmi/ipmi_msghandler.c	Fri Feb 21 15:44:13 2003
+++ /home/minyard/2.5.66/linux-main/drivers/char/ipmi/ipmi_msghandler.c	Thu Apr  3 07:58:06 2003
@@ -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);
 

[-- Attachment #2: Type: application/pgp-signature, Size: 252 bytes --]

Re: IPMI driver version 19 release

[Jeff Garzik]

On Thu, Apr 03, 2003 at 10:24:02AM -0600, Corey Minyard wrote:
> @@ -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

Re: IPMI driver version 19 release

[Corey Minyard]

-----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

Jeff Garzik wrote:

>On Thu, Apr 03, 2003 at 10:24:02AM -0600, Corey Minyard wrote:
>
>>@@ -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 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-----