[PATCH 1/6] memcg: replace unsigned long by u64 to avoid overflow

[Wanpeng Li]

From: Wanpeng Li <li...@linux.vnet.ibm.com>

Since the return value variable in mem_cgroup_zone_nr_lru_pages and
mem_cgroup_node_nr_lru_pages functions are u64, so replace the return
value of funtions by u64 to avoid overflow.

Signed-off-by: Wanpeng Li <liwp....@gmail.com>
---
mm/memcontrol.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a2677e0..724bd02 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -776,8 +776,7 @@ mem_cgroup_zone_nr_lru_pages(struct mem_cgroup *memcg, int nid, int zid,
return ret;
}

-static unsigned long
-mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
+static u64 mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
int nid, unsigned int lru_mask)
{
u64 total = 0;
@@ -790,7 +789,7 @@ mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
return total;
}

-static unsigned long mem_cgroup_nr_lru_pages(struct mem_cgroup *memcg,
+static u64 mem_cgroup_nr_lru_pages(struct mem_cgroup *memcg,
unsigned int lru_mask)
{
int nid;
--
1.7.9.5

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

[Johannes Weiner]

On Sat, Jun 23, 2012 at 02:15:34PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <li...@linux.vnet.ibm.com>
>
> Since the return value variable in mem_cgroup_zone_nr_lru_pages and
> mem_cgroup_node_nr_lru_pages functions are u64, so replace the return
> value of funtions by u64 to avoid overflow.

I wonder what 16 TB of memory must think running on a 32-bit kernel...
"What is this, an address space for ants?"

[Wanpeng Li]

On Sat, Jun 23, 2012 at 10:15:14AM +0200, Johannes Weiner wrote:
>On Sat, Jun 23, 2012 at 02:15:34PM +0800, Wanpeng Li wrote:
>> From: Wanpeng Li <li...@linux.vnet.ibm.com>
>>
>> Since the return value variable in mem_cgroup_zone_nr_lru_pages and
>> mem_cgroup_node_nr_lru_pages functions are u64, so replace the return
>> value of funtions by u64 to avoid overflow.
>
>I wonder what 16 TB of memory must think running on a 32-bit kernel...
>"What is this, an address space for ants?"

Hi Johannes,

You mean change all u64 in memcg to unsigned long? or something I
miss....

Regards,
Wanpeng Li

[Johannes Weiner]

On Sat, Jun 23, 2012 at 05:03:39PM +0800, Wanpeng Li wrote:
> On Sat, Jun 23, 2012 at 10:15:14AM +0200, Johannes Weiner wrote:
> >On Sat, Jun 23, 2012 at 02:15:34PM +0800, Wanpeng Li wrote:
> >> From: Wanpeng Li <li...@linux.vnet.ibm.com>
> >>
> >> Since the return value variable in mem_cgroup_zone_nr_lru_pages and
> >> mem_cgroup_node_nr_lru_pages functions are u64, so replace the return
> >> value of funtions by u64 to avoid overflow.
> >
> >I wonder what 16 TB of memory must think running on a 32-bit kernel...
> >"What is this, an address space for ants?"
>
> Hi Johannes,
>
> You mean change all u64 in memcg to unsigned long? or something I
> miss....

Not _all_ of them, we have some that count bytes. But those counting
pages should probably be ulong, yes.

I think Kame added the ones that you wanted to adjust the surroundings
for in particular, so let's ask him. Kame?

[Kamezawa Hiroyuki]

(2012/06/23 18:26), Johannes Weiner wrote:
> On Sat, Jun 23, 2012 at 05:03:39PM +0800, Wanpeng Li wrote:
>> On Sat, Jun 23, 2012 at 10:15:14AM +0200, Johannes Weiner wrote:
>>> On Sat, Jun 23, 2012 at 02:15:34PM +0800, Wanpeng Li wrote:
>>>> From: Wanpeng Li <li...@linux.vnet.ibm.com>
>>>>
>>>> Since the return value variable in mem_cgroup_zone_nr_lru_pages and
>>>> mem_cgroup_node_nr_lru_pages functions are u64, so replace the return
>>>> value of funtions by u64 to avoid overflow.
>>>
>>> I wonder what 16 TB of memory must think running on a 32-bit kernel...
>>> "What is this, an address space for ants?"
>>
>> Hi Johannes,
>>
>> You mean change all u64 in memcg to unsigned long? or something I
>> miss....
>
> Not _all_ of them, we have some that count bytes. But those counting
> pages should probably be ulong, yes.
>
> I think Kame added the ones that you wanted to adjust the surroundings
> for in particular, so let's ask him. Kame?
>

I've been using 'unsigned long' for the number of pages and 'u64' for the number of
bytes. I think it's enough and it should be. I don't have any reason to use u64 for
the number of pages on 32bit archs.
If 'bytes' are handled by 'unsigned long', please fix it.

BTW, zone_page_state()::/include/linux/vmstat.h returns 'unsigned long'.
If you want to change this in memcg, please change zone's ones first.

Thanks,
-Kame

[Wanpeng Li]

On Mon, Jun 25, 2012 at 01:24:26PM +0900, Kamezawa Hiroyuki wrote:
>(2012/06/23 18:26), Johannes Weiner wrote:
>>On Sat, Jun 23, 2012 at 05:03:39PM +0800, Wanpeng Li wrote:
>>>On Sat, Jun 23, 2012 at 10:15:14AM +0200, Johannes Weiner wrote:
>>>>On Sat, Jun 23, 2012 at 02:15:34PM +0800, Wanpeng Li wrote:
>>>>>From: Wanpeng Li <li...@linux.vnet.ibm.com>
>>>>>
>>>>>Since the return value variable in mem_cgroup_zone_nr_lru_pages and
>>>>>mem_cgroup_node_nr_lru_pages functions are u64, so replace the return
>>>>>value of funtions by u64 to avoid overflow.
>>>>
>>>>I wonder what 16 TB of memory must think running on a 32-bit kernel...
>>>>"What is this, an address space for ants?"
>>>
>>>Hi Johannes,
>>>
>>>You mean change all u64 in memcg to unsigned long? or something I
>>>miss....
>>
>>Not _all_ of them, we have some that count bytes. But those counting
>>pages should probably be ulong, yes.
>>
>>I think Kame added the ones that you wanted to adjust the surroundings
>>for in particular, so let's ask him. Kame?
>>
>
>I've been using 'unsigned long' for the number of pages and 'u64' for the number of
>bytes. I think it's enough and it should be. I don't have any reason to use u64 for
>the number of pages on 32bit archs.
>If 'bytes' are handled by 'unsigned long', please fix it.
>
>BTW, zone_page_state()::/include/linux/vmstat.h returns 'unsigned long'.
>If you want to change this in memcg, please change zone's ones first.

Thank you for your comments Kame! :-) I will do this in next version.
Is any suggestion about patches in this patchset.

Regards,
Wanpeng Li

[Wanpeng Li]

Changlog:

V2 -> V1:
* fix zone_page_state()::/include/linux/vmstat.h returns 'unsigned long'

From: Wanpeng Li <li...@linux.vnet.ibm.com>

Since the return value variable in mem_cgroup_zone_nr_lru_pages and
mem_cgroup_node_nr_lru_pages functions are u64, so replace the return
value of funtions by u64 to avoid overflow.

Signed-off-by: Wanpeng Li <liwp....@gmail.com>
---

include/linux/vmstat.h | 2 +-
mm/memcontrol.c | 5 ++---
2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
index 65efb92..6a14291 100644
--- a/include/linux/vmstat.h
+++ b/include/linux/vmstat.h
@@ -106,7 +106,7 @@ static inline unsigned long global_page_state(enum zone_stat_item item)
return x;
}

