[Devel] Re: [PATCH] memory cgroup enhancements take 4 [5/8] add status accounting function for memory cgroup

[KAMEZAWA Hiroyuki]

At first, thank you for review.

On Wed, 31 Oct 2007 15:12:34 -0700
Andrew Morton <akpm at linux-foundation.org> wrote:
> > +static inline void __mem_cgroup_stat_add(struct mem_cgroup_stat *stat,
> > +                enum mem_cgroup_stat_index idx, int val)
> > +{
> > +	int cpu = smp_processor_id();
> > +	preempt_disable();
> > +	stat->cpustat[cpu].count[idx] += val;
> > +	preempt_enable();
> > +}
> 
> This is clearly doing smp_processor_id() in preemptible code.  (or the
> preempt_disable() just isn't needed).  I fixed it up.
> 
Thanks,
> Please ensure that you test with all runtime debugging options enabled -
> you should have seen a warning here.
> 
Sorry, I'll take care.


> > +/*
> > + * For accounting under irq disable, no need for increment preempt count.
> > + */
> > +static inline void __mem_cgroup_stat_add_safe(struct mem_cgroup_stat *stat,
> > +		enum mem_cgroup_stat_index idx, int val)
> > +{
> > +	int cpu = smp_processor_id();
> > +	stat->cpustat[cpu].count[idx] += val;
> > +}
> 
> There's a wild amount of inlining in that file.  Please, just don't do it -
> inline is a highly specialised thing and is rarely needed.
> 
> When I removed the obviously-wrong inline statements, the size of
> mm/memcontrol.o went from 3823 bytes down to 3495.
> 
> It also caused this:
> 
> mm/memcontrol.c:65: warning: '__mem_cgroup_stat_add' defined but not used
> 
> so I guess I'll just remove that.
> 
ok. I'll add again if it is needed again.

Thanks,
-Kame

_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers