Messages in this thread | | | Date | Fri, 7 Nov 2008 18:19:32 +0900 | From | KAMEZAWA Hiroyuki <> | Subject | Re: [RFC][PATCH 5/6] memcg: mem+swap controller |
| |
On Fri, 7 Nov 2008 18:02:48 +0900 Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
> On Wed, 5 Nov 2008 17:23:16 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote: > > Mem+Swap controller core. > > > > This patch implements per cgroup limit for usage of memory+swap. > > However there are SwapCache, double counting of swap-cache and > > swap-entry is avoided. > > > > Mem+Swap controller works as following. > > - memory usage is limited by memory.limit_in_bytes. > > - memory + swap usage is limited by memory.memsw_limit_in_bytes. > > > > > > This has following benefits. > > - A user can limit total resource usage of mem+swap. > > > > Without this, because memory resource controller doesn't take care of > > usage of swap, a process can exhaust all the swap (by memory leak.) > > We can avoid this case. > > > > And Swap is shared resource but it cannot be reclaimed (goes back to memory) > > until it's used. This characteristic can be trouble when the memory > > is divided into some parts by cpuset or memcg. > > Assume group A and group B. > > After some application executes, the system can be.. > > > > Group A -- very large free memory space but occupy 99% of swap. > > Group B -- under memory shortage but cannot use swap...it's nearly full. > > > > Ability to set appropriate swap limit for each group is required. > > > > Maybe someone wonder "why not swap but mem+swap ?" > > > > - The global LRU(kswapd) can swap out arbitrary pages. Swap-out means > > to move account from memory to swap...there is no change in usage of > > mem+swap. > > > > In other words, when we want to limit the usage of swap without affecting > > global LRU, mem+swap limit is better than just limiting swap. > > > > > > Accounting target information is stored in swap_cgroup which is > > per swap entry record. > > > > Charge is done as following. > > map > > - charge page and memsw. > > > > unmap > > - uncharge page/memsw if not SwapCache. > > > > swap-out (__delete_from_swap_cache) > > - uncharge page > > - record mem_cgroup information to swap_cgroup. > > > > swap-in (do_swap_page) > > - charged as page and memsw. > > record in swap_cgroup is cleared. > > memsw accounting is decremented. > > > > swap-free (swap_free()) > > - if swap entry is freed, memsw is uncharged by PAGE_SIZE. > > > > > > After this, usual memory resource controller handles SwapCache. > > (It was lacked(ignored) feature in current memcg but must be handled.) > > > SwapCache has been handled in [2/6] already :) > yes. I'll rewrite this.
> (snip) > > @@ -514,12 +534,25 @@ static int __mem_cgroup_try_charge(struc > > css_get(&mem->css); > > } > > > > + while (1) { > > + int ret; > > + bool noswap = false; > > > > - while (unlikely(res_counter_charge(&mem->res, PAGE_SIZE))) { > > + ret = res_counter_charge(&mem->res, PAGE_SIZE); > > + if (likely(!ret)) { > > + if (!do_swap_account) > > + break; > > + ret = res_counter_charge(&mem->memsw, PAGE_SIZE); > > + if (likely(!ret)) > > + break; > > + /* mem+swap counter fails */ > > + res_counter_uncharge(&mem->res, PAGE_SIZE); > > + noswap = true; > > + } > > if (!(gfp_mask & __GFP_WAIT)) > > goto nomem; > > > > - if (try_to_free_mem_cgroup_pages(mem, gfp_mask)) > > + if (try_to_free_mem_cgroup_pages(mem, gfp_mask, noswap)) > > continue; > > > > /* > I have two comment about try_charge. > > 1. It would be better if possible to avoid charging memsw at swapin (and uncharging > it again at mem_cgroup_cache_charge_swapin/mem_cgroup_commit_charge_swapin). > How about adding a new argument "charge_memsw" ? (it has many args already now...)
Hmm, maybe possible and good. I'll cosider this again.
> 2. Should we use swap when exceeding mem.limit but mem.limit == memsw.limit ? > I'd like to put that special case into "TODO" list. Hmm... maybe set noswap=true in that case is enough. but we have to be careful.
> (snip) > > void mem_cgroup_cancel_charge_swapin(struct mem_cgroup *mem) > > @@ -838,6 +947,7 @@ void mem_cgroup_cancel_charge_swapin(str > > if (!mem) > > return; > > res_counter_uncharge(&mem->res, PAGE_SIZE); > > + res_counter_uncharge(&mem->memsw, PAGE_SIZE); > > css_put(&mem->css); > > } > > > "if (do_swap_account)" is needed before uncharging memsw. > good catch !
> (snip) > > static struct cftype mem_cgroup_files[] = { > > { > > .name = "usage_in_bytes", > > - .private = RES_USAGE, > > + .private = MEMFILE_PRIVATE(_MEM, RES_USAGE), > > .read_u64 = mem_cgroup_read, > > }, > > { > > .name = "max_usage_in_bytes", > > - .private = RES_MAX_USAGE, > > + .private = MEMFILE_PRIVATE(_MEM, RES_MAX_USAGE), > > .trigger = mem_cgroup_reset, > > .read_u64 = mem_cgroup_read, > > }, > > { > > .name = "limit_in_bytes", > > - .private = RES_LIMIT, > > + .private = MEMFILE_PRIVATE(_MEM, RES_LIMIT), > > .write_string = mem_cgroup_write, > > .read_u64 = mem_cgroup_read, > > }, > > { > > .name = "failcnt", > > - .private = RES_FAILCNT, > > + .private = MEMFILE_PRIVATE(_MEM, RES_FAILCNT), > > .trigger = mem_cgroup_reset, > > .read_u64 = mem_cgroup_read, > > }, > > @@ -1317,6 +1541,31 @@ static struct cftype mem_cgroup_files[] > > .name = "stat", > > .read_map = mem_control_stat_show, > > }, > > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP > > + { > > + .name = "memsw.usage_in_bytes", > > + .private = MEMFILE_PRIVATE(_MEMSWAP, RES_USAGE), > > + .read_u64 = mem_cgroup_read, > > + }, > > + { > > + .name = "memsw.max_usage_in_bytes", > > + .private = MEMFILE_PRIVATE(_MEMSWAP, RES_MAX_USAGE), > > + .trigger = mem_cgroup_reset, > > + .read_u64 = mem_cgroup_read, > > + }, > > + { > > + .name = "memsw.limit_in_bytes", > > + .private = MEMFILE_PRIVATE(_MEMSWAP, RES_LIMIT), > > + .write_string = mem_cgroup_write, > > + .read_u64 = mem_cgroup_read, > > + }, > > + { > > + .name = "memsw.failcnt", > > + .private = MEMFILE_PRIVATE(_MEMSWAP, RES_FAILCNT), > > + .trigger = mem_cgroup_reset, > > + .read_u64 = mem_cgroup_read, > > + }, > > +#endif > > }; > > > IMHO, it would be better to define those "memsw.*" files as memsw_cgroup_files[], > and change mem_cgroup_populate() like: > > static int mem_cgroup_populate(struct cgroup_subsys *ss, > struct cgroup *cont) > { > int ret; > > ret = cgroup_add_files(cont, ss, mem_cgroup_files, > ARRAY_SIZE(mem_cgroup_files)); > if (!ret && do_swap_account) > ret = cgroup_add_files(cont, ss, memsw_cgroup_files, > ARRAY_SIZE(memsw_cgroup_files)); > > return ret; > } > > so that those files appear only when swap accounting is enabled. >
Nice idea. I'll try that.
Thanks, -Kame
| |