[PATCH v6 0/3] Directed kmem charging

[PATCH v6 0/3] Directed kmem charging

[Shakeel Butt]

This patchset introduces memcg variant memory allocation functions.  The
caller can explicitly pass the memcg to charge for kmem allocations.
Currently the kernel, for __GFP_ACCOUNT memory allocation requests,
extract the memcg of the current task to charge for the kmem allocation.
This patch series introduces kmem allocation functions where the caller
can pass the pointer to the remote memcg.  The remote memcg will be
charged for the allocation instead of the memcg of the caller.  However
the caller must have a reference to the remote memcg.  This patch series
also introduces scope API for targeted memcg charging. So, all the
__GFP_ACCOUNT alloctions within the specified scope will be charged to
the given target memcg.

Shakeel Butt (3):
  mm: memcg: remote memcg charging for kmem allocations
  fs: fsnotify: account fsnotify metadata to kmemcg
  fs, mm: account buffer_head to kmemcg

 fs/buffer.c                          | 14 ++++-
 fs/notify/dnotify/dnotify.c          |  5 +-
 fs/notify/fanotify/fanotify.c        |  6 +-
 fs/notify/fanotify/fanotify_user.c   |  5 +-
 fs/notify/group.c                    |  6 ++
 fs/notify/inotify/inotify_fsnotify.c |  2 +-
 fs/notify/inotify/inotify_user.c     |  5 +-
 include/linux/fsnotify_backend.h     | 12 ++--
 include/linux/memcontrol.h           | 14 +++++
 include/linux/sched.h                |  3 +
 include/linux/sched/mm.h             | 24 ++++++++
 include/linux/slab.h                 | 83 ++++++++++++++++++++++++++++
 kernel/fork.c                        |  3 +
 mm/memcontrol.c                      | 54 ++++++++++++++++--
 14 files changed, 220 insertions(+), 16 deletions(-)

-- 
2.18.0.rc1.244.gcf134e6275-goog

[PATCH 1/3] mm: memcg: remote memcg charging for kmem allocations

[Shakeel Butt]

Introduce the memcg variant for kmalloc[_node] and
kmem_cache_alloc[_node]. For kmem_cache_alloc, the kernel switches the
root kmem cache with the memcg specific kmem cache for __GFP_ACCOUNT
allocations to charge those allocations to the memcg. However, the memcg
to charge is extracted from the current task_struct. This patch
introduces the variant of kmem cache allocation functions where the memcg
can be provided explicitly by the caller instead of deducing the memcg
from the current task.

The kmalloc allocations are underlying served using the kmem caches unless
the size of the allocation request is larger than KMALLOC_MAX_CACHE_SIZE,
in which case, the kmem caches are bypassed and the request is routed
directly to page allocator. So, for __GFP_ACCOUNT kmalloc allocations,
the memcg of current task is charged. This patch introduces memcg variant
of kmalloc functions to allow callers to provide memcg for charging.

These functions are useful for use-cases where the allocations should be
charged to the memcg different from the memcg of the caller. One such
concrete use-case is the allocations for fsnotify event objects where the
objects should be charged to the listener instead of the producer.

One requirement to call these functions is that the caller must have the
reference to the memcg provided to these functions. If reference
acquition on the given memcg is failed (it can fail if memcg is offline)
then the current's memcg is tried. These functions implicitly assumes
that the caller wants a __GFP_ACCOUNT allocation.

This patch also introduces scope API for targeted memcg charging. All
the __GFP_ACCOUNT allocations between memalloc_memcg_save(target_memcg)
and memalloc_memcg_restore(old_memcg) will be charged to target_memcg.

Traditionally kmem charging is skipped for allocations by kthreads and
allocations during interrupts. The reason is that the current's memcg
might not be the right owner for such allocations. However targeted
memcg charging does not have such limitation and can work even for
allocations by kthreads and for allocations during interrupts. For now
due to lack of actual use-case, targeted memcg charging for such
allocations is not added. Though this might change in future.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Greg Thelen <gthelen@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Changelog sinve v5:
- Added more explanation in commit message.
- Added handling of NULL memcg for targeted memcg allocation functions.

Changelog since v4:
- Removed branch from hot path of memory charging.

Changelog since v3:
- Added node variant of directed kmem allocation functions.

Changelog since v2:
- Merge the kmalloc_memcg patch into this patch.
- Instead of plumbing memcg throughout, use field in task_struct to pass
  the target_memcg.

Changelog since v1:
- Fixed build for SLOB

 include/linux/sched.h    |  3 ++
 include/linux/sched/mm.h | 24 ++++++++++++
 include/linux/slab.h     | 83 ++++++++++++++++++++++++++++++++++++++++
 kernel/fork.c            |  3 ++
 mm/memcontrol.c          | 18 ++++++++-
 5 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 87bf02d93a27..cbd0def60fd4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1149,6 +1149,9 @@ struct task_struct {
 
 	/* Number of pages to reclaim on returning to userland: */
 	unsigned int			memcg_nr_pages_over_high;
+
+	/* Used by memcontrol for targeted memcg charge: */
+	struct mem_cgroup		*target_memcg;
 #endif
 
 #ifdef CONFIG_UPROBES
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 44d356f5e47c..2ffb194f3f32 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -248,6 +248,30 @@ static inline void memalloc_noreclaim_restore(unsigned int flags)
 	current->flags = (current->flags & ~PF_MEMALLOC) | flags;
 }
 
+#ifdef CONFIG_MEMCG
+static inline struct mem_cgroup *memalloc_memcg_save(struct mem_cgroup *memcg)
+{
+	struct mem_cgroup *old_memcg = current->target_memcg;
+
+	current->target_memcg = memcg;
+	return old_memcg;
+}
+
+static inline void memalloc_memcg_restore(struct mem_cgroup *memcg)
+{
+	current->target_memcg = memcg;
+}
+#else
+static inline struct mem_cgroup *memalloc_memcg_save(struct mem_cgroup *memcg)
+{
+	return NULL;
+}
+
+static inline void memalloc_memcg_restore(struct mem_cgroup *memcg)
+{
+}
+#endif /* CONFIG_MEMCG */
+
 #ifdef CONFIG_MEMBARRIER
 enum {
 	MEMBARRIER_STATE_PRIVATE_EXPEDITED_READY		= (1U << 0),
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 14e3fe4bd6a1..2f6319fa0d3d 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -16,6 +16,7 @@
 #include <linux/overflow.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
+#include <linux/sched/mm.h>
 
 
 /*
@@ -375,6 +376,27 @@ static __always_inline void kfree_bulk(size_t size, void **p)
 	kmem_cache_free_bulk(NULL, size, p);
 }
 
+/*
+ * Calling kmem_cache_alloc_memcg implicitly assumes that the caller wants
+ * a __GFP_ACCOUNT allocation. However if memcg is NULL then
+ * kmem_cache_alloc_memcg is same as kmem_cache_alloc.
+ */
+static __always_inline void *kmem_cache_alloc_memcg(struct kmem_cache *cachep,
+						    gfp_t flags,
+						    struct mem_cgroup *memcg)
+{
+	struct mem_cgroup *old_memcg;
+	void *ptr;
+
+	if (!memcg)
+		return kmem_cache_alloc(cachep, flags);
+
+	old_memcg = memalloc_memcg_save(memcg);
+	ptr = kmem_cache_alloc(cachep, flags | __GFP_ACCOUNT);
+	memalloc_memcg_restore(old_memcg);
+	return ptr;
+}
+
 #ifdef CONFIG_NUMA
 void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignment __malloc;
 void *kmem_cache_alloc_node(struct kmem_cache *, gfp_t flags, int node) __assume_slab_alignment __malloc;
@@ -390,6 +412,27 @@ static __always_inline void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t f
 }
 #endif
 
+/*
+ * Calling kmem_cache_alloc_node_memcg implicitly assumes that the caller
+ * wants a __GFP_ACCOUNT allocation. However if memcg is NULL then
+ * kmem_cache_alloc_node_memcg is same as kmem_cache_alloc_node.
+ */
+static __always_inline void *
+kmem_cache_alloc_node_memcg(struct kmem_cache *cachep, gfp_t flags, int node,
+			    struct mem_cgroup *memcg)
+{
+	struct mem_cgroup *old_memcg;
+	void *ptr;
+
+	if (!memcg)
+		return kmem_cache_alloc_node(cachep, flags, node);
+
+	old_memcg = memalloc_memcg_save(memcg);
+	ptr = kmem_cache_alloc_node(cachep, flags | __GFP_ACCOUNT, node);
+	memalloc_memcg_restore(old_memcg);
+	return ptr;
+}
+
 #ifdef CONFIG_TRACING
 extern void *kmem_cache_alloc_trace(struct kmem_cache *, gfp_t, size_t) __assume_slab_alignment __malloc;
 
