Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1184694 > unrolled thread

[PATCH 0/5] expose mem_cgroup + cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] expose mem_cgroup + cleanups Michal Hocko <mhocko@kernel.org> - 2015-07-15 13:20 +0200
    [PATCH 3/5] memcg: get rid of extern for functions in memcontrol.h Michal Hocko <mhocko@kernel.org> - 2015-07-15 13:20 +0200
    [PATCH 2/5] memcg: get rid of mem_cgroup_root_css for !CONFIG_MEMCG Michal Hocko <mhocko@kernel.org> - 2015-07-15 13:20 +0200
    [PATCH 5/5] memcg, tcp_kmem: check for cg_proto in sock_update_memcg Michal Hocko <mhocko@kernel.org> - 2015-07-15 13:20 +0200
    Re: [PATCH 1/5] memcg: export struct mem_cgroup Andrew Morton <akpm@linux-foundation.org> - 2015-07-15 23:00 +0200
      Re: [PATCH 1/5] memcg: export struct mem_cgroup Andrew Morton <akpm@linux-foundation.org> - 2015-07-16 23:40 +0200
        Re: [PATCH 1/5] memcg: export struct mem_cgroup Johannes Weiner <hannes@cmpxchg.org> - 2015-07-17 01:00 +0200
          Re: [PATCH 1/5] memcg: export struct mem_cgroup Andrew Morton <akpm@linux-foundation.org> - 2015-07-17 01:10 +0200
            Re: [PATCH 1/5] memcg: export struct mem_cgroup Johannes Weiner <hannes@cmpxchg.org> - 2015-07-17 14:30 +0200

#1184694 — [PATCH 0/5] expose mem_cgroup + cleanups

Hi,
this is just the cleanup portion of the series posted previously here:
http://marc.info/?l=linux-kernel&m=143635871831843. I am still thinking
about how to move on regarding mm_struct::owner but this is more tricky
than originally anticipated. The cleanup still makes some sense IMO.

I have incorporated review feedback from Vladimir (thanks!) and dropped
http://marc.info/?l=linux-mm&m=143635849631722&w=2 because Vladimir
didn't like it.

Diffstat:
 include/linux/memcontrol.h | 387 ++++++++++++++++++++++++++++++++++++++++-----
 include/linux/swap.h       |  10 +-
 include/net/sock.h         |  28 ----
 mm/memcontrol.c            | 378 ++++---------------------------------------
 mm/memory-failure.c        |   2 +-
 mm/slab_common.c           |   2 +-
 mm/vmscan.c                |   2 +-
 7 files changed, 390 insertions(+), 419 deletions(-)

Shortlog:
Michal Hocko (4):
      memcg: export struct mem_cgroup
      memcg: get rid of mem_cgroup_root_css for !CONFIG_MEMCG
      memcg: get rid of extern for functions in memcontrol.h
      memcg, tcp_kmem: check for cg_proto in sock_update_memcg

Tejun Heo (1):
      memcg: restructure mem_cgroup_can_attach()

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1184695 — [PATCH 3/5] memcg: get rid of extern for functions in memcontrol.h

From: Michal Hocko <mhocko@suse.cz>

Most of the exported functions in this header are not marked extern so
change the rest to follow the same style.

Reviewed-by: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 include/linux/memcontrol.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 292e6701f3fd..ce85bdb556f5 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -305,10 +305,10 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *, struct zone *);
 
 bool task_in_mem_cgroup(struct task_struct *task, struct mem_cgroup *memcg);
 
-extern struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page);
-extern struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
+struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page);
+struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
 
