Messages in this thread | | | Date | Tue, 12 Aug 2014 14:09:05 +0200 | From | Michal Hocko <> | Subject | Re: [Patch] x86,mm: check freeze request in page fault handler |
| |
On Mon 11-08-14 17:53:55, Cong Wang wrote: > When a process triggers a page fault and kernel keeps > trying to retry the fault, there is no chance for this process > to be frozen, so the freeze request will always be pending.
The retry cannot happen indefinitely, no?
Besides that the patch is broken in at least 2 ways. You are not releasing mmap_sem and this will break memcg OOM killer handling.
If a memcg is under OOM (because of hard limit) then try_charge calls mem_cgroup_oom which marks the current task with OOM information. Notably takes a reference to memcg->css. The charge fail will then gets up the pagefault stack until we get to mm_fault_error where you put the task into freezer and then returns without pagefault_out_of_memory which would handle memcg specific parts in mem_cgroup_oom_synchronize. If the task wakes up and the page fault retry succeeds (because some charges were released in the meantime) then you leak a reference to memcg->css.
Besides that the whole change would need a better justification. Why other archs do not need this?
> This patch lets the page fault handler check pending > freeze request and freeze current process if so. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: David Rientjes <rientjes@google.com> > Cc: Michal Hocko <mhocko@suse.cz> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Tejun Heo <tj@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > arch/x86/mm/fault.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > index a241946..ad9728a 100644 > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -14,6 +14,7 @@ > #include <linux/hugetlb.h> /* hstate_index_to_shift */ > #include <linux/prefetch.h> /* prefetchw */ > #include <linux/context_tracking.h> /* exception_enter(), ... */ > +#include <linux/freezer.h> /* try_to_freeze() */ > > #include <asm/traps.h> /* dotraplinkage, ... */ > #include <asm/pgalloc.h> /* pgd_*(), ... */ > @@ -885,6 +886,9 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code, > up_read(¤t->mm->mmap_sem); > no_context(regs, error_code, address, 0, 0); > return; > + } else if (signal_pending(current) && (error_code & PF_USER)) { > + if (try_to_freeze()) > + return; > } > > if (fault & VM_FAULT_OOM) { > -- > 1.8.3.1 >
-- Michal Hocko SUSE Labs
| |