| Message ID | 1475225434-3753-1-git-send-email-aryabinin@virtuozzo.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 09/30, Andrey Ryabinin wrote: > > @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state) > if (core_waiters > 0) { > struct core_thread *ptr; > > + freezer_do_not_count(); > wait_for_completion(&core_state->startup); > + freezer_count(); Agreed... we could probably even do --- x/fs/coredump.c +++ x/fs/coredump.c @@ -423,7 +423,13 @@ static int coredump_wait(int exit_code, if (core_waiters > 0) { struct core_thread *ptr; - wait_for_completion(&core_state->startup); + if (wait_for_completion_interruptible(&core_state->startup)) { + /* see the comment in dump_interrupted() */ + down_write(&mm->mmap_sem); + coredump_finish(mm, false); + up_write(&mm->mmap_sem); + return -EINTR; + } /* * Wait for all the threads to become inactive, so that * all the thread context (extended register state, like but this change looks fine to me too. Acked-by: Oleg Nesterov <oleg@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 2016-09-30 11:50:34, Andrey Ryabinin wrote: > It could be not possible to freeze coredumping task when it waits > for 'core_state->startup' completion, because threads are frozen > in get_signal() before they got a chance to complete 'core_state->startup'. > > Use freezer_do_not_count() to tell freezer to ignore coredumping > task while it waits for core_state->startup completion. > > Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: stable@vger.kernel.org Acked-by: Pavel Machek <pavel@ucw.cz>
On Fri 30-09-16 14:47:41, Oleg Nesterov wrote: > On 09/30, Andrey Ryabinin wrote: > > > > @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state) > > if (core_waiters > 0) { > > struct core_thread *ptr; > > > > + freezer_do_not_count(); > > wait_for_completion(&core_state->startup); > > + freezer_count(); > > Agreed... we could probably even do > > --- x/fs/coredump.c > +++ x/fs/coredump.c > @@ -423,7 +423,13 @@ static int coredump_wait(int exit_code, > if (core_waiters > 0) { > struct core_thread *ptr; > > - wait_for_completion(&core_state->startup); > + if (wait_for_completion_interruptible(&core_state->startup)) { > + /* see the comment in dump_interrupted() */ > + down_write(&mm->mmap_sem); > + coredump_finish(mm, false); > + up_write(&mm->mmap_sem); > + return -EINTR; > + } > /* > * Wait for all the threads to become inactive, so that > * all the thread context (extended register state, like This looks like a very good idea to me. We really want to make the whole coredump_wait killable. I guess this should help us to remove the hackish sig->flags & SIGNAL_GROUP_COREDUMP check from __task_will_free_mem. Or are there any other problems that would make oom victims in the middle of coredump problematic?
On 10/04, Michal Hocko wrote: > > On Fri 30-09-16 14:47:41, Oleg Nesterov wrote: > > On 09/30, Andrey Ryabinin wrote: > > > > > > @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state) > > > if (core_waiters > 0) { > > > struct core_thread *ptr; > > > > > > + freezer_do_not_count(); > > > wait_for_completion(&core_state->startup); > > > + freezer_count(); > > > > Agreed... we could probably even do > > > > --- x/fs/coredump.c > > +++ x/fs/coredump.c > > @@ -423,7 +423,13 @@ static int coredump_wait(int exit_code, > > if (core_waiters > 0) { > > struct core_thread *ptr; > > > > - wait_for_completion(&core_state->startup); > > + if (wait_for_completion_interruptible(&core_state->startup)) { > > + /* see the comment in dump_interrupted() */ > > + down_write(&mm->mmap_sem); > > + coredump_finish(mm, false); > > + up_write(&mm->mmap_sem); > > + return -EINTR; > > + } > > /* > > * Wait for all the threads to become inactive, so that > > * all the thread context (extended register state, like > > This looks like a very good idea to me. We really want to make the whole > coredump_wait killable. Well, it is already killable. And with the change above it can sleep in down_write(mmap_sem) and we really need this lock to abort, so it won't necessarily react to SIGKILL faster. > I guess this should help us to remove the > hackish sig->flags & SIGNAL_GROUP_COREDUMP check from > __task_will_free_mem. Why? This doesn't depend on "killable". __task_will_free_mem() checks this flag to detect the CLONE_VM processes which won't exit soon because they participate in the coredumping. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 04-10-16 18:13:05, Oleg Nesterov wrote: > On 10/04, Michal Hocko wrote: > > > > On Fri 30-09-16 14:47:41, Oleg Nesterov wrote: > > > On 09/30, Andrey Ryabinin wrote: > > > > > > > > @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state) > > > > if (core_waiters > 0) { > > > > struct core_thread *ptr; > > > > > > > > + freezer_do_not_count(); > > > > wait_for_completion(&core_state->startup); > > > > + freezer_count(); > > > > > > Agreed... we could probably even do > > > > > > --- x/fs/coredump.c > > > +++ x/fs/coredump.c > > > @@ -423,7 +423,13 @@ static int coredump_wait(int exit_code, > > > if (core_waiters > 0) { > > > struct core_thread *ptr; > > > > > > - wait_for_completion(&core_state->startup); > > > + if (wait_for_completion_interruptible(&core_state->startup)) { > > > + /* see the comment in dump_interrupted() */ > > > + down_write(&mm->mmap_sem); > > > + coredump_finish(mm, false); > > > + up_write(&mm->mmap_sem); > > > + return -EINTR; > > > + } > > > /* > > > * Wait for all the threads to become inactive, so that > > > * all the thread context (extended register state, like > > > > This looks like a very good idea to me. We really want to make the whole > > coredump_wait killable. > > Well, it is already killable. Except wait_for_completion is not killable and the exiting tasks might be blocked in a !killable state blocking this one to continue. But... > And with the change above it can sleep > in down_write(mmap_sem) and we really need this lock to abort, so it > won't necessarily react to SIGKILL faster. you are right that somebody might be holding mmap_sem and we cannot get rid of it here. > > I guess this should help us to remove the > > hackish sig->flags & SIGNAL_GROUP_COREDUMP check from > > __task_will_free_mem. > > Why? This doesn't depend on "killable". __task_will_free_mem() checks > this flag to detect the CLONE_VM processes which won't exit soon because > they participate in the coredumping. I just (wrongly) assumed that if we make this path killable completely we can guarantee a forward progress and get rid of SIGNAL_GROUP_COREDUMP check completely. But you are right this won't be sufficient.
On 09/30/2016 11:50 AM, Andrey Ryabinin wrote: > It could be not possible to freeze coredumping task when it waits > for 'core_state->startup' completion, because threads are frozen > in get_signal() before they got a chance to complete 'core_state->startup'. > > Use freezer_do_not_count() to tell freezer to ignore coredumping > task while it waits for core_state->startup completion. > > Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: stable@vger.kernel.org > --- Ping. Can someone apply this please? > fs/coredump.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/coredump.c b/fs/coredump.c > index 281b768..eb9c92c 100644 > --- a/fs/coredump.c > +++ b/fs/coredump.c > @@ -1,6 +1,7 @@ > #include <linux/slab.h> > #include <linux/file.h> > #include <linux/fdtable.h> > +#include <linux/freezer.h> > #include <linux/mm.h> > #include <linux/stat.h> > #include <linux/fcntl.h> > @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state) > if (core_waiters > 0) { > struct core_thread *ptr; > > + freezer_do_not_count(); > wait_for_completion(&core_state->startup); > + freezer_count(); > /* > * Wait for all the threads to become inactive, so that > * all the thread context (extended register state, like > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 30 Sep 2016 11:50:34 +0300 Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > It could be not possible to freeze coredumping task when it waits > for 'core_state->startup' completion, because threads are frozen > in get_signal() before they got a chance to complete 'core_state->startup'. > > Use freezer_do_not_count() to tell freezer to ignore coredumping > task while it waits for core_state->startup completion. > > Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: stable@vger.kernel.org The changelog provides no reason why this patch should be merged into -stable. Nor into anything else, really. Please (as always) provide a full description of the bug's end-user visible effects. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/coredump.c b/fs/coredump.c index 281b768..eb9c92c 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -1,6 +1,7 @@ #include <linux/slab.h> #include <linux/file.h> #include <linux/fdtable.h> +#include <linux/freezer.h> #include <linux/mm.h> #include <linux/stat.h> #include <linux/fcntl.h> @@ -423,7 +424,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state) if (core_waiters > 0) { struct core_thread *ptr; + freezer_do_not_count(); wait_for_completion(&core_state->startup); + freezer_count(); /* * Wait for all the threads to become inactive, so that * all the thread context (extended register state, like
It could be not possible to freeze coredumping task when it waits for 'core_state->startup' completion, because threads are frozen in get_signal() before they got a chance to complete 'core_state->startup'. Use freezer_do_not_count() to tell freezer to ignore coredumping task while it waits for core_state->startup completion. Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> Cc: stable@vger.kernel.org --- fs/coredump.c | 3 +++ 1 file changed, 3 insertions(+)