Messages in this thread | | | Date | Thu, 28 Feb 2013 07:39:51 -0800 | Subject | Re: [PATCH v2 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread | From | Mandeep Singh Baines <> |
| |
On Wed, Feb 27, 2013 at 10:08 AM, Oleg Nesterov <oleg@redhat.com> wrote: > On 02/26, Mandeep Singh Baines wrote: >> >> >> Change freeze_task() to check PF_DUMPCORE along with PF_KTHREAD. We >> >> need to recheck PF_DUMPCORE under ->siglock to avoid the race with >> >> zap_threads() which can set this flag right before we take the lock. >> >> >> > >> > Won't this prevent suspend? > > Hmm. I guess you mean that pipe_write() can hang in pipe_wait() if the > user-space handler was already freezed... Damn, and I even mentioned > this race when we discussed this 2 weeks ago. > > I need to think, but most probably you are right, and we need another > solution... > >> You'd rather have reliable suspend than coredumps that aren't >> truncated so you need to set TIF_SIGPENDING to break waits in the >> dump_write path. > > Oh, I agree. In this case the necessary changes look simple. > >> static void wait_for_dump_helpers(struct file *file) >> { >> struct pipe_inode_info *pipe; >> >> pipe = file->f_path.dentry->d_inode->i_pipe; >> >> pipe_lock(pipe); >> pipe->readers++; >> pipe->writers--; >> >> while (pipe->readers > 1) { >> unsigned long flags; >> >> wake_up_interruptible_sync(&pipe->wait); >> kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); >> pipe_wait(pipe); >> >> pipe_unlock(pipe); >> try_to_freeze(); >> pipe_lock(pipe); >> >> if (fatal_signal_pending(current)) >> break; >> >> /* Clear fake signal from freeze_task(). */ >> spin_lock_irqsave(¤t->sighand->siglock, flags); >> recalc_sigpending(); >> spin_unlock_irqrestore(¤t->sighand->siglock, flags); > > IIRC, this is what you added into your tree. But note that > recalc_sigpending() is wrong, exactly because (say) SIGCHLD can > be pending if it was sent before we set SIGNAL_GROUP_COREDUMP. > > So this code needs something like > > spin_lock_irq(siglock); > if (!fatal_signal_pending) > clear_thread_flag(TIF_SIGPENDING); > spin_unlock_irq(siglock); > > Or we need to change recalc_sigpending() to check SIGNAL_GROUP_COREDUMP > or PF_DUMPCORE. I'd like to avoid this, but perhaps we have to do this... > > (Btw, this is offtopic, but whatever we do 3/3 still looks like a nice > cleanup to me, although it probably needs more changes) > >> What do you think? That would fix most cases. You'll still get a >> truncated core if you were to receive the signal during pipe_write or >> something. > > Let me think a bit... > > Right now I can only say that personally I do not really like the > idea to fix wait_for_dump_helpers() but not pipe_write(). I mean, > if pipe_write() can fail due to freezing(), then why should we care > about wait_for_dump_helpers() ? Let them all fail, suspend is not > that often. >
I agree. This does seem like a minor problem since its likely very rare. Probably not worth fixing unless there is an elegant, non-invasive solution.
As long as suspend is reliable, a very small chance of a truncated core dumps is OK.
Regards, Mandeep
> Or we should try to make everything freezer-friendly. But if > freeze_task() sets TIF_SIGPENDING then we need the ugly "retry" > logic in dump_write()... Not good. > > Thanks Mandeep. If you have other ideas please tell me ;) > > Oleg. >
| |