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

[George Anzinger]

Nish Aravamudan wrote:
> On 10/23/05, Andrew Morton <akpm@xxxxxxxx> wrote:
>
> > Corey Minyard <minyard@xxxxxxx> 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.

I think it would be VERY nice if we could eliminate calls to sleep for NIL time.  Sooner or later 
these are going to bite us very badly.

In the above code, the handling of the "task state" is, well, funny.  If last is 0, it is not 
modified implying that:

	if (last)
		schedule_timeout(last);

might be a better rendering of the intended function.
>

--
George Anzinger   george@xxxxxxxxxx
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/