On Tuesday, 10 July 2007 17:00, Oleg Nesterov wrote:
> On 07/09, Rafael J. Wysocki wrote:
> >
> > Commit b74d0deb968e1f85942f17080eace015ce3c332c has changed
> > recalc_sigpending_tsk() so that it doesn't clear TIF_SIGPENDING. For this
> > reason, the freezer should not send fake signals to kernel threads any more,
> > since otherwise some of them may run with TIF_SIGPENDING set forever if the
> > freezing of kernel threads fails.
>
> I personally think it is very good to get rid of signals to kthread, regardless
> of changed behaviour of recalc_sigpending_tsk().
Yes, anyway. :-)
> > +static int freeze_user_process(struct task_struct *p)
> > +{
> > + int ret = 1;
> > +
> > + task_lock(p);
> > + if (has_mm(p)) {
> > + if (freezing(p)) {
> > + if (!signal_pending(p))
> > + fake_signal_wake_up(p, 0);
> > + else
> > + wake_up_state(p, TASK_INTERRUPTIBLE);
>
> Why do we need the "else" branch? It is already a bug if the task sleeps
> in TASK_INTERRUPTIBLE state but has signal_pending().
>
> The same for freeze_task(). Actually, they look very similar. Imho, it would
> be better to have a single function with a "user_space_only" parameter.
OK, will do.
> > @@ -99,7 +99,6 @@ static inline int has_pending_signals(si
> > static int recalc_sigpending_tsk(struct task_struct *t)
> > {
> > if (t->signal->group_stop_count > 0 ||
> > - (freezing(t)) ||
> > PENDING(&t->pending, &t->blocked) ||
> > PENDING(&t->signal->shared_pending, &t->blocked)) {
> > set_tsk_thread_flag(t, TIF_SIGPENDING);
>
> This is important (and imho good) change, but changelog says nothing about it.
Yes, I need to modify the changelog.
> I guess this should works because freeze_user_process/freeze_task repeatedly
> "re-send" the signal in a loop when TIF_SIGPENDING is cleared, yes?
Exactly.
> > +#define wait_event_freezable(wq, condition) \
> > +({ \
> > + int __ret; \
> > + do { \
> > + __ret = wait_event_interruptible(wq, \
> > + (condition) || freezing(current)); \
> > + } while (try_to_freeze()); \
> > + __ret; \
> > +})
>
> I don't think this is right.
>
> wait_event_freezable() should return success _only_ if "condition" == true.
> What if TIF_FREEZE was cleared between freezing() and try_to_freeze() ?
Hmm, that means we should loop while(!(condition)) . I didn't do that
previously, because I thought that would be a problem if (condition)
is satisfied in wait_event_interruptible(), but then changes immediately.
> > --- linux-2.6.22-rc6-mm1.orig/drivers/input/gameport/gameport.c
> > +++ linux-2.6.22-rc6-mm1/drivers/input/gameport/gameport.c
> > @@ -448,9 +448,8 @@ static int gameport_thread(void *nothing
> > set_freezable();
> > do {
> > gameport_handle_event();
> > - wait_event_interruptible(gameport_wait,
> > + wait_event_freezable(gameport_wait,
> > kthread_should_stop() || !list_empty(&gameport_event_list));
> > - try_to_freeze();
> > } while (!kthread_should_stop());
>
> Isn't it better to break this patch into 2 separate ones? The first adds
> wait_event_freezable() and "fixes" gameport_thread() and friends in advance,
> the second deals with TIF_SIGPENDING. (please ignore if this is not convenient).
Not really. I thought about that, but the change in gameport_thread() and
friends is actually necessary _because_ we don't send signals to kernel
threads any more.
I'll rework the patch in a while, but I think I'll send the entire series once
again in a new thread, tomorrow.
Greetings,
Rafael
--
"Premature optimization is the root of all evil." - Donald Knuth
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
