[PATCH] memcg: Fix thresholds for 32b architectures.

[PATCH] memcg: Fix thresholds for 32b architectures.

[mhocko]

From: Michal Hocko <mhocko@suse.com>

424cdc141380 ("memcg: convert threshold to bytes") has fixed a
regression introduced by 3e32cb2e0a12 ("mm: memcontrol: lockless page
counters") where thresholds were silently converted to use page units
rather than bytes when interpreting the user input.

The fix is not complete, though, as properly pointed out by Ben
Hutchings during stable backport review. The page count is converted
to bytes but unsigned long is used to hold the value which would
be obviously not sufficient for 32b systems with more than 4G
thresholds. The same applies to usage as taken from mem_cgroup_usage
which might overflow.

Let's remove this bytes vs. pages internal tracking differences and
handle thresholds in page units internally. Chage mem_cgroup_usage()
to return the value in page units and revert 424cdc141380 because this
should be sufficient for the consistent handling.
mem_cgroup_read_u64 as the only users of mem_cgroup_usage outside of
the threshold handling code is converted to give the proper in bytes
result. It is doing that already for page_counter output so this is
more consistent as well.

The value presented to the userspace is still in bytes units.

Fixes: 424cdc141380 ("memcg: convert threshold to bytes")
Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
CC: stable@vger.kernel.org
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memcontrol.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f3cc594ffa2d..2823cafc269e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2802,7 +2802,7 @@ static unsigned long tree_stat(struct mem_cgroup *memcg,
 	return val;
 }
 
-static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
+static inline unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
 {
 	u64 val;
 
@@ -2817,7 +2817,7 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
 		else
 			val = page_counter_read(&memcg->memsw);
 	}
-	return val << PAGE_SHIFT;
+	return val;
 }
 
 enum {
@@ -2851,9 +2851,9 @@ static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css,
 	switch (MEMFILE_ATTR(cft->private)) {
 	case RES_USAGE:
 		if (counter == &memcg->memory)
-			return mem_cgroup_usage(memcg, false);
+			return (u64)mem_cgroup_usage(memcg, false) * PAGE_SIZE;
 		if (counter == &memcg->memsw)
-			return mem_cgroup_usage(memcg, true);
+			return (u64)mem_cgroup_usage(memcg, true) * PAGE_SIZE;
 		return (u64)page_counter_read(counter) * PAGE_SIZE;
 	case RES_LIMIT:
 		return (u64)counter->limit * PAGE_SIZE;
@@ -3353,7 +3353,6 @@ static int __mem_cgroup_usage_register_event(struct mem_cgroup *memcg,
 	ret = page_counter_memparse(args, "-1", &threshold);
 	if (ret)
 		return ret;
-	threshold <<= PAGE_SHIFT;
 
 	mutex_lock(&memcg->thresholds_lock);
 
-- 
2.6.1

--
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>

Re: [PATCH] memcg: Fix thresholds for 32b architectures.

[Vladimir Davydov]

On Tue, Oct 27, 2015 at 11:37:14AM +0100, mhocko@kernel.org wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> 424cdc141380 ("memcg: convert threshold to bytes") has fixed a
> regression introduced by 3e32cb2e0a12 ("mm: memcontrol: lockless page
> counters") where thresholds were silently converted to use page units
> rather than bytes when interpreting the user input.
> 
> The fix is not complete, though, as properly pointed out by Ben
> Hutchings during stable backport review. The page count is converted
> to bytes but unsigned long is used to hold the value which would
> be obviously not sufficient for 32b systems with more than 4G
> thresholds. The same applies to usage as taken from mem_cgroup_usage
> which might overflow.
> 
> Let's remove this bytes vs. pages internal tracking differences and
> handle thresholds in page units internally. Chage mem_cgroup_usage()
> to return the value in page units and revert 424cdc141380 because this
> should be sufficient for the consistent handling.
> mem_cgroup_read_u64 as the only users of mem_cgroup_usage outside of
> the threshold handling code is converted to give the proper in bytes
> result. It is doing that already for page_counter output so this is
> more consistent as well.
> 
> The value presented to the userspace is still in bytes units.
> 
> Fixes: 424cdc141380 ("memcg: convert threshold to bytes")
> Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
> CC: stable@vger.kernel.org
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Reviewed-by: Vladimir Davydov <vdavydov@virtuozzo.com>

--
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>

Re: [PATCH] memcg: Fix thresholds for 32b architectures.

[Johannes Weiner]

On Tue, Oct 27, 2015 at 11:37:14AM +0100, mhocko@kernel.org wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> 424cdc141380 ("memcg: convert threshold to bytes") has fixed a
> regression introduced by 3e32cb2e0a12 ("mm: memcontrol: lockless page
> counters") where thresholds were silently converted to use page units
> rather than bytes when interpreting the user input.
> 
> The fix is not complete, though, as properly pointed out by Ben
> Hutchings during stable backport review. The page count is converted
> to bytes but unsigned long is used to hold the value which would
> be obviously not sufficient for 32b systems with more than 4G
> thresholds. The same applies to usage as taken from mem_cgroup_usage
> which might overflow.
> 
> Let's remove this bytes vs. pages internal tracking differences and
> handle thresholds in page units internally. Chage mem_cgroup_usage()
> to return the value in page units and revert 424cdc141380 because this
> should be sufficient for the consistent handling.
> mem_cgroup_read_u64 as the only users of mem_cgroup_usage outside of
> the threshold handling code is converted to give the proper in bytes
> result. It is doing that already for page_counter output so this is
> more consistent as well.
> 
> The value presented to the userspace is still in bytes units.
> 
> Fixes: 424cdc141380 ("memcg: convert threshold to bytes")
> Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
> CC: stable@vger.kernel.org
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

> +++ b/mm/memcontrol.c
> @@ -2802,7 +2802,7 @@ static unsigned long tree_stat(struct mem_cgroup *memcg,
>  	return val;
>  }
>  
> -static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
> +static inline unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
>  {
>  	u64 val;

Minor nit, but this should probably be unsigned long now.

--
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>

Re: [PATCH] memcg: Fix thresholds for 32b architectures.

[Michal Hocko]

On Tue 27-10-15 09:23:31, Johannes Weiner wrote:
[...]
> > Fixes: 424cdc141380 ("memcg: convert threshold to bytes")
> > Fixes: 3e32cb2e0a12 ("mm: memcontrol: lockless page counters")
> > CC: stable@vger.kernel.org
> > Reported-by: Ben Hutchings <ben@decadent.org.uk>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks!

> 
> > +++ b/mm/memcontrol.c
> > @@ -2802,7 +2802,7 @@ static unsigned long tree_stat(struct mem_cgroup *memcg,
> >  	return val;
> >  }
> >  
> > -static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
> > +static inline unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
> >  {
> >  	u64 val;
> 
> Minor nit, but this should probably be unsigned long now.

Yeah, I've missed this. Andrew, do you want me to post a new version or
you can fold a trivial update here?
---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2823cafc269e..f4c09c4e895f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2804,7 +2804,7 @@ static unsigned long tree_stat(struct mem_cgroup *memcg,
 
 static inline unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
 {
-	u64 val;
+	unsigned long val;
 
 	if (mem_cgroup_is_root(memcg)) {
 		val = tree_stat(memcg, MEM_CGROUP_STAT_CACHE);

-- 
Michal Hocko
SUSE Labs

--
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>