Re: PATCH: freezer: add fake signal clearing back when thaw task

[Oleg Nesterov <oleg@redhat.com>]

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.