Re: per-cpu statistics

[Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

(2013/02/28 16:59), Glauber Costa wrote:
> Hi guys
>
> Please enlighten me regarding some historic aspect of memcg before I go
> changing something I shouldn't...
>
> Regarding memcg stats, is there any reason for us to use the current
> per-cpu implementation we have instead of a percpu_counter?
>
> We are doing something like this:
>
>          get_online_cpus();
>          for_each_online_cpu(cpu)
>                  val += per_cpu(memcg->stat->count[idx], cpu);
> #ifdef CONFIG_HOTPLUG_CPU
>          spin_lock(&memcg->pcp_counter_lock);
>          val += memcg->nocpu_base.count[idx];
>          spin_unlock(&memcg->pcp_counter_lock);
> #endif
>          put_online_cpus();
>
> It seems to me that we are just re-implementing whatever percpu_counters
> already do, handling the complication ourselves.
>
> It surely is an array, and this keeps the fields together. But does it
> really matter? Did it come from some measurable result?
>
> I wouldn't touch it if it wouldn't be bothering me. But the reason I
> ask, is that I am resurrecting the patches to bypass the root cgroup
> charges when it is the only group in the system. For that, I would like
> to transfer charges from global, to our memcg equivalents.
>
> Things like MM_ANONPAGES are not percpu, though, and when I add it to
> the memcg percpu structures, I would have to somehow distribute them
> around. When we uncharge, that can become negative.
>
> percpu_counters already handle all that, and then can cope well with
> temporary negative charges in the percpu data, that is later on
> withdrawn from the main base counter.
>
> We are counting pages, so the fact that we're restricted to only half of
> the 64-bit range in percpu counters doesn't seem to be that much of a
> problem.
>
> If this is just a historic leftover, I can replace them all with
> percpu_counters. Any words on that ?
>

An reason I didn't like percpu_counter *was* its memory layout.

==
struct percpu_counter {
         raw_spinlock_t lock;
         s64 count;
#ifdef CONFIG_HOTPLUG_CPU
         struct list_head list;  /* All percpu_counters are on a list */
#endif
         s32 __percpu *counters;
};
==

Assume we have counters in an array, then, we'll have

    lock
    count
    list
    pointer
    lock
    count
    list
    pointer
    ....

An counter's lock ops will invalidate pointers in the array.
We tend to update several counters at once.

If you measure performance on enough large SMP and it looks good,
I think it's ok to go with lib/percpu_counter.c.

Thanks,
-Kame
















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