Re: [patch 5/6] mm: memcg: enable memcg OOM killer only for user faults

[KOSAKI Motohiro <kosaki.motohiro@gmail.com>]

(7/29/13 3:44 PM), Johannes Weiner wrote:
> On Mon, Jul 29, 2013 at 03:18:36PM -0400, KOSAKI Motohiro wrote:
>> (7/25/13 6:25 PM), Johannes Weiner wrote:
>>> System calls and kernel faults (uaccess, gup) can handle an out of
>>> memory situation gracefully and just return -ENOMEM.
>>>
>>> Enable the memcg OOM killer only for user faults, where it's really
>>> the only option available.
>>>
>>> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>>> ---
>>>    include/linux/memcontrol.h | 23 +++++++++++++++++++++++
>>>    include/linux/sched.h      |  3 +++
>>>    mm/filemap.c               | 11 ++++++++++-
>>>    mm/memcontrol.c            |  2 +-
>>>    mm/memory.c                | 40 ++++++++++++++++++++++++++++++----------
>>>    5 files changed, 67 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>> index 7b4d9d7..9bb5eeb 100644
>>> --- a/include/linux/memcontrol.h
>>> +++ b/include/linux/memcontrol.h
>>> @@ -125,6 +125,24 @@ extern void mem_cgroup_print_oom_info(struct mem_cgroup *memcg,
>>>    extern void mem_cgroup_replace_page_cache(struct page *oldpage,
>>>    					struct page *newpage);
>>>
>>> +/**
>>> + * mem_cgroup_xchg_may_oom - toggle the memcg OOM killer for a task
>>> + * @p: task
>>> + * @new: true to enable, false to disable
>>> + *
>>> + * Toggle whether a failed memcg charge should invoke the OOM killer
>>> + * or just return -ENOMEM.  Returns the previous toggle state.
>>> + */
>>> +static inline bool mem_cgroup_xchg_may_oom(struct task_struct *p, bool new)
>>> +{
>>> +	bool old;
>>> +
>>> +	old = p->memcg_oom.may_oom;
>>> +	p->memcg_oom.may_oom = new;
>>> +
>>> +	return old;
>>> +}
>>
>> The name of xchg strongly suggest the function use compare-swap op. So, it seems
>> misleading name. I suggest just use "set_*" or something else. In linux kernel,
>> many setter functions already return old value. Don't mind.
>
> I renamed it to bool mem_cgroup_toggle_oom(bool onoff) when I
> incorporated Michal's feedback, would you be okay with that?

Yes, thank you.

>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index fc09d21..4b3effc 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -1398,6 +1398,9 @@ struct task_struct {
>>>    		unsigned long memsw_nr_pages; /* uncharged mem+swap usage */
>>>    	} memcg_batch;
>>>    	unsigned int memcg_kmem_skip_account;
>>> +	struct memcg_oom_info {
>>> +		unsigned int may_oom:1;
>>> +	} memcg_oom;
>>
>> This ":1" makes slower but doesn't diet any memory space, right? I suggest
>> to use bool. If anybody need to diet in future, he may change it to bit field.
>> That's ok, let's stop too early and questionable micro optimization.
>
> It should sit in the same word as the memcg_kmem_skip_account, plus
> I'm adding another bit in the next patch (in_memcg_oom), so we save
> space.  It's also the OOM path, so anything but performance critical.

Oh, if you added another bit too, it's ok, of course.

>>> diff --git a/mm/filemap.c b/mm/filemap.c
>>> index a6981fe..2932810 100644
>>> --- a/mm/filemap.c
>>> +++ b/mm/filemap.c
>>> @@ -1617,6 +1617,7 @@ int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>>>    	struct file_ra_state *ra = &file->f_ra;
>>>    	struct inode *inode = mapping->host;
>>>    	pgoff_t offset = vmf->pgoff;
>>> +	unsigned int may_oom;
>>
>> Why don't you use bool? your mem_cgroup_xchg_may_oom() uses bool and it seems cleaner more.
>
> Yup, forgot to convert it with the interface, I changed it to bool.

thx.


>>> +	/*
>>> +	 * Enable the memcg OOM handling for faults triggered in user
>>> +	 * space.  Kernel faults are handled more gracefully.
>>> +	 */
>>> +	if (flags & FAULT_FLAG_USER)
>>> +		WARN_ON(mem_cgroup_xchg_may_oom(current, true) == true);
>>
>> Please don't assume WARN_ON never erase any code. I'm not surprised if embedded
>> guys replace WARN_ON with nop in future.
>
> That would be really messed up.
>
> But at the same time, the WARN_ON() obfuscates what's going on a
> little bit, so putting it separately should make the code more
> readable.  I'll change it.
>
> Thanks for your input!

No problem. :)