Messages in this thread | | | Date | Mon, 31 Aug 2009 17:40:08 +0530 | From | Balbir Singh <> | Subject | Re: [RFC][PATCH 2/5] memcg: uncharge in batched manner |
| |
* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-08-31 20:59:18]:
> Balbir Singh wrote: > > * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-08-28 > > 13:24:38]: > > >> + } > >> + if (!batch || batch->memcg != mem) { > >> + res_counter_uncharge(&mem->res, PAGE_SIZE); > >> + if (uncharge_memsw) > >> + res_counter_uncharge(&mem->memsw, PAGE_SIZE); > > > > Could you please add a comment stating that if memcg is different that > > we do a direct uncharge else we batch. > > > really necessary ?. ok. I'll do. >
I think it will help new readers of the code.
> >> + } else { > >> + batch->pages += PAGE_SIZE; > >> + if (uncharge_memsw) > >> + batch->memsw += PAGE_SIZE; > >> + } > >> + return soft_limit_excess; > >> +} > >> /* > >> * uncharge if !page_mapped(page) > >> */ > >> @@ -1886,12 +1914,8 @@ __mem_cgroup_uncharge_common(struct page > >> break; > >> } > >> > >> - if (!mem_cgroup_is_root(mem)) { > >> - res_counter_uncharge(&mem->res, PAGE_SIZE); > >> - if (do_swap_account && > >> - (ctype != MEM_CGROUP_CHARGE_TYPE_SWAPOUT)) > >> - res_counter_uncharge(&mem->memsw, PAGE_SIZE); > >> - } > >> + if (!mem_cgroup_is_root(mem)) > >> + __do_batch_uncharge(mem, ctype); > > > > Now I am beginning to think we need a cond_mem_cgroup_is_not_root() > > function. > >
> I can't catch waht cond_mem_cgroup_is_not_root() means. >
It is something like cond_resched(), checks if mem_cgroup is not root, if so executes. Just a nit-pick
> > >> if (ctype == MEM_CGROUP_CHARGE_TYPE_SWAPOUT) > >> mem_cgroup_swap_statistics(mem, true); > >> mem_cgroup_charge_statistics(mem, pc, false); > >> @@ -1938,6 +1962,40 @@ void mem_cgroup_uncharge_cache_page(stru > >> __mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE); > >> } > >> > >> +void mem_cgroup_uncharge_batch_start(void) > >> +{ > >> + VM_BUG_ON(current->memcg_batch.do_batch); > >> + /* avoid batch if killed by OOM */ > >> + if (test_thread_flag(TIF_MEMDIE)) > >> + return; > >> + current->memcg_batch.do_batch = 1; > >> + current->memcg_batch.memcg = NULL; > >> + current->memcg_batch.pages = 0; > >> + current->memcg_batch.memsw = 0; > >> +} > >> + > >> +void mem_cgroup_uncharge_batch_end(void) > >> +{ > >> + struct mem_cgroup *mem; > >> + > >> + if (!current->memcg_batch.do_batch) > >> + return; > >> + > >> + current->memcg_batch.do_batch = 0; > >> + > >> + mem = current->memcg_batch.memcg; > >> + if (!mem) > >> + return; > >> + if (current->memcg_batch.pages) > >> + res_counter_uncharge(&mem->res, > >> + current->memcg_batch.pages, NULL); > >> + if (current->memcg_batch.memsw) > >> + res_counter_uncharge(&mem->memsw, > >> + current->memcg_batch.memsw, NULL); > >> + /* we got css's refcnt */ > >> + cgroup_release_and_wakeup_rmdir(&mem->css); > > > > > > Does this effect deleting of a group and delay it by a large amount? > > > plz see what cgroup_release_and_xxxx fixed. This is not for delay > but for race-condition, which makes rmdir sleep permanently. >
I've seen those patches, where rmdir() can hang. My conern was time elapsed since we do css_get() and do a cgroup_release_and_wake_rmdir()
-- Balbir
| |