-extern struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *memcg);
+struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *memcg);
 static inline
 struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *css){
 	return css ? container_of(css, struct mem_cgroup, css) : NULL;
@@ -343,7 +343,7 @@ static inline bool mm_match_cgroup(struct mm_struct *mm,
 	return match;
 }
 
-extern struct cgroup_subsys_state *mem_cgroup_css_from_page(struct page *page);
+struct cgroup_subsys_state *mem_cgroup_css_from_page(struct page *page);
 
 static inline bool mem_cgroup_disabled(void)
 {
@@ -402,8 +402,8 @@ static inline int mem_cgroup_inactive_anon_is_low(struct lruvec *lruvec)
 	return inactive * inactive_ratio < active;
 }
 
-extern void mem_cgroup_print_oom_info(struct mem_cgroup *memcg,
-					struct task_struct *p);
+void mem_cgroup_print_oom_info(struct mem_cgroup *memcg,
+				struct task_struct *p);
 
 static inline void mem_cgroup_oom_enable(void)
 {
@@ -718,8 +718,8 @@ static inline void sock_release_memcg(struct sock *sk)
 extern struct static_key memcg_kmem_enabled_key;
 
 extern int memcg_nr_cache_ids;
-extern void memcg_get_cache_ids(void);
-extern void memcg_put_cache_ids(void);
+void memcg_get_cache_ids(void);
+void memcg_put_cache_ids(void);
 
 /*
  * Helper macro to loop through all memcg-specific caches. Callers must still
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1184696 — [PATCH 2/5] memcg: get rid of mem_cgroup_root_css for !CONFIG_MEMCG

The only user is cgwb_bdi_init and that one depends on
CONFIG_CGROUP_WRITEBACK which in turn depends on CONFIG_MEMCG
so it doesn't make much sense to definte an empty stub for
!CONFIG_MEMCG. Moreover ERR_PTR(-EINVAL) is ugly and would lead
to runtime crashes if used in unguarded code paths. Better fail
during compilation.

Reviewed-by: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Michal Hocko <mhocko@kernel.org>
---
 include/linux/memcontrol.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 42f118ae04cf..292e6701f3fd 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -497,8 +497,6 @@ void mem_cgroup_split_huge_fixup(struct page *head);
 #else /* CONFIG_MEMCG */
 struct mem_cgroup;
 
-#define mem_cgroup_root_css ((struct cgroup_subsys_state *)ERR_PTR(-EINVAL))
-
 static inline void mem_cgroup_events(struct mem_cgroup *memcg,
 				     enum mem_cgroup_events_index idx,
 				     unsigned int nr)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1184699 — [PATCH 5/5] memcg, tcp_kmem: check for cg_proto in sock_update_memcg

From: Michal Hocko <mhocko@suse.cz>

sk_prot->proto_cgroup is allowed to return NULL but sock_update_memcg
doesn't check for NULL. The function relies on the mem_cgroup_is_root
check because we shouldn't get NULL otherwise because
mem_cgroup_from_task will always return !NULL.

All other callers are checking for NULL and we can safely replace
mem_cgroup_is_root() check by cg_proto != NULL which will be more
straightforward (proto_cgroup returns NULL for the root memcg already).

Reviewed-by: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 5d4fba8cbdd0..cf9fb1f41831 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -315,8 +315,7 @@ void sock_update_memcg(struct sock *sk)
 		rcu_read_lock();
 		memcg = mem_cgroup_from_task(current);
 		cg_proto = sk->sk_prot->proto_cgroup(memcg);
-		if (!mem_cgroup_is_root(memcg) &&
-		    memcg_proto_active(cg_proto) &&
+		if (cg_proto && memcg_proto_active(cg_proto) &&
 		    css_tryget_online(&memcg->css)) {
 			sk->sk_cgrp = cg_proto;
 		}
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1185041 — Re: [PATCH 1/5] memcg: export struct mem_cgroup

On Wed, 15 Jul 2015 13:14:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> mem_cgroup structure is defined in mm/memcontrol.c currently which
> means that the code outside of this file has to use external API even
> for trivial access stuff.
> 
> This patch exports mm_struct with its dependencies and makes some of the
> exported functions inlines. This even helps to reduce the code size a bit
> (make defconfig + CONFIG_MEMCG=y)
> 
> text		data    bss     dec     	 hex 	filename
> 12355346        1823792 1089536 15268674         e8fb42 vmlinux.before
> 12354970        1823792 1089536 15268298         e8f9ca vmlinux.after
> 
> This is not much (370B) but better than nothing. We also save a function
> call in some hot paths like callers of mem_cgroup_count_vm_event which is
> used for accounting.
> 
> The patch doesn't introduce any functional changes.
> 
> ...
>
>  include/linux/memcontrol.h | 369 +++++++++++++++++++++++++++++++++++++++++----

Boy, that's a ton of new stuff into the header file.  Do we actually
*need* to expose all this?  Is some other patch dependent on it?  If
not then perhaps we shouldn't do this - if the code was already this
way, I'd be attracted to a patch which was the reverse of this one!

There's some risk of build breakage here - just from a quick scan,
memcontrol.h is going to need eventfd.h for eventfd_ctx.  But what else
is needed?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186275 — Re: [PATCH 1/5] memcg: export struct mem_cgroup

On Thu, 16 Jul 2015 09:19:49 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> On Wed 15-07-15 13:57:11, Andrew Morton wrote:
> > On Wed, 15 Jul 2015 13:14:41 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > 
> > > mem_cgroup structure is defined in mm/memcontrol.c currently which
> > > means that the code outside of this file has to use external API even
> > > for trivial access stuff.
> > > 
> > > This patch exports mm_struct with its dependencies and makes some of the
> > > exported functions inlines. This even helps to reduce the code size a bit
> > > (make defconfig + CONFIG_MEMCG=y)
> > > 
> > > text		data    bss     dec     	 hex 	filename
> > > 12355346        1823792 1089536 15268674         e8fb42 vmlinux.before
> > > 12354970        1823792 1089536 15268298         e8f9ca vmlinux.after
> > > 
> > > This is not much (370B) but better than nothing. We also save a function
> > > call in some hot paths like callers of mem_cgroup_count_vm_event which is
> > > used for accounting.
> > > 
> > > The patch doesn't introduce any functional changes.
> > > 
> > > ...
> > >
> > >  include/linux/memcontrol.h | 369 +++++++++++++++++++++++++++++++++++++++++----
> > 
> > Boy, that's a ton of new stuff into the header file.  Do we actually
> > *need* to expose all this?
> 
> I am exporting struct mem_cgroup with its dependencies + some small
> functions which allow to inline some really trivial code and helps to
> generate a better code.
> 
> > Is some other patch dependent on it? 
> 
> Without mem_cgroup visible outside of memcontrol.c we couldn't inline
> and now we can also use some fields from mem_cgroup directly and get rid
> of some really trivial access functions.
> 
> > If
> > not then perhaps we shouldn't do this - if the code was already this
> > way, I'd be attracted to a patch which was the reverse of this one!
> 
> I agree with Johannes who originally suggested to expose mem_cgroup that
> it will allow for a better code later.

Sure, but how *much* better?  Are there a significant number of
fastpath functions involved?

From a maintainability/readability point of view, this is quite a bad
patch.  It exposes a *lot* of stuff to the whole world.  We need to get
a pretty good runtime benefit from doing this to ourselves.  I don't
think that saving 376 bytes on a fatconfig build is sufficient
justification?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186307 — Re: [PATCH 1/5] memcg: export struct mem_cgroup

On Thu, Jul 16, 2015 at 02:34:33PM -0700, Andrew Morton wrote:
> On Thu, 16 Jul 2015 09:19:49 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > I agree with Johannes who originally suggested to expose mem_cgroup that
> > it will allow for a better code later.
> 
> Sure, but how *much* better?  Are there a significant number of
> fastpath functions involved?
> 
> From a maintainability/readability point of view, this is quite a bad
> patch.  It exposes a *lot* of stuff to the whole world.  We need to get
> a pretty good runtime benefit from doing this to ourselves.  I don't
> think that saving 376 bytes on a fatconfig build is sufficient
> justification?

It's not a performance issue for me.  Some stuff is hard to read when
you have memcg functions with klunky names interrupting the code flow
to do something trivial to a struct mem_cgroup member, like
mem_cgroup_lruvec_online() and mem_cgroup_get_lru_size().

Maybe we can keep thresholds private and encapsulate the softlimit
tree stuff in mem_cgroup_per_zone into something private as well, as
this is not used - and unlikely to be used - outside of memcg proper.

But otherwise, I think struct mem_cgroup should have mm-scope.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186311 — Re: [PATCH 1/5] memcg: export struct mem_cgroup

On Thu, 16 Jul 2015 18:56:39 -0400 Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Thu, Jul 16, 2015 at 02:34:33PM -0700, Andrew Morton wrote:
> > On Thu, 16 Jul 2015 09:19:49 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > > I agree with Johannes who originally suggested to expose mem_cgroup that
> > > it will allow for a better code later.
> > 
> > Sure, but how *much* better?  Are there a significant number of
> > fastpath functions involved?
> > 
> > From a maintainability/readability point of view, this is quite a bad
> > patch.  It exposes a *lot* of stuff to the whole world.  We need to get
> > a pretty good runtime benefit from doing this to ourselves.  I don't
> > think that saving 376 bytes on a fatconfig build is sufficient
> > justification?
> 
> It's not a performance issue for me.  Some stuff is hard to read when
> you have memcg functions with klunky names interrupting the code flow
> to do something trivial to a struct mem_cgroup member, like
> mem_cgroup_lruvec_online() and mem_cgroup_get_lru_size().
> 
> Maybe we can keep thresholds private and encapsulate the softlimit
> tree stuff in mem_cgroup_per_zone into something private as well, as
> this is not used - and unlikely to be used - outside of memcg proper.
> 
> But otherwise, I think struct mem_cgroup should have mm-scope.

Meaning a new mm/memcontrol.h?  That's a bit better I suppose.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186780 — Re: [PATCH 1/5] memcg: export struct mem_cgroup

On Thu, Jul 16, 2015 at 04:03:58PM -0700, Andrew Morton wrote:
> On Thu, 16 Jul 2015 18:56:39 -0400 Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > On Thu, Jul 16, 2015 at 02:34:33PM -0700, Andrew Morton wrote:
> > > On Thu, 16 Jul 2015 09:19:49 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > > > I agree with Johannes who originally suggested to expose mem_cgroup that
> > > > it will allow for a better code later.
> > > 
> > > Sure, but how *much* better?  Are there a significant number of
> > > fastpath functions involved?
> > > 
> > > From a maintainability/readability point of view, this is quite a bad
> > > patch.  It exposes a *lot* of stuff to the whole world.  We need to get
> > > a pretty good runtime benefit from doing this to ourselves.  I don't
> > > think that saving 376 bytes on a fatconfig build is sufficient
> > > justification?
> > 
> > It's not a performance issue for me.  Some stuff is hard to read when
> > you have memcg functions with klunky names interrupting the code flow
> > to do something trivial to a struct mem_cgroup member, like
> > mem_cgroup_lruvec_online() and mem_cgroup_get_lru_size().
> > 
> > Maybe we can keep thresholds private and encapsulate the softlimit
> > tree stuff in mem_cgroup_per_zone into something private as well, as
> > this is not used - and unlikely to be used - outside of memcg proper.
> > 
> > But otherwise, I think struct mem_cgroup should have mm-scope.
> 
> Meaning a new mm/memcontrol.h?  That's a bit better I suppose.

I meant as opposed to being private to memcontrol.c.  I'm not sure I
quite see the problem of having these definitions in include/linux, as
long as we keep the stuff that is genuinely only used in memcontrol.c
private to that file.  But mm/memcontrol.h would probably work too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web