Re: [Patch] x86,mm: check freeze request in page fault handler

[Michal Hocko]

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@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: David Rientjes <rientjes@xxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxx>
> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> Cc: Tejun Heo <tj@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Cong Wang <xiyou.wangcong@xxxxxxxxx>
> ---
>  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(&current->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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/