Re: [linux-usb-devel] init 1 kill khubd on 2.6.11

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

On 5/1/05, Andrew Morton <akpm@osdl.org> wrote:
> Nish Aravamudan <nish.aravamudan@gmail.com> wrote:
> >
> > > -       /* Send me a signal to get me die (for debugging) */
> > >         do {
> > >                 hub_events();
> > > -               wait_event_interruptible(khubd_wait, !list_empty(&hub_event_list));
> > > +               wait_event_interruptible(khubd_wait,
> > > +                               !list_empty(&hub_event_list) ||
> > > +                               kthread_should_stop());
> > >                 try_to_freeze(PF_FREEZE);
> > > -       } while (!signal_pending(current));
> > > +       } while (!kthread_should_stop() || !list_empty(&hub_event_list));
> >
> > Shouldn't this simply be a wait_event(), instead of
> > wait_event_interruptible()?
> 
> That would cause uninterruptible sleep, which contributes to load average.

True, and this is the argument I face(d) with a lot of the msleep()
changes I made. I guess I would like a comment for this case, where
we're using wait_event_interruptible(), but actually are ignoring the
signals that might make us return early.
 
> > Then the do-while() can be gotten rid of,
> > as the only reason it is there currently, I guess, is to ignore
> > signals?
> 
> Nope, the do-while is a basic part of the daemon's operation: keep doing
> stuff until either there's no stuff to do or until we're told to exit.

I see that now, thanks.

> > Also, the while's conditional should be (!kthread_should_stop() ||
> > list_empty(&hub_event_list) to match the negation of wait_event's?
> > (wait_event() expects the condition to stop on, while while() expects
> > the condition to continue on)
> 
> Nope, the wait_event_interruptible test says
> 
>   "sleep unless the list is not empty or I am being asked to exit"
> 
> the while termination test says
> 
>   "loop until the list is empty and I am being asked to stop".
> 
> I think.  I had to scratch my head for a while over that code ;)

You're right again -- sorry for the noise, I must have been reading it
wrong. Rewriting it as !(kthread_should_stop() &&
list_empty(&hub_event_list)) helped me :)

Thanks!
Nish