Re: [PATCH 9/9] ipmi: add timer thread

[Nish Aravamudan <nish.aravamudan@gmail.com>]

On 10/23/05, Andrew Morton <akpm@osdl.org> wrote:
> Corey Minyard <minyard@acm.org> wrote:
> >
> > 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.
> >
> > ...
> >
> > +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>

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

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