From: Andrew Morton <akpm@linux-foundation.org> To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Cc: "linux-mm@kvack.org" <linux-mm@kvack.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "nishimura@mxp.nes.nec.co.jp" <nishimura@mxp.nes.nec.co.jp>, "balbir@linux.vnet.ibm.com" <balbir@linux.vnet.ibm.com>, Ying Han <yinghan@google.com>, hannes@cmpxchg.org, Michal Hocko <mhocko@suse.cz> Subject: Re: [PATCH 8/8] memcg asyncrhouns reclaim workqueue Date: Fri, 20 May 2011 14:51:15 -0700 [thread overview] Message-ID: <20110520145115.d52f3693.akpm@linux-foundation.org> (raw) In-Reply-To: <20110520124837.72978344.kamezawa.hiroyu@jp.fujitsu.com> On Fri, 20 May 2011 12:48:37 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote: > workqueue for memory cgroup asynchronous memory shrinker. > > This patch implements the workqueue of async shrinker routine. each > memcg has a work and only one work can be scheduled at the same time. > > If shrinking memory doesn't goes well, delay will be added to the work. > When this code explodes (as it surely will), users will see large amounts of CPU consumption in the work queue thread. We want to make this as easy to debug as possible, so we should try to make the workqueue's names mappable back onto their memcg's. And anything else we can think of to help? > > ... > > +static void mem_cgroup_async_shrink(struct work_struct *work) > +{ > + struct delayed_work *dw = to_delayed_work(work); > + struct mem_cgroup *mem = container_of(dw, > + struct mem_cgroup, async_work); > + bool congested = false; > + int delay = 0; > + unsigned long long required, usage, limit, shrink_to; There's a convention which is favored by some (and ignored by the clueless ;)) which says "one definition per line". The reason I like one-definition-per-line is that it leaves a little room on the right where the programmer can explain the role of the local. Another advantage is that one can initialise it. eg: unsigned long limit = res_counter_read_u64(&mem->res, RES_LIMIT); That conveys useful information: the reader can see what it's initialised with and can infer its use. A third advantage is that it can now be made const, which conveys very useful informtation and can prevent bugs. A fourth advantage is that it makes later patches to this function more readable and easier to apply when there are conflicts. > + limit = res_counter_read_u64(&mem->res, RES_LIMIT); > + shrink_to = limit - MEMCG_ASYNC_MARGIN - PAGE_SIZE; > + usage = res_counter_read_u64(&mem->res, RES_USAGE); > + if (shrink_to <= usage) { > + required = usage - shrink_to; > + required = (required >> PAGE_SHIFT) + 1; > + /* > + * This scans some number of pages and returns that memory > + * reclaim was slow or now. If slow, we add a delay as > + * congestion_wait() in vmscan.c > + */ > + congested = mem_cgroup_shrink_static_scan(mem, (long)required); > + } > + if (test_bit(ASYNC_NORESCHED, &mem->async_flags) > + || mem_cgroup_async_should_stop(mem)) > + goto finish_scan; > + /* If memory reclaim couldn't go well, add delay */ > + if (congested) > + delay = HZ/10; Another magic number. If Moore's law holds, we need to reduce this number by 1.4 each year. Is this good? > + queue_delayed_work(memcg_async_shrinker, &mem->async_work, delay); > + return; > +finish_scan: > + cgroup_release_and_wakeup_rmdir(&mem->css); > + clear_bit(ASYNC_RUNNING, &mem->async_flags); > + return; > +} > + > +static void run_mem_cgroup_async_shrinker(struct mem_cgroup *mem) > +{ > + if (test_bit(ASYNC_NORESCHED, &mem->async_flags)) > + return; I can't work out what ASYNC_NORESCHED does. Is its name well-chosen? > + if (test_and_set_bit(ASYNC_RUNNING, &mem->async_flags)) > + return; > + cgroup_exclude_rmdir(&mem->css); > + /* > + * start reclaim with small delay. This delay will allow us to do job > + * in batch. Explain more? > + */ > + if (!queue_delayed_work(memcg_async_shrinker, &mem->async_work, 1)) { > + cgroup_release_and_wakeup_rmdir(&mem->css); > + clear_bit(ASYNC_RUNNING, &mem->async_flags); > + } > + return; > +} > + > > ... >
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Morton <akpm@linux-foundation.org> To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Cc: "linux-mm@kvack.org" <linux-mm@kvack.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "nishimura@mxp.nes.nec.co.jp" <nishimura@mxp.nes.nec.co.jp>, "balbir@linux.vnet.ibm.com" <balbir@linux.vnet.ibm.com>, Ying Han <yinghan@google.com>, hannes@cmpxchg.org, Michal Hocko <mhocko@suse.cz> Subject: Re: [PATCH 8/8] memcg asyncrhouns reclaim workqueue Date: Fri, 20 May 2011 14:51:15 -0700 [thread overview] Message-ID: <20110520145115.d52f3693.akpm@linux-foundation.org> (raw) In-Reply-To: <20110520124837.72978344.kamezawa.hiroyu@jp.fujitsu.com> On Fri, 20 May 2011 12:48:37 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote: > workqueue for memory cgroup asynchronous memory shrinker. > > This patch implements the workqueue of async shrinker routine. each > memcg has a work and only one work can be scheduled at the same time. > > If shrinking memory doesn't goes well, delay will be added to the work. > When this code explodes (as it surely will), users will see large amounts of CPU consumption in the work queue thread. We want to make this as easy to debug as possible, so we should try to make the workqueue's names mappable back onto their memcg's. And anything else we can think of to help? > > ... > > +static void mem_cgroup_async_shrink(struct work_struct *work) > +{ > + struct delayed_work *dw = to_delayed_work(work); > + struct mem_cgroup *mem = container_of(dw, > + struct mem_cgroup, async_work); > + bool congested = false; > + int delay = 0; > + unsigned long long required, usage, limit, shrink_to; There's a convention which is favored by some (and ignored by the clueless ;)) which says "one definition per line". The reason I like one-definition-per-line is that it leaves a little room on the right where the programmer can explain the role of the local. Another advantage is that one can initialise it. eg: unsigned long limit = res_counter_read_u64(&mem->res, RES_LIMIT); That conveys useful information: the reader can see what it's initialised with and can infer its use. A third advantage is that it can now be made const, which conveys very useful informtation and can prevent bugs. A fourth advantage is that it makes later patches to this function more readable and easier to apply when there are conflicts. > + limit = res_counter_read_u64(&mem->res, RES_LIMIT); > + shrink_to = limit - MEMCG_ASYNC_MARGIN - PAGE_SIZE; > + usage = res_counter_read_u64(&mem->res, RES_USAGE); > + if (shrink_to <= usage) { > + required = usage - shrink_to; > + required = (required >> PAGE_SHIFT) + 1; > + /* > + * This scans some number of pages and returns that memory > + * reclaim was slow or now. If slow, we add a delay as > + * congestion_wait() in vmscan.c > + */ > + congested = mem_cgroup_shrink_static_scan(mem, (long)required); > + } > + if (test_bit(ASYNC_NORESCHED, &mem->async_flags) > + || mem_cgroup_async_should_stop(mem)) > + goto finish_scan; > + /* If memory reclaim couldn't go well, add delay */ > + if (congested) > + delay = HZ/10; Another magic number. If Moore's law holds, we need to reduce this number by 1.4 each year. Is this good? > + queue_delayed_work(memcg_async_shrinker, &mem->async_work, delay); > + return; > +finish_scan: > + cgroup_release_and_wakeup_rmdir(&mem->css); > + clear_bit(ASYNC_RUNNING, &mem->async_flags); > + return; > +} > + > +static void run_mem_cgroup_async_shrinker(struct mem_cgroup *mem) > +{ > + if (test_bit(ASYNC_NORESCHED, &mem->async_flags)) > + return; I can't work out what ASYNC_NORESCHED does. Is its name well-chosen? > + if (test_and_set_bit(ASYNC_RUNNING, &mem->async_flags)) > + return; > + cgroup_exclude_rmdir(&mem->css); > + /* > + * start reclaim with small delay. This delay will allow us to do job > + * in batch. Explain more? > + */ > + if (!queue_delayed_work(memcg_async_shrinker, &mem->async_work, 1)) { > + cgroup_release_and_wakeup_rmdir(&mem->css); > + clear_bit(ASYNC_RUNNING, &mem->async_flags); > + } > + return; > +} > + > > ... > -- 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/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2011-05-20 21:51 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-20 3:37 [PATCH 0/8] memcg async reclaim v2 KAMEZAWA Hiroyuki 2011-05-20 3:37 ` KAMEZAWA Hiroyuki 2011-05-20 3:41 ` [PATCH 1/8] memcg: export zone reclaimable pages KAMEZAWA Hiroyuki 2011-05-20 3:41 ` KAMEZAWA Hiroyuki 2011-05-20 3:42 ` [PATCH 2/8] memcg: easy check routine for reclaimable KAMEZAWA Hiroyuki 2011-05-20 3:42 ` KAMEZAWA Hiroyuki 2011-05-20 21:49 ` Andrew Morton 2011-05-20 21:49 ` Andrew Morton 2011-05-20 23:57 ` Hiroyuki Kamezawa 2011-05-20 23:57 ` Hiroyuki Kamezawa 2011-05-20 3:43 ` [PATCH 0/8] memcg: clean up, export swapiness KAMEZAWA Hiroyuki 2011-05-20 3:43 ` KAMEZAWA Hiroyuki 2011-05-23 17:26 ` Ying Han 2011-05-23 17:26 ` Ying Han 2011-05-23 23:55 ` KAMEZAWA Hiroyuki 2011-05-23 23:55 ` KAMEZAWA Hiroyuki 2011-05-20 3:44 ` [PATCH 4/8] memcg: export release victim KAMEZAWA Hiroyuki 2011-05-20 3:44 ` KAMEZAWA Hiroyuki 2011-05-20 3:46 ` [PATCH 6/8] memcg asynchronous memory reclaim interface KAMEZAWA Hiroyuki 2011-05-20 3:46 ` KAMEZAWA Hiroyuki 2011-05-20 21:49 ` Andrew Morton 2011-05-20 21:49 ` Andrew Morton 2011-05-20 23:56 ` Hiroyuki Kamezawa 2011-05-20 23:56 ` Hiroyuki Kamezawa 2011-05-23 23:36 ` Ying Han 2011-05-23 23:36 ` Ying Han 2011-05-24 0:11 ` KAMEZAWA Hiroyuki 2011-05-24 0:11 ` KAMEZAWA Hiroyuki 2011-05-24 0:26 ` Ying Han 2011-05-24 0:26 ` Ying Han 2011-05-20 3:47 ` [PATCH 7/8] memcg static scan reclaim for asyncrhonous reclaim KAMEZAWA Hiroyuki 2011-05-20 3:47 ` KAMEZAWA Hiroyuki 2011-05-20 21:50 ` Andrew Morton 2011-05-20 21:50 ` Andrew Morton 2011-05-21 0:23 ` Hiroyuki Kamezawa 2011-05-21 0:23 ` Hiroyuki Kamezawa 2011-05-20 3:48 ` [PATCH 8/8] memcg asyncrhouns reclaim workqueue KAMEZAWA Hiroyuki 2011-05-20 3:48 ` KAMEZAWA Hiroyuki 2011-05-20 21:51 ` Andrew Morton [this message] 2011-05-20 21:51 ` Andrew Morton 2011-05-21 0:41 ` Hiroyuki Kamezawa 2011-05-21 0:41 ` Hiroyuki Kamezawa 2011-05-21 1:26 ` Andrew Morton 2011-05-21 1:26 ` Andrew Morton 2011-05-23 0:25 ` KAMEZAWA Hiroyuki 2011-05-23 0:25 ` KAMEZAWA Hiroyuki 2011-05-25 5:51 ` Ying Han [not found] ` <BANLkTimd0CAqoAnuGz7WvKsbwphJxo0eZQ@mail.gmail.com> 2011-05-24 0:19 ` [PATCH 0/8] memcg async reclaim v2 KAMEZAWA Hiroyuki 2011-05-24 0:19 ` KAMEZAWA Hiroyuki
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110520145115.d52f3693.akpm@linux-foundation.org \ --to=akpm@linux-foundation.org \ --cc=balbir@linux.vnet.ibm.com \ --cc=hannes@cmpxchg.org \ --cc=kamezawa.hiroyu@jp.fujitsu.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@suse.cz \ --cc=nishimura@mxp.nes.nec.co.jp \ --cc=yinghan@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.