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

[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@xxxxxxxxxxxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxx>
> >>>> ---
> >>>>  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/