ipmi: add timer thread

We must poll for responses to commands when interrupts aren't in use. The default poll interval is based on using a kernel timer, which varies with HZ. For character-based interfaces like KCS and SMIC though, that can be way too slow (>15 minutes to flash a new firmware with KCS, >20 seconds to retrieve the sensor list).

This creates a low-priority kernel thread to poll more often. If the state machine is idle, so is the kernel thread. But if there's an active command, it polls quite rapidly. This decrease a firmware flash time from 15 minutes to 1.5 minutes, and the sensor list time to 4.5 seconds, on a Dell PowerEdge x8x system.

The timer-based polling remains, to ensure some amount of responsiveness even under high user process CPU load.

Checking for a stopped timer at rmmod now uses atomics and del_timer_sync() to ensure safe stoppage.

...

drivers/char/ipmi/ipmi_si_intf.c +20 -0

+static int ipmi_thread(void *data)
+{
+	struct smi_info *smi_info = data;
+	unsigned long flags, last=1;
+	enum si_sm_result smi_result;
+
+	daemonize("kipmi%d", smi_info->intf_num);
+	allow_signal(SIGKILL);
+	set_user_nice(current, 19);
+	while (!atomic_read(&smi_info->stop_operation)) {
+		schedule_timeout(last);
+		spin_lock_irqsave(&(smi_info->si_lock), flags);
+		smi_result=smi_event_handler(smi_info, 0);
+		spin_unlock_irqrestore(&(smi_info->si_lock), flags);
+		if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
+			last = 0;
+		else if (smi_result == SI_SM_CALL_WITH_DELAY) {
+			udelay(1);
+			last = 0;
+		}

Quoted 51 lines · Corey Minyard

We must poll for responses to commands when interrupts aren't in use. The default poll interval is based on using a kernel timer, which varies with HZ. For character-based interfaces like KCS and SMIC though, that can be way too slow (>15 minutes to flash a new firmware with KCS, >20 seconds to retrieve the sensor list).

This creates a low-priority kernel thread to poll more often. If the state machine is idle, so is the kernel thread. But if there's an active command, it polls quite rapidly. This decrease a firmware flash time from 15 minutes to 1.5 minutes, and the sensor list time to 4.5 seconds, on a Dell PowerEdge x8x system.

The timer-based polling remains, to ensure some amount of responsiveness even under high user process CPU load.

Checking for a stopped timer at rmmod now uses atomics and del_timer_sync() to ensure safe stoppage.

...

drivers/char/ipmi/ipmi_si_intf.c +30 -0

+static int ipmi_thread(void *data)
+{
+     struct smi_info *smi_info = data;
+     unsigned long flags, last=1;
+     enum si_sm_result smi_result;
+
+     daemonize("kipmi%d", smi_info->intf_num);
+     allow_signal(SIGKILL);
+     set_user_nice(current, 19);
+     while (!atomic_read(&smi_info->stop_operation)) {
+             schedule_timeout(last);
+             spin_lock_irqsave(&(smi_info->si_lock), flags);
+             smi_result=smi_event_handler(smi_info, 0);
+             spin_unlock_irqrestore(&(smi_info->si_lock), flags);
+             if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
+                     last = 0;
+             else if (smi_result == SI_SM_CALL_WITH_DELAY) {
+                     udelay(1);
+                     last = 0;
+             }
+             else {
+                     /* System is idle; go to sleep */
+                     last = 1;
+                     current->state = TASK_INTERRUPTIBLE;
+             }
+     }
+     smi_info->thread_pid = 0;
+     complete_and_exit(&(smi_info->exiting), 0);
+     return 0;
+}

The first call to schedule_timeout() here will not actually sleep at all, due to it being in state TASK_RUNNING. Is that deliberate?

Also, this thread can exit in state TASK_INTERUPTIBLE. That's not a bug per-se, but apparently it'll spit a warning in some of the patches which Ingo is working on. I don't know why, but I'm sure there's a good reason ;)

Quoted 2 lines · Andrew Morton

The first call to schedule_timeout() here will not actually sleep at all, due to it being in state TASK_RUNNING. Is that deliberate?

Quoted 4 lines

Also, this thread can exit in state TASK_INTERUPTIBLE. That's not a bug per-se, but apparently it'll spit a warning in some of the patches which Ingo is working on. I don't know why, but I'm sure there's a good reason ;)

You beat me to this one, Andrew! :) Both issue can be avoided by using schedule_timeout_interruptible().

Additionally, I think the last variable is simply being used to switch between a 0 and 1 jiffy sleep (took me a while to figure that out in GMail sadly -- any chance the variable could be renamed?). In the current implementaion of schedule_timeout(), these will result in the same behavior, expiring the timer at the next timer interrupt (the next jiffy increment is the first time we'll notice we had a timer in the past to expire). Not sure if that's the intent and perhaps just a means to indicate what is desired (a sleep will still occur, even though a udelay() has already in the loop, for instance), but wanted to make sure.

Quoted 52 lines · Andrew Morton

Quoted 50 lines · Corey Minyard

We must poll for responses to commands when interrupts aren't in use. The default poll interval is based on using a kernel timer, which varies with HZ. For character-based interfaces like KCS and SMIC though, that can be way too slow (>15 minutes to flash a new firmware with KCS, >20 seconds to retrieve the sensor list).

This creates a low-priority kernel thread to poll more often. If the state machine is idle, so is the kernel thread. But if there's an active command, it polls quite rapidly. This decrease a firmware flash time from 15 minutes to 1.5 minutes, and the sensor list time to 4.5 seconds, on a Dell PowerEdge x8x system.

The timer-based polling remains, to ensure some amount of responsiveness even under high user process CPU load.

Checking for a stopped timer at rmmod now uses atomics and del_timer_sync() to ensure safe stoppage.

...

drivers/char/ipmi/ipmi_si_intf.c +30 -0

+static int ipmi_thread(void *data)
+{
+     struct smi_info *smi_info = data;
+     unsigned long flags, last=1;
+     enum si_sm_result smi_result;
+
+     daemonize("kipmi%d", smi_info->intf_num);
+     allow_signal(SIGKILL);
+     set_user_nice(current, 19);
+     while (!atomic_read(&smi_info->stop_operation)) {
+             schedule_timeout(last);
+             spin_lock_irqsave(&(smi_info->si_lock), flags);
+             smi_result=smi_event_handler(smi_info, 0);
+             spin_unlock_irqrestore(&(smi_info->si_lock), flags);
+             if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
+                     last = 0;
+             else if (smi_result == SI_SM_CALL_WITH_DELAY) {
+                     udelay(1);
+                     last = 0;
+             }
+             else {
+                     /* System is idle; go to sleep */
+                     last = 1;
+                     current->state = TASK_INTERRUPTIBLE;
+             }
+     }
+     smi_info->thread_pid = 0;
+     complete_and_exit(&(smi_info->exiting), 0);
+     return 0;
+}

<snip>

Quoted 7 lines

The first call to schedule_timeout() here will not actually sleep at all, due to it being in state TASK_RUNNING. Is that deliberate?

Also, this thread can exit in state TASK_INTERUPTIBLE. That's not a bug per-se, but apparently it'll spit a warning in some of the patches which Ingo is working on. I don't know why, but I'm sure there's a good reason ;)

You beat me to this one, Andrew! :) Both issue can be avoided by using schedule_timeout_interruptible().

Additionally, I think the last variable is simply being used to switch between a 0 and 1 jiffy sleep (took me a while to figure that out in GMail sadly -- any chance the variable could be renamed?). In the current implementaion of schedule_timeout(), these will result in the same behavior, expiring the timer at the next timer interrupt (the next jiffy increment is the first time we'll notice we had a timer in the past to expire). Not sure if that's the intent and perhaps just a means to indicate what is desired (a sleep will still occur, even though a udelay() has already in the loop, for instance), but wanted to make sure.