On Friday, 15 June 2007 13:31, Oleg Nesterov wrote:
> On 06/15, Rafael J. Wysocki wrote:
> >
> > +static void freeze_task(struct task_struct *p)
> > +{
> > if (!freezing(p)) {
> > rmb();
> > if (!frozen(p)) {
> > set_freeze_flag(p);
> > - if (p->state == TASK_STOPPED)
> > - force_sig_specific(SIGSTOP, p);
> > - spin_lock_irqsave(&p->sighand->siglock, flags);
> > - signal_wake_up(p, p->state == TASK_STOPPED);
> > - spin_unlock_irqrestore(&p->sighand->siglock, flags);
> > + task_lock(p);
> > + /* We don't want to send signals to kernel threads */
> > + if (p->mm && !(p->flags & PF_BORROWED_MM)) {
> > + task_unlock(p);
> > + send_fake_signal(p);
> > + } else {
> > + task_unlock(p);
> > + wake_up_state(p, TASK_INTERRUPTIBLE);
> > + }
>
> I don't think this is enough. Note that recalc_sigpending() checks freezing().
> So a kernel thread still can get TIF_SIGPENDING if it does
> recalc_sigpending().
Yes, you're right, I have overlooked that.
Still, this can be prevented by changing recalc_sigpending_tsk() in the
following way:
--- linux-2.6.22-rc4.orig/kernel/signal.c 2007-06-07 00:01:48.000000000
+0200
+++ linux-2.6.22-rc4/kernel/signal.c 2007-06-15 21:22:00.000000000 +0200
@@ -99,13 +99,13 @@ 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);
return 1;
}
- clear_tsk_thread_flag(t, TIF_SIGPENDING);
+ if (!freezing(t))
+ clear_tsk_thread_flag(t, TIF_SIGPENDING);
return 0;
}
> > --- linux-2.6.22-rc4-mm2.orig/include/linux/wait.h 2007-06-15
> > 01:05:33.000000000 +0200
> > +++ linux-2.6.22-rc4-mm2/include/linux/wait.h 2007-06-15
> > 01:05:41.000000000 +0200
> > @@ -240,7 +240,7 @@ do {
> > \
> > prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
> > if (condition) \
> > break; \
> > - if (!signal_pending(current)) { \
> > + if (!signal_pending(current) && !freezing(current)) { \
> > schedule(); \
> > continue; \
> > } \
>
> Personally, I think we should not modify wait_event_interruptible() and
> friends.
> If a kernel thread wants to be frozen, it should take care about freezing()
> itself.
Yes, I agree, but wanted to get a working patch quickly. ;-)
I think we might define freezer-friendly versions of wait_event_interruptible()
and friends in <linux/freezer.h>, like this:
+#define wait_event_interruptible_ff(wq, condition) \
+({ \
+ int __ret = 0; \
+ if (!(condition) && !freezing(current)) \
+ __wait_event_interruptible(wq, \
+ (condition) || freezing(current), \
+ __ret); \
+ if (!(condition) && freezing(current)) \
+ __ret = -ERESTARTSYS; \
+ __ret; \
+})
and make the freezable kernel threads use them instead of the ones defined
in <linux/wait.h>. There are only a few threads that need that, BTW.
> OK, I guess I was too paranoid and you were right, it is better to ignore this
> minor problem for now.
Okay, but I still think we shouldn't send fake signals to kernel threads. :-)
I'd like to revisit it after 2.6.22 is out, if you don't mind.
Greetings,
Rafael
--
"Premature optimization is the root of all evil." - Donald Knuth
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
