On Fri, Jul 26, 2013 at 04:16:42PM +0200, Michal Hocko wrote:
> On Thu 25-07-13 18:25:37, 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@xxxxxxxxxxx>
>
> It looks OK to me, but I have few comments bellow. Nothing really huge
> but I do not like mem_cgroup_xchg_may_oom for !MEMCG.
:-)
> > ---
> > 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
>
> Is this ever safe to call on !current? If not then I wouldn't allow to
> give p as a parameter.
Makes sense, I removed the parameter.
> > @@ -1634,10 +1639,14 @@ int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > * We found the page, so try async readahead before
> > * waiting for the lock.
> > */
> > + may_oom = mem_cgroup_xchg_may_oom(current, 0);
>
> s/0/false/
>
> below ditto
Oops, updated both sites.
> > do_async_mmap_readahead(vma, ra, file, page, offset);
> > + mem_cgroup_xchg_may_oom(current, may_oom);
> > } else if (!page) {
> > /* No page in the page cache at all */
> > + may_oom = mem_cgroup_xchg_may_oom(current, 0);
> > do_sync_mmap_readahead(vma, ra, file, offset);
> > + mem_cgroup_xchg_may_oom(current, may_oom);
> > count_vm_event(PGMAJFAULT);
> > mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT);
> > ret = VM_FAULT_MAJOR;
> [...]
> > diff --git a/mm/memory.c b/mm/memory.c
> > index f2ab2a8..5ea7b47 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> [...]
> > @@ -3851,6 +3843,34 @@ retry:
> > return handle_pte_fault(mm, vma, address, pte, pmd, flags);
> > }
> >
> > +int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> > + unsigned long address, unsigned int flags)
> > +{
> > + int ret;
> > +
> > + __set_current_state(TASK_RUNNING);
> > +
> > + count_vm_event(PGFAULT);
> > + mem_cgroup_count_vm_event(mm, PGFAULT);
> > +
> > + /* do counter updates before entering really critical section. */
> > + check_sync_rss_stat(current);
> > +
> > + /*
> > + * 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);
> > +
> > + ret = __handle_mm_fault(mm, vma, address, flags);
> > +
> > + if (flags & FAULT_FLAG_USER)
> > + WARN_ON(mem_cgroup_xchg_may_oom(current, false) == false);
>
> Ohh, I see why you used !new in mem_cgroup_xchg_may_oom for !MEMCG case
> above. This could be fixed easily if you add mem_cgroup_{enable,disable}_oom
> which would be empty for !MEMCG.
You're right, it's much cleaner this way. I added the enable/disable
functions, which has the advantage that the memcg-specific WARN_ON is
also not in generic code but encapsulated nicely.
Thanks for your feedback!
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
