Re: [RFC][PATCH 0/2] memcg: replace memcg's per cpu status counter with array counter like vmstat

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

On Thu, 1 Oct 2009 09:45:14 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:

> On Wed, 30 Sep 2009 19:04:17 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > Hi,
> > 
> > In current implementation, memcg uses its own percpu counters for counting
> > evetns and # of RSS, CACHES. Now, counter is maintainer per cpu without
> > any synchronization as vm_stat[] or percpu_counter. So, this is
> >  update-is-fast-but-read-is-slow conter.
> > 
> > Because "read" for these counter was only done by memory.stat file, I thought
> > read-side-slowness was acceptable. Amount of memory usage, which affects
> > memory limit check, can be read by memory.usage_in_bytes. It's maintained
> > by res_counter.
> > 
> > But in current -rc, root memcg's memory usage is calcualted by this per cpu
> > counter and read side slowness may be trouble if it's frequently read.
> > 
> > And, in recent discusstion, I wonder we should maintain NR_DIRTY etc...
> > in memcg. So, slow-read-counter will not match our requirements, I guess.
> > I want some counter like vm_stat[] in memcg.
> > 
> I see your concern.
> 
> But IMHO, it would be better to explain why we need a new percpu array counter
> instead of using array of percpu_counter(size or consolidation of related counters ?),
> IOW, what the benefit of percpu array counter is.
> 
Ok.
  array of 4 percpu counter means a struct like following.

     lock                4bytes (int)
     count               8bytes
     list_head           16bytes
     pointer to percpu   8bytes
     lock                ,,,
     count
     list_head
     pointer to percpu
     lock
     count
     list_head
     pointer to percpu
     lock
     count
     list_head
     pointer to percpu

    36x4= 144 bytes and this has 4 spinlocks.2 cache lines.
    4 spinlock means if one of "batch" expires in a cpu, all cache above will
    be invalidated. Most of read-only data will lost.

    Making alignments of each percpu counter to cacheline for avoiding
    false sharing means this will use 4 cachelines + percpu area.
    That's bad.

  array counter of 4 entry is:
     s8 batch            4bytes (will be aligned)
     pointer to percpu   8bytes
     elements            4bytes.
     list head           16bytes
     ==== cacheline aligned here== 128bytes.
     atomic_long_t       4x8==32bytes
     ==== should be aligned to cache ? maybe yes===

  Then, this will occupy 2 cachelines + percpu area.
  No false sharing in read-only area.
  All writes are done in one (locked) access.

Hmm..I may have to consider more about archs which has not atomic_xxx ops.

Considerng sets of counters can be updated at once, array of percpu counter
is not good choice. I think.

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>