Message-ID: <20130226145412.GA31943@redhat.com>
Date:	Tue, 26 Feb 2013 15:54:12 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Lianwei Wang <lianwei.wang@...il.com>,
	linux-kernel@...r.kernel.org, "Rafael J. Wysocki" <rjw@...k.pl>
Subject: Re: PATCH: freezer: add fake signal clearing back when thaw task

Damn.

Tejun, et all, sorry for confusion.

Somehow I misunderstood your email completely! as if you argue with
my "freezer: do not send a fake signal to a PF_DUMPCORE thread"
change http://marc.info/?l=linux-kernel&m=136173112604220

As for "add fake signal clearing back when thaw task", I do not
understand why do we need this change too, so I agree with you.

On 02/26, Oleg Nesterov wrote:
>
> On 02/25, Tejun Heo wrote:
> >
> > (cc'ing Rafael and Oleg and quoting whole body)
> >
> > On Thu, Feb 21, 2013 at 02:19:21PM +0800, Lianwei Wang wrote:
> > > Hi Tejun Heo and all,
> > >
> > > The commit of "34b087e freezer: kill unused
> > > set_freezable_with_signal()" remove recalc_sigpending*() calls in
> > > freezer, so the user tasks get TIF_SIGPENDING fake signal that is set
> > > when freezing userspace process. It left the fake signal to userspcae
> > > which cause the userspace task that wait_event_freezable and friends
> > > return a wrong ERESTARTSYS. This is not good because it waste cpu time
> > > to handle the fake signal.
> >
> > Is this even measureable?  Freeze / thaw isn't exactly a hot path and
> > I'm having difficult time believing -ERESTARTSYS would have a
> > noticeable impact on anything.  Can you please explain why this is a
> > problem?
>
> For example, wait_for_dump_helpers() can fail because it checks
> signal_pending(). And we can sleep in TASK_KILLABLE because pipe_release()
> does wake_up_interruptible(). But this is minor, wait_for_dump_helpers()
> could be fixed without this change.
>
> The real problem is dump_write-like code. Say, pipe_write() can fail if
> signal_pending() == T. I am not saying this is unsolvable, in fact I was
> going to add the freeze + recalc_sigpending + retry logic initially, but
> this looks soooo ugly.
>
> Also. Rightly or not, but I came to conclusion that this change is right
> even if we forget about killable/freezable problems in coredump. The
> coredumping thread is no longer a "real" user-space process. It can never
> handle the signals, it doesn't return to user-mode, but it does a lot of
> work in kernel space. So I think it should look as PF_KTHREAD to freezer.
> 
> > > Can we just call the recalc_sigpending to clear the fake signal for
> > > userspace tasks? as below patch do:
> > >
> > > +static void fake_signal_clear(struct task_struct *p)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + if (lock_task_sighand(p, &flags)) {
> > > +     recalc_sigpending();
> > > +     unlock_task_sighand(p, &flags);
> > > + }
> 
> You know, _perhaps_ we have another reason for this change. Otherwise
> wait_event_freezable() doesn't look right. Or we should clarify that
> it is only for PF_KTHREAD but than we can simplify wait_event_freezable().
> And in this case I do not think we should reintroduce recalc_sigpending()
> removed by 34b087e48 "freezer: kill unused set_freezable_with_signal()".
> 
> I'll write another email about this, nobody actually need
> wait_event_freezable().
> 
> But. The change above can't help the coredumping thread. It still
> needs to do
> 
> 	spin_lock_irq(current->siglock);
> 	if (!__fatal_signal_pending(current))
> 		clear_thread_flag(TIF_SIGPENDING);
> 	spin_unlock_irq(current->siglock);
> 
> or we should change recalc_sigpending() to check PF_KTHREAD.
> 
> Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists