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

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

[Wanpeng Li]

Changlog:

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

From: Wanpeng Li <liwp@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.linux@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

Re: [PATCH 1/5 v2] memcg: replace unsigned long by u64 to avoid overflow

[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 <liwp@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.linux@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.

Re: [PATCH 1/5 v2] memcg: replace unsigned long by u64 to avoid overflow

[Kamezawa Hiroyuki]

(2012/06/25 16:52), Johannes Weiner wrote:
> 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 <liwp@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.linux@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.
>
I agree. Then, Nack from me.

Thanks,
-Kame

Re: [PATCH 1/5 v2] memcg: replace unsigned long by u64 to avoid overflow

[Wanpeng Li]

On Mon, Jun 25, 2012 at 05:30:11PM +0900, Kamezawa Hiroyuki wrote:
>(2012/06/25 16:52), Johannes Weiner wrote:
>>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 <liwp@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.linux@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.
>>
>I agree. Then, Nack from me.
>
>Thanks,
>-Kame

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

>
>

Re: [PATCH 1/5 v2] memcg: replace unsigned long by u64 to avoid overflow

[Glauber Costa]

On 07/04/2012 04:24 PM, Wanpeng Li wrote:
> On Mon, Jun 25, 2012 at 05:30:11PM +0900, Kamezawa Hiroyuki wrote:
>> (2012/06/25 16:52), Johannes Weiner wrote:
>>> 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 <liwp@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.linux@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.
>>>
>> I agree. Then, Nack from me.
>>
>> Thanks,
>> -Kame
> 
> 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?

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

Re: [PATCH 1/5 v2] memcg: replace unsigned long by u64 to avoid overflow

[Johannes Weiner]

On Wed, Jul 04, 2012 at 04:29:48PM +0400, Glauber Costa wrote:
> On 07/04/2012 04:24 PM, Wanpeng Li wrote:
> > On Mon, Jun 25, 2012 at 05:30:11PM +0900, Kamezawa Hiroyuki wrote:
> >> (2012/06/25 16:52), Johannes Weiner wrote:
> >>> 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 <liwp@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.linux@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.
> >>>
> >> I agree. Then, Nack from me.
> >>
> >> Thanks,
> >> -Kame
> > 
> > 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?
> 
> How many bytes do you intend to save by replacing "u64" with "unsigned
> long" ? Have you asked yourself this question ?

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.

Re: [PATCH 1/5 v2] memcg: replace unsigned long by u64 to avoid overflow

[Glauber Costa]

On 07/04/2012 05:16 PM, Johannes Weiner wrote:
> On Wed, Jul 04, 2012 at 04:29:48PM +0400, Glauber Costa wrote:
>> On 07/04/2012 04:24 PM, Wanpeng Li wrote:
>>> On Mon, Jun 25, 2012 at 05:30:11PM +0900, Kamezawa Hiroyuki wrote:
>>>> (2012/06/25 16:52), Johannes Weiner wrote:
>>>>> 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 <liwp@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.linux@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.
>>>>>
>>>> I agree. Then, Nack from me.
>>>>
>>>> Thanks,
>>>> -Kame
>>>
>>> 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?
>>
>> How many bytes do you intend to save by replacing "u64" with "unsigned
>> long" ? Have you asked yourself this question ?
> 
> 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.
> 
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.