Discussion:
[patch] mm: memcontrol: do not iterate uninitialized memcgs
Johannes Weiner
2014-09-25 02:31:18 UTC
Permalink
The cgroup iterators yield css objects that have not yet gone through
css_online(), but they are not complete memcgs at this point and so
the memcg iterators should not return them. d8ad30559715 ("mm/memcg:
iteration skip memcgs not yet fully initialized") set out to implement
exactly this, but it uses CSS_ONLINE, a cgroup-internal flag that does
not meet the ordering requirements for memcg, and so we still may see
partially initialized memcgs from the iterators.

The cgroup core can not reasonably provide a clear answer on whether
the object around the css has been fully initialized, as that depends
on controller-specific locking and lifetime rules. Thus, introduce a
memcg-specific flag that is set after the memcg has been initialized
in css_online(), and read before mem_cgroup_iter() callers access the
memcg members.

Signed-off-by: Johannes Weiner <hannes-***@public.gmane.org>
Cc: <stable-***@public.gmane.org> [3.12+]
---
mm/memcontrol.c | 35 ++++++++++++++++++++++++++++++-----
1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 306b6470784c..71ed15e3a148 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -292,6 +292,9 @@ struct mem_cgroup {
/* vmpressure notifications */
struct vmpressure vmpressure;

+ /* css_online() has been completed */
+ bool initialized;
+
/*
* the counter to account for mem+swap usage.
*/
@@ -1090,10 +1093,22 @@ skip_node:
* skipping css reference should be safe.
*/
if (next_css) {
- if ((next_css == &root->css) ||
- ((next_css->flags & CSS_ONLINE) &&
- css_tryget_online(next_css)))
- return mem_cgroup_from_css(next_css);
+ if (next_css == &root->css ||
+ css_tryget_online(next_css)) {
+ struct mem_cgroup *memcg;
+
+ memcg = mem_cgroup_from_css(next_css);
+ if (memcg->initialized) {
+ /*
+ * Make sure the caller's accesses to
+ * the memcg members are issued after
+ * we see this flag set.
+ */
+ smp_rmb();
+ return memcg;
+ }
+ css_put(next_css);
+ }

prev_css = next_css;
goto skip_node;
@@ -5413,6 +5428,7 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
{
struct mem_cgroup *memcg = mem_cgroup_from_css(css);
struct mem_cgroup *parent = mem_cgroup_from_css(css->parent);
+ int ret;

if (css->id > MEM_CGROUP_ID_MAX)
return -ENOSPC;
@@ -5449,7 +5465,16 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
}
mutex_unlock(&memcg_create_mutex);

- return memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ ret = memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ if (ret)
+ return ret;
+
+ /* Make sure the initialization is visible before the flag */
+ smp_wmb();
+
+ memcg->initialized = true;
+
+ return 0;
}

/*
--
2.1.0
Johannes Weiner
2014-09-25 02:40:55 UTC
Permalink
Argh, buggy css_put() against the root. Hand grenades, everywhere.
Update:

---
From 9b0b4d72d71cd8acd7aaa58d2006c751decc8739 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <***@cmpxchg.org>
Date: Wed, 24 Sep 2014 22:00:20 -0400
Subject: [patch] mm: memcontrol: do not iterate uninitialized memcgs

The cgroup iterators yield css objects that have not yet gone through
css_online(), but they are not complete memcgs at this point and so
the memcg iterators should not return them. d8ad30559715 ("mm/memcg:
iteration skip memcgs not yet fully initialized") set out to implement
exactly this, but it uses CSS_ONLINE, a cgroup-internal flag that does
not meet the ordering requirements for memcg, and so we still may see
partially initialized memcgs from the iterators.

The cgroup core can not reasonably provide a clear answer on whether
the object around the css has been fully initialized, as that depends
on controller-specific locking and lifetime rules. Thus, introduce a
memcg-specific flag that is set after the memcg has been initialized
in css_online(), and read before mem_cgroup_iter() callers access the
memcg members.

Signed-off-by: Johannes Weiner <***@cmpxchg.org>
Cc: <***@vger.kernel.org> [3.12+]
---
mm/memcontrol.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 306b6470784c..bafdac0f724e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -292,6 +292,9 @@ struct mem_cgroup {
/* vmpressure notifications */
struct vmpressure vmpressure;

+ /* css_online() has been completed */
+ bool initialized;
+
/*
* the counter to account for mem+swap usage.
*/
@@ -1090,10 +1093,23 @@ skip_node:
* skipping css reference should be safe.
*/
if (next_css) {
- if ((next_css == &root->css) ||
- ((next_css->flags & CSS_ONLINE) &&
- css_tryget_online(next_css)))
- return mem_cgroup_from_css(next_css);
+ struct mem_cgroup *memcg = mem_cgroup_from_css(next_css);
+
+ if (next_css == &root->css)
+ return memcg;
+
+ if (css_tryget_online(next_css)) {
+ if (memcg->initialized) {
+ /*
+ * Make sure the caller's accesses to
+ * the memcg members are issued after
+ * we see this flag set.
+ */
+ smp_rmb();
+ return memcg;
+ }
+ css_put(next_css);
+ }

prev_css = next_css;
goto skip_node;
@@ -5413,6 +5429,7 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
{
struct mem_cgroup *memcg = mem_cgroup_from_css(css);
struct mem_cgroup *parent = mem_cgroup_from_css(css->parent);
+ int ret;

if (css->id > MEM_CGROUP_ID_MAX)
return -ENOSPC;
@@ -5449,7 +5466,16 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
}
mutex_unlock(&memcg_create_mutex);

- return memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ ret = memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ if (ret)
+ return ret;
+
+ /* Make sure the initialization is visible before the flag */
+ smp_wmb();
+
+ memcg->initialized = true;
+
+ return 0;
}

/*
--
2.1.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to ***@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"***@kvack.org"> ***@kvack.org </a>
Michal Hocko
2014-09-25 11:43:39 UTC
Permalink
Post by Johannes Weiner
Argh, buggy css_put() against the root. Hand grenades, everywhere.
---
From 9b0b4d72d71cd8acd7aaa58d2006c751decc8739 Mon Sep 17 00:00:00 2001
Date: Wed, 24 Sep 2014 22:00:20 -0400
Subject: [patch] mm: memcontrol: do not iterate uninitialized memcgs
The cgroup iterators yield css objects that have not yet gone through
css_online(), but they are not complete memcgs at this point and so
iteration skip memcgs not yet fully initialized") set out to implement
exactly this, but it uses CSS_ONLINE, a cgroup-internal flag that does
not meet the ordering requirements for memcg, and so we still may see
partially initialized memcgs from the iterators.
I do not see how would this happen. CSS_ONLINE is set after css_online
callback returns and mem_cgroup_css_online ends the core initialization
with mutex_unlock which should provide sufficient memory ordering
requirements (kmem is not covered but activate_kmem_mutex kmem.tcp by
proto_list_mutex). So the worst thing that might happen is that we miss
an already initialized memcg but that shouldn't matter because such a
memcg doesn't contain any tasks nor memory. memcg_has_children doesn't
rely on our iterators so important parts will not miss anything.

So I do not see any bug right now. The flag abuse is another story and I
do agree we should use proper memcg specific synchronization here as
explained by Tejun in other email.
Post by Johannes Weiner
The cgroup core can not reasonably provide a clear answer on whether
the object around the css has been fully initialized, as that depends
on controller-specific locking and lifetime rules. Thus, introduce a
memcg-specific flag that is set after the memcg has been initialized
in css_online(), and read before mem_cgroup_iter() callers access the
memcg members.
With updated changelog
This is not necessary IMO
Post by Johannes Weiner
---
mm/memcontrol.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 306b6470784c..bafdac0f724e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -292,6 +292,9 @@ struct mem_cgroup {
/* vmpressure notifications */
struct vmpressure vmpressure;
+ /* css_online() has been completed */
+ bool initialized;
+
/*
* the counter to account for mem+swap usage.
*/
* skipping css reference should be safe.
*/
if (next_css) {
- if ((next_css == &root->css) ||
- ((next_css->flags & CSS_ONLINE) &&
- css_tryget_online(next_css)))
- return mem_cgroup_from_css(next_css);
+ struct mem_cgroup *memcg = mem_cgroup_from_css(next_css);
+
+ if (next_css == &root->css)
+ return memcg;
+
+ if (css_tryget_online(next_css)) {
+ if (memcg->initialized) {
+ /*
+ * Make sure the caller's accesses to
+ * the memcg members are issued after
+ * we see this flag set.
+ */
+ smp_rmb();
+ return memcg;
+ }
+ css_put(next_css);
+ }
prev_css = next_css;
goto skip_node;
@@ -5413,6 +5429,7 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
{
struct mem_cgroup *memcg = mem_cgroup_from_css(css);
struct mem_cgroup *parent = mem_cgroup_from_css(css->parent);
+ int ret;
if (css->id > MEM_CGROUP_ID_MAX)
return -ENOSPC;
@@ -5449,7 +5466,16 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
}
mutex_unlock(&memcg_create_mutex);
- return memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ ret = memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ if (ret)
+ return ret;
+
+ /* Make sure the initialization is visible before the flag */
+ smp_wmb();
+
+ memcg->initialized = true;
+
+ return 0;
}
/*
--
2.1.0
--
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to ***@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"***@kvack.org"> ***@kvack.org </a>
Johannes Weiner
2014-09-25 13:54:50 UTC
Permalink
Post by Michal Hocko
Post by Johannes Weiner
Argh, buggy css_put() against the root. Hand grenades, everywhere.
---
From 9b0b4d72d71cd8acd7aaa58d2006c751decc8739 Mon Sep 17 00:00:00 2001
Date: Wed, 24 Sep 2014 22:00:20 -0400
Subject: [patch] mm: memcontrol: do not iterate uninitialized memcgs
The cgroup iterators yield css objects that have not yet gone through
css_online(), but they are not complete memcgs at this point and so
iteration skip memcgs not yet fully initialized") set out to implement
exactly this, but it uses CSS_ONLINE, a cgroup-internal flag that does
not meet the ordering requirements for memcg, and so we still may see
partially initialized memcgs from the iterators.
I do not see how would this happen. CSS_ONLINE is set after css_online
callback returns and mem_cgroup_css_online ends the core initialization
with mutex_unlock which should provide sufficient memory ordering
requirements
But the iterators do not use the mutex? We are missing the matching
acquire for the proper ordering.
Michal Hocko
2014-09-25 14:11:48 UTC
Permalink
Post by Johannes Weiner
Post by Michal Hocko
Post by Johannes Weiner
Argh, buggy css_put() against the root. Hand grenades, everywhere.
---
From 9b0b4d72d71cd8acd7aaa58d2006c751decc8739 Mon Sep 17 00:00:00 2001
Date: Wed, 24 Sep 2014 22:00:20 -0400
Subject: [patch] mm: memcontrol: do not iterate uninitialized memcgs
The cgroup iterators yield css objects that have not yet gone through
css_online(), but they are not complete memcgs at this point and so
iteration skip memcgs not yet fully initialized") set out to implement
exactly this, but it uses CSS_ONLINE, a cgroup-internal flag that does
not meet the ordering requirements for memcg, and so we still may see
partially initialized memcgs from the iterators.
I do not see how would this happen. CSS_ONLINE is set after css_online
callback returns and mem_cgroup_css_online ends the core initialization
with mutex_unlock which should provide sufficient memory ordering
requirements
But the iterators do not use the mutex? We are missing the matching
acquire for the proper ordering.
OK, I guess you are right. Besides that I am not sure what are the
ordering guarantees of mutex now that I am looking into the code.

Anyway it is definitely better to be explicit about barriers.
--
Michal Hocko
SUSE Labs
Tejun Heo
2014-09-25 02:57:58 UTC
Permalink
Hello,

On Wed, Sep 24, 2014 at 10:31:18PM -0400, Johannes Weiner wrote:
..
Post by Johannes Weiner
not meet the ordering requirements for memcg, and so we still may see
partially initialized memcgs from the iterators.
It's mainly the other way around - a fully initialized css may not
show up in an iteration, but given that there's no memory ordering or
synchronization around the flag, anything can happen.

...
Post by Johannes Weiner
+ if (next_css == &root->css ||
+ css_tryget_online(next_css)) {
+ struct mem_cgroup *memcg;
+
+ memcg = mem_cgroup_from_css(next_css);
+ if (memcg->initialized) {
+ /*
+ * Make sure the caller's accesses to
+ * the memcg members are issued after
+ * we see this flag set.
I usually prefer if the comment points to the exact location that the
matching memory barriers live. Sometimes it's difficult to locate the
partner barrier even w/ the functional explanation.
Post by Johannes Weiner
+ */
+ smp_rmb();
+ return memcg;
In an unlikely event this rmb becomes an issue, a self-pointing
pointer which is set/read using smp_store_release() and
smp_load_acquire() respectively can do with plain barrier() on the
reader side on archs which don't need data dependency barrier
(basically everything except alpha). Not sure whether that'd be more
or less readable than this tho.

Thanks.
--
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to ***@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"***@kvack.org"> ***@kvack.org </a>
Johannes Weiner
2014-09-25 13:43:42 UTC
Permalink
Post by Tejun Heo
Hello,
..
Post by Johannes Weiner
not meet the ordering requirements for memcg, and so we still may see
partially initialized memcgs from the iterators.
It's mainly the other way around - a fully initialized css may not
show up in an iteration, but given that there's no memory ordering or
synchronization around the flag, anything can happen.
Oh sure, I'm just more worried about leaking invalid memcgs rather
than temporarily skipping over a fully initialized one. But I updated
the changelog to mention both possibilities.
Post by Tejun Heo
Post by Johannes Weiner
+ if (next_css == &root->css ||
+ css_tryget_online(next_css)) {
+ struct mem_cgroup *memcg;
+
+ memcg = mem_cgroup_from_css(next_css);
+ if (memcg->initialized) {
+ /*
+ * Make sure the caller's accesses to
+ * the memcg members are issued after
+ * we see this flag set.
I usually prefer if the comment points to the exact location that the
matching memory barriers live. Sometimes it's difficult to locate the
partner barrier even w/ the functional explanation.
That makes sense, updated.
Post by Tejun Heo
Post by Johannes Weiner
+ */
+ smp_rmb();
+ return memcg;
In an unlikely event this rmb becomes an issue, a self-pointing
pointer which is set/read using smp_store_release() and
smp_load_acquire() respectively can do with plain barrier() on the
reader side on archs which don't need data dependency barrier
(basically everything except alpha). Not sure whether that'd be more
or less readable than this tho.
So as far as I understand memory-barriers.txt we do not even need a
data dependency here to use store_release and load_acquire:

mem_cgroup_css_online():
<initialize memcg>
smp_store_release(&memcg->initialized, 1);

mem_cgroup_iter():
<look up maybe-initialized memcg>
if (smp_load_acquire(&memcg->initialized))
return memcg;

So while I doubt that the smp_rmb() will become a problem in this
path, it would be neat to annotate the state flag around which we
synchronize like this, rather than have an anonymous barrier.

Peter, would you know if this is correct, or whether these primitives
actually do require a data dependency?

Thanks!

Updated patch:

---
From 1cd659f42f399adc58522d478f54587c8c4dd5cc Mon Sep 17 00:00:00 2001
From: Johannes Weiner <***@cmpxchg.org>
Date: Wed, 24 Sep 2014 22:00:20 -0400
Subject: [patch] mm: memcontrol: do not iterate uninitialized memcgs

The cgroup iterators yield css objects that have not yet gone through
css_online(), but they are not complete memcgs at this point and so
the memcg iterators should not return them. d8ad30559715 ("mm/memcg:
iteration skip memcgs not yet fully initialized") set out to implement
exactly this, but it uses CSS_ONLINE, a cgroup-internal flag that does
not meet the ordering requirements for memcg, and so the iterator may
skip over initialized groups, or return partially initialized memcgs.

The cgroup core can not reasonably provide a clear answer on whether
the object around the css has been fully initialized, as that depends
on controller-specific locking and lifetime rules. Thus, introduce a
memcg-specific flag that is set after the memcg has been initialized
in css_online(), and read before mem_cgroup_iter() callers access the
memcg members.

Signed-off-by: Johannes Weiner <***@cmpxchg.org>
Cc: <***@vger.kernel.org> [3.12+]
---
mm/memcontrol.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 306b6470784c..23976fd885fd 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -292,6 +292,9 @@ struct mem_cgroup {
/* vmpressure notifications */
struct vmpressure vmpressure;

+ /* css_online() has been completed */
+ int initialized;
+
/*
* the counter to account for mem+swap usage.
*/
@@ -1090,10 +1093,21 @@ skip_node:
* skipping css reference should be safe.
*/
if (next_css) {
- if ((next_css == &root->css) ||
- ((next_css->flags & CSS_ONLINE) &&
- css_tryget_online(next_css)))
- return mem_cgroup_from_css(next_css);
+ struct mem_cgroup *memcg = mem_cgroup_from_css(next_css);
+
+ if (next_css == &root->css)
+ return memcg;
+
+ if (css_tryget_online(next_css)) {
+ /*
+ * Make sure the memcg is initialized:
+ * mem_cgroup_css_online() orders the the
+ * initialization against setting the flag.
+ */
+ if (smp_load_acquire(&memcg->initialized))
+ return memcg;
+ css_put(next_css);
+ }

prev_css = next_css;
goto skip_node;
@@ -5413,6 +5427,7 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
{
struct mem_cgroup *memcg = mem_cgroup_from_css(css);
struct mem_cgroup *parent = mem_cgroup_from_css(css->parent);
+ int ret;

if (css->id > MEM_CGROUP_ID_MAX)
return -ENOSPC;
@@ -5449,7 +5464,18 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
}
mutex_unlock(&memcg_create_mutex);

- return memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ ret = memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ if (ret)
+ return ret;
+
+ /*
+ * Make sure the memcg is initialized: mem_cgroup_iter()
+ * orders reading memcg->initialized against its callers
+ * reading the memcg members.
+ */
+ smp_store_release(&memcg->initialized, 1);
+
+ return 0;
}

/*
--
2.1.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to ***@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"***@kvack.org"> ***@kvack.org </a>
Michal Hocko
2014-09-25 14:23:12 UTC
Permalink
On Thu 25-09-14 09:43:42, Johannes Weiner wrote:
[...]
Post by Johannes Weiner
From 1cd659f42f399adc58522d478f54587c8c4dd5cc Mon Sep 17 00:00:00 2001
Date: Wed, 24 Sep 2014 22:00:20 -0400
Subject: [patch] mm: memcontrol: do not iterate uninitialized memcgs
The cgroup iterators yield css objects that have not yet gone through
css_online(), but they are not complete memcgs at this point and so
iteration skip memcgs not yet fully initialized") set out to implement
exactly this, but it uses CSS_ONLINE, a cgroup-internal flag that does
not meet the ordering requirements for memcg, and so the iterator may
skip over initialized groups, or return partially initialized memcgs.
The cgroup core can not reasonably provide a clear answer on whether
the object around the css has been fully initialized, as that depends
on controller-specific locking and lifetime rules. Thus, introduce a
memcg-specific flag that is set after the memcg has been initialized
in css_online(), and read before mem_cgroup_iter() callers access the
memcg members.
I am not an expert (obviously) on memory barriers but from
Documentation/memory-barriers.txt, my understanding is that
smp_load_acquire and smp_store_release is exactly what we need here.
"
However, after an ACQUIRE on a given variable, all memory accesses
preceding any prior RELEASE on that same variable are guaranteed to be
visible.
"

Acked-by: Michal Hocko <***@suse.cz>

Stable backport would be trickier because ACQUIRE/RELEASE were
introduced later but smp_mb() should be safe replacement.

Thanks!
Post by Johannes Weiner
---
mm/memcontrol.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 306b6470784c..23976fd885fd 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -292,6 +292,9 @@ struct mem_cgroup {
/* vmpressure notifications */
struct vmpressure vmpressure;
+ /* css_online() has been completed */
+ int initialized;
+
/*
* the counter to account for mem+swap usage.
*/
* skipping css reference should be safe.
*/
if (next_css) {
- if ((next_css == &root->css) ||
- ((next_css->flags & CSS_ONLINE) &&
- css_tryget_online(next_css)))
- return mem_cgroup_from_css(next_css);
+ struct mem_cgroup *memcg = mem_cgroup_from_css(next_css);
+
+ if (next_css == &root->css)
+ return memcg;
+
+ if (css_tryget_online(next_css)) {
+ /*
+ * mem_cgroup_css_online() orders the the
+ * initialization against setting the flag.
+ */
+ if (smp_load_acquire(&memcg->initialized))
+ return memcg;
+ css_put(next_css);
+ }
prev_css = next_css;
goto skip_node;
@@ -5413,6 +5427,7 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
{
struct mem_cgroup *memcg = mem_cgroup_from_css(css);
struct mem_cgroup *parent = mem_cgroup_from_css(css->parent);
+ int ret;
if (css->id > MEM_CGROUP_ID_MAX)
return -ENOSPC;
@@ -5449,7 +5464,18 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
}
mutex_unlock(&memcg_create_mutex);
- return memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ ret = memcg_init_kmem(memcg, &memory_cgrp_subsys);
+ if (ret)
+ return ret;
+
+ /*
+ * Make sure the memcg is initialized: mem_cgroup_iter()
+ * orders reading memcg->initialized against its callers
+ * reading the memcg members.
+ */
+ smp_store_release(&memcg->initialized, 1);
+
+ return 0;
}
/*
--
2.1.0
--
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to ***@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"***@kvack.org"> ***@kvack.org </a>
Peter Zijlstra
2014-09-26 13:39:25 UTC
Permalink
Post by Johannes Weiner
Post by Tejun Heo
Post by Johannes Weiner
+ if (next_css == &root->css ||
+ css_tryget_online(next_css)) {
+ struct mem_cgroup *memcg;
+
+ memcg = mem_cgroup_from_css(next_css);
+ if (memcg->initialized) {
+ /*
+ * Make sure the caller's accesses to
+ * the memcg members are issued after
+ * we see this flag set.
I usually prefer if the comment points to the exact location that the
matching memory barriers live. Sometimes it's difficult to locate the
partner barrier even w/ the functional explanation.
That is indeed good practise! :-)
Post by Johannes Weiner
Post by Tejun Heo
Post by Johannes Weiner
+ */
+ smp_rmb();
+ return memcg;
In an unlikely event this rmb becomes an issue, a self-pointing
pointer which is set/read using smp_store_release() and
smp_load_acquire() respectively can do with plain barrier() on the
reader side on archs which don't need data dependency barrier
(basically everything except alpha). Not sure whether that'd be more
or less readable than this tho.
So as far as I understand memory-barriers.txt we do not even need a
<initialize memcg>
smp_store_release(&memcg->initialized, 1);
<look up maybe-initialized memcg>
if (smp_load_acquire(&memcg->initialized))
return memcg;
So while I doubt that the smp_rmb() will become a problem in this
path, it would be neat to annotate the state flag around which we
synchronize like this, rather than have an anonymous barrier.
Peter, would you know if this is correct, or whether these primitives
actually do require a data dependency?
I'm fairly sure you do not. load_acquire() has the same barrier in on
Alpha that read_barrier_depends() does, and that's the only arch that
matters.

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