@@ -518,6 +561,26 @@ static __always_inline void *kmalloc(size_t size, gfp_t flags)
 	return __kmalloc(size, flags);
 }
 
+/*
+ * Calling kmalloc_memcg implicitly assumes that the caller wants a
+ * __GFP_ACCOUNT allocation. However if memcg is NULL then kmalloc_memcg
+ * is same as kmalloc.
+ */
+static __always_inline void *kmalloc_memcg(size_t size, gfp_t flags,
+					   struct mem_cgroup *memcg)
+{
+	struct mem_cgroup *old_memcg;
+	void *ptr;
+
+	if (!memcg)
+		return kmalloc(size, flags);
+
+	old_memcg = memalloc_memcg_save(memcg);
+	ptr = kmalloc(size, flags | __GFP_ACCOUNT);
+	memalloc_memcg_restore(old_memcg);
+	return ptr;
+}
+
 /*
  * Determine size used for the nth kmalloc cache.
  * return size or 0 if a kmalloc cache for that
@@ -555,6 +618,26 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
 	return __kmalloc_node(size, flags, node);
 }
 
+/*
+ * Calling kmalloc_node_memcg implicitly assumes that the caller wants a
+ * __GFP_ACCOUNT allocation. However if memcg is NULL then kmalloc_node_memcg
+ * is same as kmalloc_node.
+ */
+static __always_inline void *
+kmalloc_node_memcg(size_t size, gfp_t flags, int node, struct mem_cgroup *memcg)
+{
+	struct mem_cgroup *old_memcg;
+	void *ptr;
+
+	if (!memcg)
+		return kmalloc_node(size, flags, node);
+
+	old_memcg = memalloc_memcg_save(memcg);
+	ptr = kmalloc_node(size, flags | __GFP_ACCOUNT, node);
+	memalloc_memcg_restore(old_memcg);
+	return ptr;
+}
+
 struct memcg_cache_array {
 	struct rcu_head rcu;
 	struct kmem_cache *entries[0];
diff --git a/kernel/fork.c b/kernel/fork.c
index a64d0a19f174..5bf300015790 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -843,6 +843,9 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 	tsk->fail_nth = 0;
 #endif
 
+#ifdef CONFIG_MEMCG
+	tsk->target_memcg = NULL;
+#endif
 	return tsk;
 
 free_stack:
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 02e77c88967a..08bfb8c2411b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -701,6 +701,20 @@ static struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 	return memcg;
 }
 