-static inline unsigned long zone_page_state(struct zone *zone,
+static inline u64 zone_page_state(struct zone *zone,
enum zone_stat_item item)
{
long x = atomic_long_read(&zone->vm_stat[item]);

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a2677e0..724bd02 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -776,8 +776,7 @@ mem_cgroup_zone_nr_lru_pages(struct mem_cgroup *memcg, int nid, int zid,
return ret;
}

-static unsigned long
-mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
+static u64 mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
int nid, unsigned int lru_mask)
{
u64 total = 0;
@@ -790,7 +789,7 @@ mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
return total;
}

-static unsigned long mem_cgroup_nr_lru_pages(struct mem_cgroup *memcg,
+static u64 mem_cgroup_nr_lru_pages(struct mem_cgroup *memcg,
unsigned int lru_mask)
{
int nid;
--
1.7.9.5

[Johannes Weiner]

On Mon, Jun 25, 2012 at 02:04:20PM +0800, Wanpeng Li wrote:
> Changlog:
>
> V2 -> V1:
> * fix zone_page_state()::/include/linux/vmstat.h returns 'unsigned long'
>
> From: Wanpeng Li <li...@linux.vnet.ibm.com>
>
> Since the return value variable in mem_cgroup_zone_nr_lru_pages and
> mem_cgroup_node_nr_lru_pages functions are u64, so replace the return
> value of funtions by u64 to avoid overflow.
>
> Signed-off-by: Wanpeng Li <liwp....@gmail.com>
> ---
> include/linux/vmstat.h | 2 +-
> mm/memcontrol.c | 5 ++---
> 2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
> index 65efb92..6a14291 100644
> --- a/include/linux/vmstat.h
> +++ b/include/linux/vmstat.h
> @@ -106,7 +106,7 @@ static inline unsigned long global_page_state(enum zone_stat_item item)
> return x;
> }
>
> -static inline unsigned long zone_page_state(struct zone *zone,
> +static inline u64 zone_page_state(struct zone *zone,
> enum zone_stat_item item)
> {
> long x = atomic_long_read(&zone->vm_stat[item]);

We established that there is no known reason to use ulong for page
counters and that IF YOU HAD ONE, you should obviously say so and then
do a wholesale conversion. But I don't think you have one.

This patch makes absolutely no sense.

[Kamezawa Hiroyuki]

I agree. Then, Nack from me.

Thanks,
-Kame

[Wanpeng Li]

static unsigned long

mem_cgroup_node_nr_lru_pages(struct mem_cgroup *memcg,
int nid, unsigned int lru_mask)
{
u64 total = 0;

int zid;

for (zid = 0; zid < MAX_NR_ZONES; zid++)
total += mem_cgroup_zone_nr_lru_pages(memcg,
nid, zid, lru_mask);

return total;
}

Since you use unsigned long to caculate nr_pages and unsigned long long
to caculate bytes, so u64 in function mem_cgroup_node_nr_lru_pages should
replace by unsigned long to save kernel stack, right?

Regards,
Wanpeng Li

[Glauber Costa]

How many bytes do you intend to save by replacing "u64" with "unsigned
long" ? Have you asked yourself this question ?

[Johannes Weiner]

I think fixing these types is a good thing. Not for their memory
savings but to clarify the code. Otherwise, people will scratch their
heads and waste time over whether there is something that requires the
counter to be 64-bit on a 32-bit system when really there isn't.

[Glauber Costa]

I don't see a reason not to be type consistent, it's just not a stack
issue at all. Being just a type fix, its benefits need to be offset
against any problems it may bring, such as conflicts with other people
working on that, risking doing a mid-term conversion that break
something, and all the likes.