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