Re: [PATCH -mm 1/5] memcg: replace res_counter

KAMEZAWA Hiroyuki
2008-10-21T03:04:05Z

On Mon, 20 Oct 2008 19:20:16 -0700
"Paul Menage" <menage@google.com> wrote:

> On Mon, Oct 20, 2008 at 6:49 PM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > If you give me NACK, maybe I have to try that..
> > (But I believe aggregated child-parent counter will be verrrry slow.)
>
> If it's really impossible to implement the aggregated version without
> a significant performance hit then that might be a reason to have a
> separate counter class. But I'd rather have a clean generic solution
> if we can manage it.
>
I think we can't do without performance hit. Considering parent<->child counter,
parent is busier than child if usage is propergated from child to parent. So,
prefetch will be just a smal help.

> > BTW, can we have *unsigned long* version of res_counter ?
> > memcg doesn't need *unsigned long long*.
>
> Potentially - but how often is a read-only operation on the
> performance sensitive path? Don't most fast-path operations that
> involve a res_counter have an update on the res_counter when they
> succeed? In which case you have to pull the cache line into a Modified
> state anyway.
>
I don't like *unsigned long long* just because we have to do following
=
res->usage < *some number*
=
or
=
val = res->usage.
=
always under lock because usage is unsigned long long.

> >
> > And as another discussion, I'd like optimize res_counter by per_cpu.
>
> What were you thinking of doing for this?
>
just an idea. I believe a process has locality to a res_counter.

==
struct res_counter_cache {
unsigned long cache;
struct res_counter *res;
}
DEFINE_PER_CPU(res_counter_cache, pcp_memcg_res);

res_counter_charge(struct res_counter *res, struct res_counter_cache *cache, num)
{
if (cache->res == res && cache->res >= num) {
cache->cache -= num;
return 0;
} else if (cache->res != res) {
/* forget cache */
spin_lock(cache->res);
cache->res->usage -= cache->cache;
cache->cache = NULL;
spin_unlock(cache->res);
}
....
cache->cache = res;
}
....
==

But not have a fragile of code.

Thanks,
-Kame