+static __always_inline struct mem_cgroup *get_mem_cgroup(
+				struct mem_cgroup *memcg, struct mm_struct *mm)
+{
+	if (unlikely(memcg)) {
+		rcu_read_lock();
+		if (css_tryget_online(&memcg->css)) {
+			rcu_read_unlock();
+			return memcg;
+		}
+		rcu_read_unlock();
+	}
+	return get_mem_cgroup_from_mm(mm);
+}
+
 /**
  * mem_cgroup_iter - iterate over memory cgroup hierarchy
  * @root: hierarchy root
@@ -2260,7 +2274,7 @@ struct kmem_cache *memcg_kmem_get_cache(struct kmem_cache *cachep)
 	if (current->memcg_kmem_skip_account)
 		return cachep;
 
-	memcg = get_mem_cgroup_from_mm(current->mm);
+	memcg = get_mem_cgroup(current->target_memcg, current->mm);
 	kmemcg_id = READ_ONCE(memcg->kmemcg_id);
 	if (kmemcg_id < 0)
 		goto out;
@@ -2344,7 +2358,7 @@ int memcg_kmem_charge(struct page *page, gfp_t gfp, int order)
 	if (memcg_kmem_bypass())
 		return 0;
 
-	memcg = get_mem_cgroup_from_mm(current->mm);
+	memcg = get_mem_cgroup(current->target_memcg, current->mm);
 	if (!mem_cgroup_is_root(memcg)) {
 		ret = memcg_kmem_charge_memcg(page, gfp, order, memcg);
 		if (!ret)
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH 1/3] mm: memcg: remote memcg charging for kmem allocations

[Johannes Weiner]

On Mon, Jun 18, 2018 at 10:13:25PM -0700, Shakeel Butt wrote:
> @@ -248,6 +248,30 @@ static inline void memalloc_noreclaim_restore(unsigned int flags)
>  	current->flags = (current->flags & ~PF_MEMALLOC) | flags;
>  }
>  
> +#ifdef CONFIG_MEMCG
> +static inline struct mem_cgroup *memalloc_memcg_save(struct mem_cgroup *memcg)
> +{
> +	struct mem_cgroup *old_memcg = current->target_memcg;
> +
> +	current->target_memcg = memcg;
> +	return old_memcg;
> +}
> +
> +static inline void memalloc_memcg_restore(struct mem_cgroup *memcg)
> +{
> +	current->target_memcg = memcg;
> +}

The use_mm() and friends naming scheme would be better here:
memalloc_use_memcg(), memalloc_unuse_memcg(), current->active_memcg

> @@ -375,6 +376,27 @@ static __always_inline void kfree_bulk(size_t size, void **p)
>  	kmem_cache_free_bulk(NULL, size, p);
>  }
>  
> +/*
> + * Calling kmem_cache_alloc_memcg implicitly assumes that the caller wants
> + * a __GFP_ACCOUNT allocation. However if memcg is NULL then
> + * kmem_cache_alloc_memcg is same as kmem_cache_alloc.
> + */
> +static __always_inline void *kmem_cache_alloc_memcg(struct kmem_cache *cachep,
> +						    gfp_t flags,
> +						    struct mem_cgroup *memcg)
> +{
> +	struct mem_cgroup *old_memcg;
> +	void *ptr;
> +
> +	if (!memcg)
> +		return kmem_cache_alloc(cachep, flags);
> +
> +	old_memcg = memalloc_memcg_save(memcg);
> +	ptr = kmem_cache_alloc(cachep, flags | __GFP_ACCOUNT);
> +	memalloc_memcg_restore(old_memcg);
> +	return ptr;

I'm not a big fan of these functions as an interface because it
implies that kmem_cache_alloc() et al wouldn't charge a memcg - but
they do, just using current's memcg.

It's also a lot of churn to duplicate all the various slab functions.

Can you please inline the save/restore (or use/unuse) functions into
the callsites? If you make them handle NULL as parameters, it merely
adds two bracketing lines around the allocation call in the callsites,
which I think would be better to understand - in particular with a
comment on why we are charging *that* group instead of current's.

> +static __always_inline struct mem_cgroup *get_mem_cgroup(
> +				struct mem_cgroup *memcg, struct mm_struct *mm)
> +{
> +	if (unlikely(memcg)) {
> +		rcu_read_lock();
> +		if (css_tryget_online(&memcg->css)) {
> +			rcu_read_unlock();
> +			return memcg;
> +		}
> +		rcu_read_unlock();
> +	}
> +	return get_mem_cgroup_from_mm(mm);
> +}
> +
>  /**
>   * mem_cgroup_iter - iterate over memory cgroup hierarchy
>   * @root: hierarchy root
> @@ -2260,7 +2274,7 @@ struct kmem_cache *memcg_kmem_get_cache(struct kmem_cache *cachep)
>  	if (current->memcg_kmem_skip_account)
>  		return cachep;
>  
> -	memcg = get_mem_cgroup_from_mm(current->mm);
> +	memcg = get_mem_cgroup(current->target_memcg, current->mm);

get_mem_cgroup_from_current(), which uses current->active_memcg if set
and current->mm->memcg otherwise, would be a nicer abstraction IMO.

Re: [PATCH 1/3] mm: memcg: remote memcg charging for kmem allocations

[Shakeel Butt]

On Tue, Jun 19, 2018 at 9:22 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Mon, Jun 18, 2018 at 10:13:25PM -0700, Shakeel Butt wrote:
> > @@ -248,6 +248,30 @@ static inline void memalloc_noreclaim_restore(unsigned int flags)
> >       current->flags = (current->flags & ~PF_MEMALLOC) | flags;
> >  }
> >
> > +#ifdef CONFIG_MEMCG
> > +static inline struct mem_cgroup *memalloc_memcg_save(struct mem_cgroup *memcg)
> > +{
> > +     struct mem_cgroup *old_memcg = current->target_memcg;
> > +
> > +     current->target_memcg = memcg;
> > +     return old_memcg;
> > +}
> > +
> > +static inline void memalloc_memcg_restore(struct mem_cgroup *memcg)
> > +{
> > +     current->target_memcg = memcg;
> > +}
>
> The use_mm() and friends naming scheme would be better here:
> memalloc_use_memcg(), memalloc_unuse_memcg(), current->active_memcg
>

Ack. Though do you still think <linux/sched/mm.h> is the right place
for these functions?

> > @@ -375,6 +376,27 @@ static __always_inline void kfree_bulk(size_t size, void **p)
> >       kmem_cache_free_bulk(NULL, size, p);
> >  }
> >
> > +/*
> > + * Calling kmem_cache_alloc_memcg implicitly assumes that the caller wants
> > + * a __GFP_ACCOUNT allocation. However if memcg is NULL then
> > + * kmem_cache_alloc_memcg is same as kmem_cache_alloc.
> > + */
> > +static __always_inline void *kmem_cache_alloc_memcg(struct kmem_cache *cachep,
> > +                                                 gfp_t flags,
> > +                                                 struct mem_cgroup *memcg)
> > +{
> > +     struct mem_cgroup *old_memcg;
> > +     void *ptr;
> > +
> > +     if (!memcg)
> > +             return kmem_cache_alloc(cachep, flags);
> > +
> > +     old_memcg = memalloc_memcg_save(memcg);
> > +     ptr = kmem_cache_alloc(cachep, flags | __GFP_ACCOUNT);
> > +     memalloc_memcg_restore(old_memcg);
> > +     return ptr;
>
> I'm not a big fan of these functions as an interface because it
> implies that kmem_cache_alloc() et al wouldn't charge a memcg - but
> they do, just using current's memcg.
>
> It's also a lot of churn to duplicate all the various slab functions.
>
> Can you please inline the save/restore (or use/unuse) functions into
> the callsites? If you make them handle NULL as parameters, it merely
> adds two bracketing lines around the allocation call in the callsites,
> which I think would be better to understand - in particular with a
> comment on why we are charging *that* group instead of current's.
>

Ack.

> > +static __always_inline struct mem_cgroup *get_mem_cgroup(
> > +                             struct mem_cgroup *memcg, struct mm_struct *mm)
> > +{
> > +     if (unlikely(memcg)) {
> > +             rcu_read_lock();
> > +             if (css_tryget_online(&memcg->css)) {
> > +                     rcu_read_unlock();
> > +                     return memcg;
> > +             }
> > +             rcu_read_unlock();
> > +     }
> > +     return get_mem_cgroup_from_mm(mm);
> > +}
> > +
> >  /**
> >   * mem_cgroup_iter - iterate over memory cgroup hierarchy
> >   * @root: hierarchy root
> > @@ -2260,7 +2274,7 @@ struct kmem_cache *memcg_kmem_get_cache(struct kmem_cache *cachep)
> >       if (current->memcg_kmem_skip_account)
> >               return cachep;
> >
> > -     memcg = get_mem_cgroup_from_mm(current->mm);
> > +     memcg = get_mem_cgroup(current->target_memcg, current->mm);
>
> get_mem_cgroup_from_current(), which uses current->active_memcg if set
> and current->mm->memcg otherwise, would be a nicer abstraction IMO.

Ack.

thanks,
Shakeel

Re: [PATCH 1/3] mm: memcg: remote memcg charging for kmem allocations

[Johannes Weiner]

On Tue, Jun 19, 2018 at 04:31:18PM -0700, Shakeel Butt wrote:
> On Tue, Jun 19, 2018 at 9:22 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Mon, Jun 18, 2018 at 10:13:25PM -0700, Shakeel Butt wrote:
> > > @@ -248,6 +248,30 @@ static inline void memalloc_noreclaim_restore(unsigned int flags)
> > >       current->flags = (current->flags & ~PF_MEMALLOC) | flags;
> > >  }
> > >
> > > +#ifdef CONFIG_MEMCG
> > > +static inline struct mem_cgroup *memalloc_memcg_save(struct mem_cgroup *memcg)
> > > +{
> > > +     struct mem_cgroup *old_memcg = current->target_memcg;
> > > +
> > > +     current->target_memcg = memcg;
> > > +     return old_memcg;
> > > +}
> > > +
> > > +static inline void memalloc_memcg_restore(struct mem_cgroup *memcg)
> > > +{
> > > +     current->target_memcg = memcg;
> > > +}
> >
> > The use_mm() and friends naming scheme would be better here:
> > memalloc_use_memcg(), memalloc_unuse_memcg(), current->active_memcg
> >
> 
> Ack. Though do you still think <linux/sched/mm.h> is the right place
> for these functions?

Yeah, since it has the memalloc_* prefix, we should keep it there.

If we did use_memcg(), unuse_memcg(), I'd put it into memcontrol.h,
but it seems a little terse; memalloc adds valuable context, IMO.

Thanks Shakeel!

[PATCH 2/3] fs: fsnotify: account fsnotify metadata to kmemcg

[Shakeel Butt]

A lot of memory can be consumed by the events generated for the huge or
unlimited queues if there is either no or slow listener.  This can cause
system level memory pressure or OOMs.  So, it's better to account the
fsnotify kmem caches to the memcg of the listener.

There are seven fsnotify kmem caches and among them allocations from
dnotify_struct_cache, dnotify_mark_cache, fanotify_mark_cache and
inotify_inode_mark_cachep happens in the context of syscall from the
listener.  So, SLAB_ACCOUNT is enough for these caches.

The objects from fsnotify_mark_connector_cachep are not accounted as they
are small compared to the notification mark or events and it is unclear
whom to account connector to since it is shared by all events attached to
the inode.

The allocations from the event caches happen in the context of the event
producer.  For such caches we will need to remote charge the allocations
to the listener's memcg.  Thus we save the memcg reference in the
fsnotify_group structure of the listener.

This patch has also moved the members of fsnotify_group to keep the size
same, at least for 64 bit build, even with additional member by filling
the holes.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
Acked-by: Jan Kara <jack@suse.cz>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Greg Thelen <gthelen@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Changelog since v5:
- None

Changelog since v4:
- Fixed the build for CONFIG_MEMCG=n

Changelog since v3:
- Rebased over Jan's patches.
- Some cleanup based on Amir's comments.

Changelog since v2:
- None

Changelog since v1:
- no more charging fsnotify_mark_connector objects
- Fixed the build for SLOB

 fs/notify/dnotify/dnotify.c          |  5 +++--
 fs/notify/fanotify/fanotify.c        |  6 ++++--
 fs/notify/fanotify/fanotify_user.c   |  5 ++++-
 fs/notify/group.c                    |  6 ++++++
 fs/notify/inotify/inotify_fsnotify.c |  2 +-
 fs/notify/inotify/inotify_user.c     |  5 ++++-
 include/linux/fsnotify_backend.h     | 12 ++++++++----
 include/linux/memcontrol.h           |  7 +++++++
 mm/memcontrol.c                      | 15 +++++++++++++--
 9 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index e2bea2ac5dfb..a6365e6bc047 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -384,8 +384,9 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
 
 static int __init dnotify_init(void)
 {
-	dnotify_struct_cache = KMEM_CACHE(dnotify_struct, SLAB_PANIC);
-	dnotify_mark_cache = KMEM_CACHE(dnotify_mark, SLAB_PANIC);
+	dnotify_struct_cache = KMEM_CACHE(dnotify_struct,
+					  SLAB_PANIC|SLAB_ACCOUNT);
+	dnotify_mark_cache = KMEM_CACHE(dnotify_mark, SLAB_PANIC|SLAB_ACCOUNT);
 
 	dnotify_group = fsnotify_alloc_group(&dnotify_fsnotify_ops);
 	if (IS_ERR(dnotify_group))
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index f90842efea13..c8d6e37a4855 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -154,14 +154,16 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
 	if (fanotify_is_perm_event(mask)) {
 		struct fanotify_perm_event_info *pevent;
 
-		pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp);
+		pevent = kmem_cache_alloc_memcg(fanotify_perm_event_cachep, gfp,
+						group->memcg);
 		if (!pevent)
 			return NULL;
 		event = &pevent->fae;
 		pevent->response = 0;
 		goto init;
 	}
-	event = kmem_cache_alloc(fanotify_event_cachep, gfp);
+	event = kmem_cache_alloc_memcg(fanotify_event_cachep, gfp,
+				       group->memcg);
 	if (!event)
 		return NULL;
 init: __maybe_unused
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index ec4d8c59d0e3..0cf45041dc32 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -16,6 +16,7 @@
 #include <linux/uaccess.h>
 #include <linux/compat.h>
 #include <linux/sched/signal.h>
+#include <linux/memcontrol.h>
 
 #include <asm/ioctls.h>
 
@@ -756,6 +757,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 
 	group->fanotify_data.user = user;
 	atomic_inc(&user->fanotify_listeners);
+	group->memcg = get_mem_cgroup_from_mm(current->mm);
 
 	oevent = fanotify_alloc_event(group, NULL, FS_Q_OVERFLOW, NULL);
 	if (unlikely(!oevent)) {
@@ -957,7 +959,8 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
-	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark, SLAB_PANIC);
+	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
+					 SLAB_PANIC|SLAB_ACCOUNT);
 	fanotify_event_cachep = KMEM_CACHE(fanotify_event_info, SLAB_PANIC);
 	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
 		fanotify_perm_event_cachep =
diff --git a/fs/notify/group.c b/fs/notify/group.c
index aa5468f23e45..300fc0f62115 100644
--- a/fs/notify/group.c
+++ b/fs/notify/group.c
@@ -22,6 +22,7 @@
 #include <linux/srcu.h>
 #include <linux/rculist.h>
 #include <linux/wait.h>
+#include <linux/memcontrol.h>
 
 #include <linux/fsnotify_backend.h>
 #include "fsnotify.h"
@@ -36,6 +37,11 @@ static void fsnotify_final_destroy_group(struct fsnotify_group *group)
 	if (group->ops->free_group_priv)
 		group->ops->free_group_priv(group);
 
+#ifdef CONFIG_MEMCG
+	if (group->memcg)
+		css_put(&group->memcg->css);
+#endif
+
 	kfree(group);
 }
 
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index 9ab6dde38a14..749e76ea7793 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -98,7 +98,7 @@ int inotify_handle_event(struct fsnotify_group *group,
 	i_mark = container_of(inode_mark, struct inotify_inode_mark,
 			      fsn_mark);
 
-	event = kmalloc(alloc_len, GFP_KERNEL);
+	event = kmalloc_memcg(alloc_len, GFP_KERNEL, group->memcg);
 	if (unlikely(!event)) {
 		/*
 		 * Treat lost event due to ENOMEM the same way as queue
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 1cf5b779d862..749c46ababa0 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -38,6 +38,7 @@
 #include <linux/uaccess.h>
 #include <linux/poll.h>
 #include <linux/wait.h>
+#include <linux/memcontrol.h>
 
 #include "inotify.h"
 #include "../fdinfo.h"
@@ -636,6 +637,7 @@ static struct fsnotify_group *inotify_new_group(unsigned int max_events)
 	oevent->name_len = 0;
 
 	group->max_events = max_events;
+	group->memcg = get_mem_cgroup_from_mm(current->mm);
 
 	spin_lock_init(&group->inotify_data.idr_lock);
 	idr_init(&group->inotify_data.idr);
@@ -808,7 +810,8 @@ static int __init inotify_user_setup(void)
 
 	BUG_ON(hweight32(ALL_INOTIFY_BITS) != 21);
 
-	inotify_inode_mark_cachep = KMEM_CACHE(inotify_inode_mark, SLAB_PANIC);
+	inotify_inode_mark_cachep = KMEM_CACHE(inotify_inode_mark,
+					       SLAB_PANIC|SLAB_ACCOUNT);
 
 	inotify_max_queued_events = 16384;
 	init_user_ns.ucount_max[UCOUNT_INOTIFY_INSTANCES] = 128;
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index b38964a7a521..a0c4790c5302 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -84,6 +84,8 @@ struct fsnotify_event_private_data;
 struct fsnotify_fname;
 struct fsnotify_iter_info;
 
+struct mem_cgroup;
+
 /*
  * Each group much define these ops.  The fsnotify infrastructure will call
  * these operations for each relevant group.
@@ -127,6 +129,8 @@ struct fsnotify_event {
  * everything will be cleaned up.
  */
 struct fsnotify_group {
+	const struct fsnotify_ops *ops;	/* how this group handles things */
+
 	/*
 	 * How the refcnt is used is up to each group.  When the refcnt hits 0
 	 * fsnotify will clean up all of the resources associated with this group.
@@ -137,8 +141,6 @@ struct fsnotify_group {
 	 */
 	refcount_t refcnt;		/* things with interest in this group */
 
-	const struct fsnotify_ops *ops;	/* how this group handles things */
-
 	/* needed to send notification to userspace */
 	spinlock_t notification_lock;		/* protect the notification_list */
 	struct list_head notification_list;	/* list of event_holder this group needs to send to userspace */
@@ -160,6 +162,8 @@ struct fsnotify_group {
 	atomic_t num_marks;		/* 1 for each mark and 1 for not being
 					 * past the point of no return when freeing
 					 * a group */
+	atomic_t user_waits;		/* Number of tasks waiting for user
+					 * response */
 	struct list_head marks_list;	/* all inode marks for this group */
 
 	struct fasync_struct *fsn_fa;    /* async notification */
@@ -167,8 +171,8 @@ struct fsnotify_group {
 	struct fsnotify_event *overflow_event;	/* Event we queue when the
 						 * notification list is too
 						 * full */
-	atomic_t user_waits;		/* Number of tasks waiting for user
-					 * response */
+
+	struct mem_cgroup *memcg;	/* memcg to charge allocations */
 
 	/* groups can define private fields here or use the void *private */
 	union {
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 3607913032be..6c857be8a9b7 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -378,6 +378,8 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *, struct pglist_data *);
 bool task_in_mem_cgroup(struct task_struct *task, struct mem_cgroup *memcg);
 struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
 
+struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm);
+
 static inline
 struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *css){
 	return css ? container_of(css, struct mem_cgroup, css) : NULL;
@@ -857,6 +859,11 @@ static inline bool task_in_mem_cgroup(struct task_struct *task,
 	return true;
 }
 
+static inline struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
+{
+	return NULL;
+}
+
 static inline void mem_cgroup_put(struct mem_cgroup *memcg)
 {
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 08bfb8c2411b..c481e661e051 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -678,9 +678,20 @@ struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
 }
 EXPORT_SYMBOL(mem_cgroup_from_task);
 
-static struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
+/**
+ * get_mem_cgroup_from_mm: Obtain a reference on given mm_struct's memcg.
+ * @mm: mm from which memcg should be extracted. It can be NULL.
+ *
+ * Obtain a reference on mm->memcg and returns it if successful. Otherwise
+ * root_mem_cgroup is returned. However if mem_cgroup is disabled, NULL is
+ * returned.
+ */
+struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 {
-	struct mem_cgroup *memcg = NULL;
+	struct mem_cgroup *memcg;
+
+	if (mem_cgroup_disabled())
+		return NULL;
 
 	rcu_read_lock();
 	do {
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH 2/3] fs: fsnotify: account fsnotify metadata to kmemcg

[Amir Goldstein]

On Tue, Jun 19, 2018 at 8:13 AM, Shakeel Butt <shakeelb@google.com> wrote:
> A lot of memory can be consumed by the events generated for the huge or
> unlimited queues if there is either no or slow listener.  This can cause
> system level memory pressure or OOMs.  So, it's better to account the
> fsnotify kmem caches to the memcg of the listener.
>
> There are seven fsnotify kmem caches and among them allocations from
> dnotify_struct_cache, dnotify_mark_cache, fanotify_mark_cache and
> inotify_inode_mark_cachep happens in the context of syscall from the
> listener.  So, SLAB_ACCOUNT is enough for these caches.
>
> The objects from fsnotify_mark_connector_cachep are not accounted as they
> are small compared to the notification mark or events and it is unclear
> whom to account connector to since it is shared by all events attached to
> the inode.
>
> The allocations from the event caches happen in the context of the event
> producer.  For such caches we will need to remote charge the allocations
> to the listener's memcg.  Thus we save the memcg reference in the
> fsnotify_group structure of the listener.
>
> This patch has also moved the members of fsnotify_group to keep the size
> same, at least for 64 bit build, even with additional member by filling
> the holes.
>
> Signed-off-by: Shakeel Butt <shakeelb@google.com>
> Acked-by: Jan Kara <jack@suse.cz>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Greg Thelen <gthelen@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
> Changelog since v5:
> - None
>
> Changelog since v4:
> - Fixed the build for CONFIG_MEMCG=n
>
> Changelog since v3:
> - Rebased over Jan's patches.
> - Some cleanup based on Amir's comments.
>
> Changelog since v2:
> - None
>
> Changelog since v1:
> - no more charging fsnotify_mark_connector objects
> - Fixed the build for SLOB
>
>  fs/notify/dnotify/dnotify.c          |  5 +++--
>  fs/notify/fanotify/fanotify.c        |  6 ++++--
>  fs/notify/fanotify/fanotify_user.c   |  5 ++++-
>  fs/notify/group.c                    |  6 ++++++
>  fs/notify/inotify/inotify_fsnotify.c |  2 +-
>  fs/notify/inotify/inotify_user.c     |  5 ++++-
>  include/linux/fsnotify_backend.h     | 12 ++++++++----
>  include/linux/memcontrol.h           |  7 +++++++
>  mm/memcontrol.c                      | 15 +++++++++++++--
>  9 files changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index e2bea2ac5dfb..a6365e6bc047 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -384,8 +384,9 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>
>  static int __init dnotify_init(void)
>  {
> -       dnotify_struct_cache = KMEM_CACHE(dnotify_struct, SLAB_PANIC);
> -       dnotify_mark_cache = KMEM_CACHE(dnotify_mark, SLAB_PANIC);
> +       dnotify_struct_cache = KMEM_CACHE(dnotify_struct,
> +                                         SLAB_PANIC|SLAB_ACCOUNT);
> +       dnotify_mark_cache = KMEM_CACHE(dnotify_mark, SLAB_PANIC|SLAB_ACCOUNT);
>
>         dnotify_group = fsnotify_alloc_group(&dnotify_fsnotify_ops);
>         if (IS_ERR(dnotify_group))
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index f90842efea13..c8d6e37a4855 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -154,14 +154,16 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
>         if (fanotify_is_perm_event(mask)) {
>                 struct fanotify_perm_event_info *pevent;
>
> -               pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp);
> +               pevent = kmem_cache_alloc_memcg(fanotify_perm_event_cachep, gfp,
> +                                               group->memcg);
>                 if (!pevent)
>                         return NULL;
>                 event = &pevent->fae;
>                 pevent->response = 0;
>                 goto init;
>         }
> -       event = kmem_cache_alloc(fanotify_event_cachep, gfp);
> +       event = kmem_cache_alloc_memcg(fanotify_event_cachep, gfp,
> +                                      group->memcg);
>         if (!event)
>                 return NULL;
>  init: __maybe_unused
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index ec4d8c59d0e3..0cf45041dc32 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -16,6 +16,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/compat.h>
>  #include <linux/sched/signal.h>
> +#include <linux/memcontrol.h>
>
>  #include <asm/ioctls.h>
>
> @@ -756,6 +757,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>
>         group->fanotify_data.user = user;
>         atomic_inc(&user->fanotify_listeners);
> +       group->memcg = get_mem_cgroup_from_mm(current->mm);
>

It seem to me that you should export a wrapper to modules with
the mem_cgroup_ prefix and not sure that need to pass current->mm
to this wrapper.

>         oevent = fanotify_alloc_event(group, NULL, FS_Q_OVERFLOW, NULL);
>         if (unlikely(!oevent)) {
> @@ -957,7 +959,8 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark,
>   */
>  static int __init fanotify_user_setup(void)
>  {
> -       fanotify_mark_cache = KMEM_CACHE(fsnotify_mark, SLAB_PANIC);
> +       fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
> +                                        SLAB_PANIC|SLAB_ACCOUNT);
>         fanotify_event_cachep = KMEM_CACHE(fanotify_event_info, SLAB_PANIC);
>         if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
>                 fanotify_perm_event_cachep =
> diff --git a/fs/notify/group.c b/fs/notify/group.c
> index aa5468f23e45..300fc0f62115 100644
> --- a/fs/notify/group.c
> +++ b/fs/notify/group.c
> @@ -22,6 +22,7 @@
>  #include <linux/srcu.h>
>  #include <linux/rculist.h>
>  #include <linux/wait.h>
> +#include <linux/memcontrol.h>
>
>  #include <linux/fsnotify_backend.h>
>  #include "fsnotify.h"
> @@ -36,6 +37,11 @@ static void fsnotify_final_destroy_group(struct fsnotify_group *group)
>         if (group->ops->free_group_priv)
>                 group->ops->free_group_priv(group);
>
> +#ifdef CONFIG_MEMCG
> +       if (group->memcg)
> +               css_put(&group->memcg->css);
> +#endif
> +

This looks very much like an internal implementation detail that has no
business in an external module. I see evidence that you have used
mem_cgroup_put() in the patch at some point and I agree that you
need to export a pair of matching helpers to external modules.

Thanks,
Amir.

Re: [PATCH 2/3] fs: fsnotify: account fsnotify metadata to kmemcg

[Shakeel Butt]

On Tue, Jun 19, 2018 at 12:20 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jun 19, 2018 at 8:13 AM, Shakeel Butt <shakeelb@google.com> wrote:
> > A lot of memory can be consumed by the events generated for the huge or
> > unlimited queues if there is either no or slow listener.  This can cause
> > system level memory pressure or OOMs.  So, it's better to account the
> > fsnotify kmem caches to the memcg of the listener.
> >
> > There are seven fsnotify kmem caches and among them allocations from
> > dnotify_struct_cache, dnotify_mark_cache, fanotify_mark_cache and
> > inotify_inode_mark_cachep happens in the context of syscall from the
> > listener.  So, SLAB_ACCOUNT is enough for these caches.
> >
> > The objects from fsnotify_mark_connector_cachep are not accounted as they
> > are small compared to the notification mark or events and it is unclear
> > whom to account connector to since it is shared by all events attached to
> > the inode.
> >
> > The allocations from the event caches happen in the context of the event
> > producer.  For such caches we will need to remote charge the allocations
> > to the listener's memcg.  Thus we save the memcg reference in the
> > fsnotify_group structure of the listener.
> >
> > This patch has also moved the members of fsnotify_group to keep the size
> > same, at least for 64 bit build, even with additional member by filling
> > the holes.
> >
> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > Acked-by: Jan Kara <jack@suse.cz>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: Pekka Enberg <penberg@kernel.org>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > Cc: Greg Thelen <gthelen@google.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > Changelog since v5:
> > - None
> >
> > Changelog since v4:
> > - Fixed the build for CONFIG_MEMCG=n
> >
> > Changelog since v3:
> > - Rebased over Jan's patches.
> > - Some cleanup based on Amir's comments.
> >
> > Changelog since v2:
> > - None
> >
> > Changelog since v1:
> > - no more charging fsnotify_mark_connector objects
> > - Fixed the build for SLOB
> >
> >  fs/notify/dnotify/dnotify.c          |  5 +++--
> >  fs/notify/fanotify/fanotify.c        |  6 ++++--
> >  fs/notify/fanotify/fanotify_user.c   |  5 ++++-
> >  fs/notify/group.c                    |  6 ++++++
> >  fs/notify/inotify/inotify_fsnotify.c |  2 +-
> >  fs/notify/inotify/inotify_user.c     |  5 ++++-
> >  include/linux/fsnotify_backend.h     | 12 ++++++++----
> >  include/linux/memcontrol.h           |  7 +++++++
> >  mm/memcontrol.c                      | 15 +++++++++++++--
> >  9 files changed, 50 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> > index e2bea2ac5dfb..a6365e6bc047 100644
> > --- a/fs/notify/dnotify/dnotify.c
> > +++ b/fs/notify/dnotify/dnotify.c
> > @@ -384,8 +384,9 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> >
> >  static int __init dnotify_init(void)
> >  {
> > -       dnotify_struct_cache = KMEM_CACHE(dnotify_struct, SLAB_PANIC);
> > -       dnotify_mark_cache = KMEM_CACHE(dnotify_mark, SLAB_PANIC);
> > +       dnotify_struct_cache = KMEM_CACHE(dnotify_struct,
> > +                                         SLAB_PANIC|SLAB_ACCOUNT);
> > +       dnotify_mark_cache = KMEM_CACHE(dnotify_mark, SLAB_PANIC|SLAB_ACCOUNT);
> >
> >         dnotify_group = fsnotify_alloc_group(&dnotify_fsnotify_ops);
> >         if (IS_ERR(dnotify_group))
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index f90842efea13..c8d6e37a4855 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -154,14 +154,16 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
> >         if (fanotify_is_perm_event(mask)) {
> >                 struct fanotify_perm_event_info *pevent;
> >
> > -               pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp);
> > +               pevent = kmem_cache_alloc_memcg(fanotify_perm_event_cachep, gfp,
> > +                                               group->memcg);
> >                 if (!pevent)
> >                         return NULL;
> >                 event = &pevent->fae;
> >                 pevent->response = 0;
> >                 goto init;
> >         }
> > -       event = kmem_cache_alloc(fanotify_event_cachep, gfp);
> > +       event = kmem_cache_alloc_memcg(fanotify_event_cachep, gfp,
> > +                                      group->memcg);
> >         if (!event)
> >                 return NULL;
> >  init: __maybe_unused
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index ec4d8c59d0e3..0cf45041dc32 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/uaccess.h>
> >  #include <linux/compat.h>
> >  #include <linux/sched/signal.h>
> > +#include <linux/memcontrol.h>
> >
> >  #include <asm/ioctls.h>
> >
> > @@ -756,6 +757,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
> >
> >         group->fanotify_data.user = user;
> >         atomic_inc(&user->fanotify_listeners);
> > +       group->memcg = get_mem_cgroup_from_mm(current->mm);
> >
>
> It seem to me that you should export a wrapper to modules with
> the mem_cgroup_ prefix and not sure that need to pass current->mm
> to this wrapper.
>

Thanks, I will test with fsnotify as module.

> >         oevent = fanotify_alloc_event(group, NULL, FS_Q_OVERFLOW, NULL);
> >         if (unlikely(!oevent)) {
> > @@ -957,7 +959,8 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark,
> >   */
> >  static int __init fanotify_user_setup(void)
> >  {
> > -       fanotify_mark_cache = KMEM_CACHE(fsnotify_mark, SLAB_PANIC);
> > +       fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
> > +                                        SLAB_PANIC|SLAB_ACCOUNT);
> >         fanotify_event_cachep = KMEM_CACHE(fanotify_event_info, SLAB_PANIC);
> >         if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
> >                 fanotify_perm_event_cachep =
> > diff --git a/fs/notify/group.c b/fs/notify/group.c
> > index aa5468f23e45..300fc0f62115 100644
> > --- a/fs/notify/group.c
> > +++ b/fs/notify/group.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/srcu.h>
> >  #include <linux/rculist.h>
> >  #include <linux/wait.h>
> > +#include <linux/memcontrol.h>
> >
> >  #include <linux/fsnotify_backend.h>
> >  #include "fsnotify.h"
> > @@ -36,6 +37,11 @@ static void fsnotify_final_destroy_group(struct fsnotify_group *group)
> >         if (group->ops->free_group_priv)
> >                 group->ops->free_group_priv(group);
> >
> > +#ifdef CONFIG_MEMCG
> > +       if (group->memcg)
> > +               css_put(&group->memcg->css);
> > +#endif
> > +
>
> This looks very much like an internal implementation detail that has no
> business in an external module. I see evidence that you have used
> mem_cgroup_put() in the patch at some point and I agree that you
> need to export a pair of matching helpers to external modules.
>

Do not worry. Andrew will change this to mem_cgroup_put(). Basically
mem_cgroup_put() was defined by still-in-review patch series by Roman.
There were build failure reports where someone was taking this series
either without Roman's series or applying series out of order.

thanks,
Shakeel

[PATCH 3/3] fs, mm: account buffer_head to kmemcg

[Shakeel Butt]

The buffer_head can consume a significant amount of system memory and
is directly related to the amount of page cache. In our production
environment we have observed that a lot of machines are spending a
significant amount of memory as buffer_head and can not be left as
system memory overhead.

Charging buffer_head is not as simple as adding __GFP_ACCOUNT to the
allocation. The buffer_heads can be allocated in a memcg different from
the memcg of the page for which buffer_heads are being allocated. One
concrete example is memory reclaim. The reclaim can trigger I/O of pages
of any memcg on the system. So, the right way to charge buffer_head is
to extract the memcg from the page for which buffer_heads are being
allocated and then use targeted memcg charging API.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Greg Thelen <gthelen@google.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 fs/buffer.c                | 14 +++++++++++++-
 include/linux/memcontrol.h |  7 +++++++
 mm/memcontrol.c            | 21 +++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 8194e3049fc5..26389b7a3cab 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -815,10 +815,17 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
 	struct buffer_head *bh, *head;
 	gfp_t gfp = GFP_NOFS;
 	long offset;
+	struct mem_cgroup *old_memcg;
+	struct mem_cgroup *memcg = get_mem_cgroup_from_page(page);
 
 	if (retry)
 		gfp |= __GFP_NOFAIL;
 
+	if (memcg) {
+		gfp |= __GFP_ACCOUNT;
+		old_memcg = memalloc_memcg_save(memcg);
+	}
+
 	head = NULL;
 	offset = PAGE_SIZE;
 	while ((offset -= size) >= 0) {
@@ -835,6 +842,11 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
 		/* Link the buffer to its page */
 		set_bh_page(bh, page, offset);
 	}
+out:
+	if (memcg) {
+		memalloc_memcg_restore(old_memcg);
+#ifdef CONFIG_MEMCG
+		css_put(&memcg->css);
+#endif
+	}
 	return head;
 /*
  * In case anything failed, we just free everything we got.
@@ -848,7 +860,7 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
 		} while (head);
 	}
 
-	return NULL;
+	goto out;
 }
 EXPORT_SYMBOL_GPL(alloc_page_buffers);
 
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 6c857be8a9b7..d53609978eb7 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -380,6 +380,8 @@ struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
 
 struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm);
 
+struct mem_cgroup *get_mem_cgroup_from_page(struct page *page);
+
 static inline
 struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *css){
 	return css ? container_of(css, struct mem_cgroup, css) : NULL;
@@ -864,6 +866,11 @@ static inline struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 	return NULL;
 }
 
+static inline struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
+{
+	return NULL;
+}
+
 static inline void mem_cgroup_put(struct mem_cgroup *memcg)
 {
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c481e661e051..f9a9a79117b9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -712,6 +712,27 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 	return memcg;
 }
 
+/**
+ * get_mem_cgroup_from_page: Obtain a reference on given page's memcg.
+ * @page: page from which memcg should be extracted.
+ *
+ * Obtain a reference on page->memcg and returns it if successful. Otherwise
+ * NULL is returned.
+ */
+struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
+{
+	struct mem_cgroup *memcg = page->mem_cgroup;
+
+	if (mem_cgroup_disabled() || !memcg)
+		return NULL;
+
+	rcu_read_lock();
+	if (!css_tryget_online(&memcg->css))
+		memcg = NULL;
+	rcu_read_unlock();
+	return memcg;
+}
+
 static __always_inline struct mem_cgroup *get_mem_cgroup(
 				struct mem_cgroup *memcg, struct mm_struct *mm)
 {
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH 3/3] fs, mm: account buffer_head to kmemcg

[Johannes Weiner]

On Mon, Jun 18, 2018 at 10:13:27PM -0700, Shakeel Butt wrote:
> The buffer_head can consume a significant amount of system memory and
> is directly related to the amount of page cache. In our production
> environment we have observed that a lot of machines are spending a
> significant amount of memory as buffer_head and can not be left as
> system memory overhead.
> 
> Charging buffer_head is not as simple as adding __GFP_ACCOUNT to the
> allocation. The buffer_heads can be allocated in a memcg different from
> the memcg of the page for which buffer_heads are being allocated. One
> concrete example is memory reclaim. The reclaim can trigger I/O of pages
> of any memcg on the system. So, the right way to charge buffer_head is
> to extract the memcg from the page for which buffer_heads are being
> allocated and then use targeted memcg charging API.
> 
> Signed-off-by: Shakeel Butt <shakeelb@google.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Greg Thelen <gthelen@google.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  fs/buffer.c                | 14 +++++++++++++-
>  include/linux/memcontrol.h |  7 +++++++
>  mm/memcontrol.c            | 21 +++++++++++++++++++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 8194e3049fc5..26389b7a3cab 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -815,10 +815,17 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>  	struct buffer_head *bh, *head;
>  	gfp_t gfp = GFP_NOFS;
>  	long offset;
> +	struct mem_cgroup *old_memcg;
> +	struct mem_cgroup *memcg = get_mem_cgroup_from_page(page);
>  
>  	if (retry)
>  		gfp |= __GFP_NOFAIL;
>  
> +	if (memcg) {
> +		gfp |= __GFP_ACCOUNT;
> +		old_memcg = memalloc_memcg_save(memcg);
> +	}

Please move the get_mem_cgroup_from_page() call out of the
declarations and down to right before the if (memcg) branch.

>  	head = NULL;
>  	offset = PAGE_SIZE;
>  	while ((offset -= size) >= 0) {
> @@ -835,6 +842,11 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>  		/* Link the buffer to its page */
>  		set_bh_page(bh, page, offset);
>  	}
> +out:
> +	if (memcg) {
> +		memalloc_memcg_restore(old_memcg);
> +#ifdef CONFIG_MEMCG
> +		css_put(&memcg->css);
> +#endif

Please add a put_mem_cgroup() ;)

Otherwise this looks fine to me.

Re: [PATCH 3/3] fs, mm: account buffer_head to kmemcg

[Roman Gushchin]

On Tue, Jun 19, 2018 at 12:27:41PM -0400, Johannes Weiner wrote:
> On Mon, Jun 18, 2018 at 10:13:27PM -0700, Shakeel Butt wrote:
> > The buffer_head can consume a significant amount of system memory and
> > is directly related to the amount of page cache. In our production
> > environment we have observed that a lot of machines are spending a
> > significant amount of memory as buffer_head and can not be left as
> > system memory overhead.
> > 
> > Charging buffer_head is not as simple as adding __GFP_ACCOUNT to the
> > allocation. The buffer_heads can be allocated in a memcg different from
> > the memcg of the page for which buffer_heads are being allocated. One
> > concrete example is memory reclaim. The reclaim can trigger I/O of pages
> > of any memcg on the system. So, the right way to charge buffer_head is
> > to extract the memcg from the page for which buffer_heads are being
> > allocated and then use targeted memcg charging API.
> > 
> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Greg Thelen <gthelen@google.com>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  fs/buffer.c                | 14 +++++++++++++-
> >  include/linux/memcontrol.h |  7 +++++++
> >  mm/memcontrol.c            | 21 +++++++++++++++++++++
> >  3 files changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 8194e3049fc5..26389b7a3cab 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -815,10 +815,17 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> >  	struct buffer_head *bh, *head;
> >  	gfp_t gfp = GFP_NOFS;
> >  	long offset;
> > +	struct mem_cgroup *old_memcg;
> > +	struct mem_cgroup *memcg = get_mem_cgroup_from_page(page);
> >  
> >  	if (retry)
> >  		gfp |= __GFP_NOFAIL;
> >  
> > +	if (memcg) {
> > +		gfp |= __GFP_ACCOUNT;
> > +		old_memcg = memalloc_memcg_save(memcg);
> > +	}
> 
> Please move the get_mem_cgroup_from_page() call out of the
> declarations and down to right before the if (memcg) branch.
> 
> >  	head = NULL;
> >  	offset = PAGE_SIZE;
> >  	while ((offset -= size) >= 0) {
> > @@ -835,6 +842,11 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> >  		/* Link the buffer to its page */
> >  		set_bh_page(bh, page, offset);
> >  	}
> > +out:
> > +	if (memcg) {
> > +		memalloc_memcg_restore(old_memcg);
> > +#ifdef CONFIG_MEMCG
> > +		css_put(&memcg->css);
> > +#endif
> 
> Please add a put_mem_cgroup() ;)

I've added such helper by commit 8a34a8b7fd62 ("mm, oom: cgroup-aware OOM killer").
It's in the mm tree.

Thanks!

Re: [PATCH 3/3] fs, mm: account buffer_head to kmemcg

[Shakeel Butt]

On Tue, Jun 19, 2018 at 10:41 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Tue, Jun 19, 2018 at 12:27:41PM -0400, Johannes Weiner wrote:
> > On Mon, Jun 18, 2018 at 10:13:27PM -0700, Shakeel Butt wrote:
> > > The buffer_head can consume a significant amount of system memory and
> > > is directly related to the amount of page cache. In our production
> > > environment we have observed that a lot of machines are spending a
> > > significant amount of memory as buffer_head and can not be left as
> > > system memory overhead.
> > >
> > > Charging buffer_head is not as simple as adding __GFP_ACCOUNT to the
> > > allocation. The buffer_heads can be allocated in a memcg different from
> > > the memcg of the page for which buffer_heads are being allocated. One
> > > concrete example is memory reclaim. The reclaim can trigger I/O of pages
> > > of any memcg on the system. So, the right way to charge buffer_head is
> > > to extract the memcg from the page for which buffer_heads are being
> > > allocated and then use targeted memcg charging API.
> > >
> > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Cc: Greg Thelen <gthelen@google.com>
> > > Cc: Michal Hocko <mhocko@kernel.org>
> > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > ---
> > >  fs/buffer.c                | 14 +++++++++++++-
> > >  include/linux/memcontrol.h |  7 +++++++
> > >  mm/memcontrol.c            | 21 +++++++++++++++++++++
> > >  3 files changed, 41 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/buffer.c b/fs/buffer.c
> > > index 8194e3049fc5..26389b7a3cab 100644
> > > --- a/fs/buffer.c
> > > +++ b/fs/buffer.c
> > > @@ -815,10 +815,17 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> > >     struct buffer_head *bh, *head;
> > >     gfp_t gfp = GFP_NOFS;
> > >     long offset;
> > > +   struct mem_cgroup *old_memcg;
> > > +   struct mem_cgroup *memcg = get_mem_cgroup_from_page(page);
> > >
> > >     if (retry)
> > >             gfp |= __GFP_NOFAIL;
> > >
> > > +   if (memcg) {
> > > +           gfp |= __GFP_ACCOUNT;
> > > +           old_memcg = memalloc_memcg_save(memcg);
> > > +   }
> >
> > Please move the get_mem_cgroup_from_page() call out of the
> > declarations and down to right before the if (memcg) branch.
> >
> > >     head = NULL;
> > >     offset = PAGE_SIZE;
> > >     while ((offset -= size) >= 0) {
> > > @@ -835,6 +842,11 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> > >             /* Link the buffer to its page */
> > >             set_bh_page(bh, page, offset);
> > >     }
> > > +out:
> > > +   if (memcg) {
> > > +           memalloc_memcg_restore(old_memcg);
> > > +#ifdef CONFIG_MEMCG
> > > +           css_put(&memcg->css);
> > > +#endif
> >
> > Please add a put_mem_cgroup() ;)
>
> I've added such helper by commit 8a34a8b7fd62 ("mm, oom: cgroup-aware OOM killer").
> It's in the mm tree.
>

I was using mem_cgroup_put() defined by Roman's patch but there were a
lot of build failure reports where someone was taking this series
without Roman's series or applying the series out of order. Andrew
asked me to keep it like this and then he will convert these callsites
into mem_cgroup_put() after making making sure Roman's series is
applied in mm tree. I will recheck with him, how he wants to handle it
now.

thanks,
Shakeel

Re: [PATCH 3/3] fs, mm: account buffer_head to kmemcg

[Roman Gushchin]

On Tue, Jun 19, 2018 at 12:51:15PM -0700, Shakeel Butt wrote:
> On Tue, Jun 19, 2018 at 10:41 AM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Tue, Jun 19, 2018 at 12:27:41PM -0400, Johannes Weiner wrote:
> > > On Mon, Jun 18, 2018 at 10:13:27PM -0700, Shakeel Butt wrote:
> > > > The buffer_head can consume a significant amount of system memory and
> > > > is directly related to the amount of page cache. In our production
> > > > environment we have observed that a lot of machines are spending a
> > > > significant amount of memory as buffer_head and can not be left as
> > > > system memory overhead.
> > > >
> > > > Charging buffer_head is not as simple as adding __GFP_ACCOUNT to the
> > > > allocation. The buffer_heads can be allocated in a memcg different from
> > > > the memcg of the page for which buffer_heads are being allocated. One
> > > > concrete example is memory reclaim. The reclaim can trigger I/O of pages
> > > > of any memcg on the system. So, the right way to charge buffer_head is
> > > > to extract the memcg from the page for which buffer_heads are being
> > > > allocated and then use targeted memcg charging API.
> > > >
> > > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > > Cc: Jan Kara <jack@suse.cz>
> > > > Cc: Greg Thelen <gthelen@google.com>
> > > > Cc: Michal Hocko <mhocko@kernel.org>
> > > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > ---
> > > >  fs/buffer.c                | 14 +++++++++++++-
> > > >  include/linux/memcontrol.h |  7 +++++++
> > > >  mm/memcontrol.c            | 21 +++++++++++++++++++++
> > > >  3 files changed, 41 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/buffer.c b/fs/buffer.c
> > > > index 8194e3049fc5..26389b7a3cab 100644
> > > > --- a/fs/buffer.c
> > > > +++ b/fs/buffer.c
> > > > @@ -815,10 +815,17 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> > > >     struct buffer_head *bh, *head;
> > > >     gfp_t gfp = GFP_NOFS;
> > > >     long offset;
> > > > +   struct mem_cgroup *old_memcg;
> > > > +   struct mem_cgroup *memcg = get_mem_cgroup_from_page(page);
> > > >
> > > >     if (retry)
> > > >             gfp |= __GFP_NOFAIL;
> > > >
> > > > +   if (memcg) {
> > > > +           gfp |= __GFP_ACCOUNT;
> > > > +           old_memcg = memalloc_memcg_save(memcg);
> > > > +   }
> > >
> > > Please move the get_mem_cgroup_from_page() call out of the
> > > declarations and down to right before the if (memcg) branch.
> > >
> > > >     head = NULL;
> > > >     offset = PAGE_SIZE;
> > > >     while ((offset -= size) >= 0) {
> > > > @@ -835,6 +842,11 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> > > >             /* Link the buffer to its page */
> > > >             set_bh_page(bh, page, offset);
> > > >     }
> > > > +out:
> > > > +   if (memcg) {
> > > > +           memalloc_memcg_restore(old_memcg);
> > > > +#ifdef CONFIG_MEMCG
> > > > +           css_put(&memcg->css);
> > > > +#endif
> > >
> > > Please add a put_mem_cgroup() ;)
> >
> > I've added such helper by commit 8a34a8b7fd62 ("mm, oom: cgroup-aware OOM killer").
> > It's in the mm tree.
> >
> 
> I was using mem_cgroup_put() defined by Roman's patch but there were a
> lot of build failure reports where someone was taking this series
> without Roman's series or applying the series out of order. Andrew
> asked me to keep it like this and then he will convert these callsites
> into mem_cgroup_put() after making making sure Roman's series is
> applied in mm tree. I will recheck with him, how he wants to handle it
> now.

I can also split the introduction of mem_cgroup_put() into a separate commit,
as it seems to be usable not only by the cgroup oom stuff.
Please, let me know, if it's a preferred way to go.

Thanks!

Re: [PATCH 3/3] fs, mm: account buffer_head to kmemcg

[Shakeel Butt]

On Tue, Jun 19, 2018 at 12:55 PM Roman Gushchin <guro@fb.com> wrote:
>
> On Tue, Jun 19, 2018 at 12:51:15PM -0700, Shakeel Butt wrote:
> > On Tue, Jun 19, 2018 at 10:41 AM Roman Gushchin <guro@fb.com> wrote:
> > >
> > > On Tue, Jun 19, 2018 at 12:27:41PM -0400, Johannes Weiner wrote:
> > > > On Mon, Jun 18, 2018 at 10:13:27PM -0700, Shakeel Butt wrote:
> > > > > The buffer_head can consume a significant amount of system memory and
> > > > > is directly related to the amount of page cache. In our production
> > > > > environment we have observed that a lot of machines are spending a
> > > > > significant amount of memory as buffer_head and can not be left as
> > > > > system memory overhead.
> > > > >
> > > > > Charging buffer_head is not as simple as adding __GFP_ACCOUNT to the
> > > > > allocation. The buffer_heads can be allocated in a memcg different from
> > > > > the memcg of the page for which buffer_heads are being allocated. One
> > > > > concrete example is memory reclaim. The reclaim can trigger I/O of pages
> > > > > of any memcg on the system. So, the right way to charge buffer_head is
> > > > > to extract the memcg from the page for which buffer_heads are being
> > > > > allocated and then use targeted memcg charging API.
> > > > >
> > > > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > > > Cc: Jan Kara <jack@suse.cz>
> > > > > Cc: Greg Thelen <gthelen@google.com>
> > > > > Cc: Michal Hocko <mhocko@kernel.org>
> > > > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > > > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > ---
> > > > >  fs/buffer.c                | 14 +++++++++++++-
> > > > >  include/linux/memcontrol.h |  7 +++++++
> > > > >  mm/memcontrol.c            | 21 +++++++++++++++++++++
> > > > >  3 files changed, 41 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/buffer.c b/fs/buffer.c
> > > > > index 8194e3049fc5..26389b7a3cab 100644
> > > > > --- a/fs/buffer.c
> > > > > +++ b/fs/buffer.c
> > > > > @@ -815,10 +815,17 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> > > > >     struct buffer_head *bh, *head;
> > > > >     gfp_t gfp = GFP_NOFS;
> > > > >     long offset;
> > > > > +   struct mem_cgroup *old_memcg;
> > > > > +   struct mem_cgroup *memcg = get_mem_cgroup_from_page(page);
> > > > >
> > > > >     if (retry)
> > > > >             gfp |= __GFP_NOFAIL;
> > > > >
> > > > > +   if (memcg) {
> > > > > +           gfp |= __GFP_ACCOUNT;
> > > > > +           old_memcg = memalloc_memcg_save(memcg);
> > > > > +   }
> > > >
> > > > Please move the get_mem_cgroup_from_page() call out of the
> > > > declarations and down to right before the if (memcg) branch.
> > > >
> > > > >     head = NULL;
> > > > >     offset = PAGE_SIZE;
> > > > >     while ((offset -= size) >= 0) {
> > > > > @@ -835,6 +842,11 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> > > > >             /* Link the buffer to its page */
> > > > >             set_bh_page(bh, page, offset);
> > > > >     }
> > > > > +out:
> > > > > +   if (memcg) {
> > > > > +           memalloc_memcg_restore(old_memcg);
> > > > > +#ifdef CONFIG_MEMCG
> > > > > +           css_put(&memcg->css);
> > > > > +#endif
> > > >
> > > > Please add a put_mem_cgroup() ;)
> > >
> > > I've added such helper by commit 8a34a8b7fd62 ("mm, oom: cgroup-aware OOM killer").
> > > It's in the mm tree.
> > >
> >
> > I was using mem_cgroup_put() defined by Roman's patch but there were a
> > lot of build failure reports where someone was taking this series
> > without Roman's series or applying the series out of order. Andrew
> > asked me to keep it like this and then he will convert these callsites
> > into mem_cgroup_put() after making making sure Roman's series is
> > applied in mm tree. I will recheck with him, how he wants to handle it
> > now.
>
> I can also split the introduction of mem_cgroup_put() into a separate commit,
> as it seems to be usable not only by the cgroup oom stuff.
> Please, let me know, if it's a preferred way to go.
>

Oh I forgot to reply. Yes, let's do that, a separate patch to
introduce mem_cgroup_put() which can used by remote charging and memcg
aware oom-killer patches.

Shakeel

Re: [PATCH v6 0/3] Directed kmem charging

[Johannes Weiner]

Hi Shakeel,

this looks generally reasonable to me.

However, patch 1 introduces API that isn't used until patch 2 and 3,
which makes reviewing harder since you have to jump back and forth
between emails. Please fold patch 1 and introduce API along with the
users.

On Mon, Jun 18, 2018 at 10:13:24PM -0700, Shakeel Butt wrote:
> This patchset introduces memcg variant memory allocation functions.  The
> caller can explicitly pass the memcg to charge for kmem allocations.
> Currently the kernel, for __GFP_ACCOUNT memory allocation requests,
> extract the memcg of the current task to charge for the kmem allocation.
> This patch series introduces kmem allocation functions where the caller
> can pass the pointer to the remote memcg.  The remote memcg will be
> charged for the allocation instead of the memcg of the caller.  However
> the caller must have a reference to the remote memcg.  This patch series
> also introduces scope API for targeted memcg charging. So, all the
> __GFP_ACCOUNT alloctions within the specified scope will be charged to
> the given target memcg.

Can you open with the rationale for the series, i.e. the problem
statement (fsnotify and bh memory footprint), *then* follow with the
proposed solution?

Thanks!

Re: [PATCH v6 0/3] Directed kmem charging

[Shakeel Butt]

On Tue, Jun 19, 2018 at 9:09 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> Hi Shakeel,
>
> this looks generally reasonable to me.
>
> However, patch 1 introduces API that isn't used until patch 2 and 3,
> which makes reviewing harder since you have to jump back and forth
> between emails. Please fold patch 1 and introduce API along with the
> users.
>

Thanks a lot for the review. Ack, I will do as you suggested in next version.

> On Mon, Jun 18, 2018 at 10:13:24PM -0700, Shakeel Butt wrote:
> > This patchset introduces memcg variant memory allocation functions.  The
> > caller can explicitly pass the memcg to charge for kmem allocations.
> > Currently the kernel, for __GFP_ACCOUNT memory allocation requests,
> > extract the memcg of the current task to charge for the kmem allocation.
> > This patch series introduces kmem allocation functions where the caller
> > can pass the pointer to the remote memcg.  The remote memcg will be
> > charged for the allocation instead of the memcg of the caller.  However
> > the caller must have a reference to the remote memcg.  This patch series
> > also introduces scope API for targeted memcg charging. So, all the
> > __GFP_ACCOUNT alloctions within the specified scope will be charged to
> > the given target memcg.
>
> Can you open with the rationale for the series, i.e. the problem
> statement (fsnotify and bh memory footprint), *then* follow with the
> proposed solution?
>

Sure.

thanks,
Shakeel