All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hugh Dickins <hughd@google.com>
To: Michal Hocko <mhocko@suse.cz>, Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Greg Thelen <gthelen@google.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: [PATCH 2/2] memcg: barriers to see memcgs as fully initialized
Date: Wed, 12 Feb 2014 17:29:09 -0800 (PST)	[thread overview]
Message-ID: <alpine.LSU.2.11.1402121727050.5917@eggly.anvils> (raw)
In-Reply-To: <alpine.LSU.2.11.1402121717420.5917@eggly.anvils>

Commit d8ad30559715 ("mm/memcg: iteration skip memcgs not yet fully
initialized") is not bad, but Greg Thelen asks "Are barriers needed?"

Yes, I'm afraid so: this makes it a little heavier than the original,
but there's no point in guaranteeing that mem_cgroup_iter() returns only
fully initialized memcgs, if we don't guarantee that the initialization
is visible.

If we move online_css()'s setting CSS_ONLINE after rcu_assign_pointer()
(I don't see why not), we can reasonably rely on the smp_wmb() in that.
But I can't find a pre-existing barrier at the mem_cgroup_iter() end,
so add an smp_rmb() where __mem_cgroup_iter_next() returns non-NULL.

Fixes: d8ad30559715 ("mm/memcg: iteration skip memcgs not yet fully initialized")
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org # 3.12+
---
I'd have been happier not to have to add this patch: maybe you can see
a better placement, or a way we can avoid this altogether.

 kernel/cgroup.c |    8 +++++++-
 mm/memcontrol.c |   11 +++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

--- 3.14-rc2+/kernel/cgroup.c	2014-02-02 18:49:07.737302111 -0800
+++ linux/kernel/cgroup.c	2014-02-12 11:59:52.804041895 -0800
@@ -4063,9 +4063,15 @@ static int online_css(struct cgroup_subs
 	if (ss->css_online)
 		ret = ss->css_online(css);
 	if (!ret) {
-		css->flags |= CSS_ONLINE;
 		css->cgroup->nr_css++;
 		rcu_assign_pointer(css->cgroup->subsys[ss->subsys_id], css);
+		/*
+		 * Set CSS_ONLINE after rcu_assign_pointer(), so that its
+		 * smp_wmb() will guarantee that those seeing CSS_ONLINE
+		 * can see the initialization done in ss->css_online() - if
+		 * they provide an smp_rmb(), as in __mem_cgroup_iter_next().
+		 */
+		css->flags |= CSS_ONLINE;
 	}
 	return ret;
 }
--- 3.14-rc2+/mm/memcontrol.c	2014-02-12 11:55:02.836035004 -0800
+++ linux/mm/memcontrol.c	2014-02-12 11:59:52.804041895 -0800
@@ -1128,9 +1128,16 @@ skip_node:
 	 */
 	if (next_css) {
 		if ((next_css == &root->css) ||
-		    ((next_css->flags & CSS_ONLINE) && css_tryget(next_css)))
+		    ((next_css->flags & CSS_ONLINE) && css_tryget(next_css))) {
+			/*
+			 * Ensure that all memcg initialization, done before
+			 * CSS_ONLINE was set, will be visible to our caller.
+			 * This matches the smp_wmb() in online_css()'s
+			 * rcu_assign_pointer(), before it set CSS_ONLINE.
+			 */
+			smp_rmb();
 			return mem_cgroup_from_css(next_css);
-
+		}
 		prev_css = next_css;
 		goto skip_node;
 	}

WARNING: multiple messages have this Message-ID (diff)
From: Hugh Dickins <hughd@google.com>
To: Michal Hocko <mhocko@suse.cz>, Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Greg Thelen <gthelen@google.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: [PATCH 2/2] memcg: barriers to see memcgs as fully initialized
Date: Wed, 12 Feb 2014 17:29:09 -0800 (PST)	[thread overview]
Message-ID: <alpine.LSU.2.11.1402121727050.5917@eggly.anvils> (raw)
In-Reply-To: <alpine.LSU.2.11.1402121717420.5917@eggly.anvils>

Commit d8ad30559715 ("mm/memcg: iteration skip memcgs not yet fully
initialized") is not bad, but Greg Thelen asks "Are barriers needed?"

Yes, I'm afraid so: this makes it a little heavier than the original,
but there's no point in guaranteeing that mem_cgroup_iter() returns only
fully initialized memcgs, if we don't guarantee that the initialization
is visible.

If we move online_css()'s setting CSS_ONLINE after rcu_assign_pointer()
(I don't see why not), we can reasonably rely on the smp_wmb() in that.
But I can't find a pre-existing barrier at the mem_cgroup_iter() end,
so add an smp_rmb() where __mem_cgroup_iter_next() returns non-NULL.

Fixes: d8ad30559715 ("mm/memcg: iteration skip memcgs not yet fully initialized")
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org # 3.12+
---
I'd have been happier not to have to add this patch: maybe you can see
a better placement, or a way we can avoid this altogether.

 kernel/cgroup.c |    8 +++++++-
 mm/memcontrol.c |   11 +++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

--- 3.14-rc2+/kernel/cgroup.c	2014-02-02 18:49:07.737302111 -0800
+++ linux/kernel/cgroup.c	2014-02-12 11:59:52.804041895 -0800
@@ -4063,9 +4063,15 @@ static int online_css(struct cgroup_subs
 	if (ss->css_online)
 		ret = ss->css_online(css);
 	if (!ret) {
-		css->flags |= CSS_ONLINE;
 		css->cgroup->nr_css++;
 		rcu_assign_pointer(css->cgroup->subsys[ss->subsys_id], css);
+		/*
+		 * Set CSS_ONLINE after rcu_assign_pointer(), so that its
+		 * smp_wmb() will guarantee that those seeing CSS_ONLINE
+		 * can see the initialization done in ss->css_online() - if
+		 * they provide an smp_rmb(), as in __mem_cgroup_iter_next().
+		 */
+		css->flags |= CSS_ONLINE;
 	}
 	return ret;
 }
--- 3.14-rc2+/mm/memcontrol.c	2014-02-12 11:55:02.836035004 -0800
+++ linux/mm/memcontrol.c	2014-02-12 11:59:52.804041895 -0800
@@ -1128,9 +1128,16 @@ skip_node:
 	 */
 	if (next_css) {
 		if ((next_css == &root->css) ||
-		    ((next_css->flags & CSS_ONLINE) && css_tryget(next_css)))
+		    ((next_css->flags & CSS_ONLINE) && css_tryget(next_css))) {
+			/*
+			 * Ensure that all memcg initialization, done before
+			 * CSS_ONLINE was set, will be visible to our caller.
+			 * This matches the smp_wmb() in online_css()'s
+			 * rcu_assign_pointer(), before it set CSS_ONLINE.
+			 */
+			smp_rmb();
 			return mem_cgroup_from_css(next_css);
-
+		}
 		prev_css = next_css;
 		goto skip_node;
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2014-02-13  1:29 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-02-13  1:26 [PATCH 1/2] memcg: fix endless loop in __mem_cgroup_iter_next Hugh Dickins
2014-02-13  1:26 ` Hugh Dickins
2014-02-13  1:29 ` Hugh Dickins [this message]
2014-02-13  1:29   ` [PATCH 2/2] memcg: barriers to see memcgs as fully initialized Hugh Dickins
2014-02-13 14:53   ` Michal Hocko
2014-02-13 14:53     ` Michal Hocko
2014-02-16  2:52     ` Hugh Dickins
2014-02-16  2:52       ` Hugh Dickins
2014-02-13 21:07   ` Tejun Heo
2014-02-13 21:07     ` Tejun Heo
2014-02-13 14:23 ` [PATCH 1/2] memcg: fix endless loop in __mem_cgroup_iter_next Michal Hocko
2014-02-13 14:23   ` Michal Hocko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LSU.2.11.1402121727050.5917@eggly.anvils \
    --to=hughd@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=gthelen@google.com \
    --cc=hannes@cmpxchg.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.cz \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.