From: Sha Zhengju <handai.szj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> To: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org> Cc: "linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org" <linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org>, Cgroups <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>, Glauber Costa <glommer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Greg Thelen <gthelen-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Wu Fengguang <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org> Subject: Re: [PATCH V5 5/8] memcg: add per cgroup writeback pages accounting Date: Sat, 3 Aug 2013 17:25:01 +0800 [thread overview] Message-ID: <CAFj3OHV-VCKJfe6bv4UMvv+uj4LELDXsieRZFJD06Yrdyy=XxA@mail.gmail.com> (raw) In-Reply-To: <20130801145302.GJ5198-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org> On Thu, Aug 1, 2013 at 10:53 PM, Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org> wrote: > On Thu 01-08-13 19:54:11, Sha Zhengju wrote: >> From: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org> >> >> Similar to dirty page, we add per cgroup writeback pages accounting. The lock >> rule still is: >> mem_cgroup_begin_update_page_stat() >> modify page WRITEBACK stat >> mem_cgroup_update_page_stat() >> mem_cgroup_end_update_page_stat() >> >> There're two writeback interfaces to modify: test_{clear/set}_page_writeback(). >> Lock order: >> --> memcg->move_lock >> --> mapping->tree_lock >> >> Signed-off-by: Sha Zhengju <handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org> > > Looks good to me. Maybe I would suggest moving this patch up the stack > so that it might get merged earlier as it is simpler than dirty pages > accounting. Unless you insist on having the full series merged at once. I think the following three patches can be merged earlier: 1/8 memcg: remove MEMCG_NR_FILE_MAPPED 3/8 memcg: check for proper lock held in mem_cgroup_update_page_stat 5/8 memcg: add per cgroup writeback pages accounting Do I need to resent them again for you or they're enough? One more word, since dirty accounting is essential to future memcg dirty page throttling and it is not an optional feature now, I suspect whether we can merge the following two as well and leave the overhead optimization a separate series. :p 4/5 memcg: add per cgroup dirty pages accounting 8/8 memcg: Document cgroup dirty/writeback memory statistics The 2/8 ceph one still need more improvement, I'll separate it next version. > > Acked-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org> Thank you. > >> --- >> include/linux/memcontrol.h | 1 + >> mm/memcontrol.c | 5 +++++ >> mm/page-writeback.c | 15 +++++++++++++++ >> 3 files changed, 21 insertions(+) >> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> index f952be6..ccd35d8 100644 >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -43,6 +43,7 @@ enum mem_cgroup_stat_index { >> MEM_CGROUP_STAT_RSS_HUGE, /* # of pages charged as anon huge */ >> MEM_CGROUP_STAT_FILE_MAPPED, /* # of pages charged as file rss */ >> MEM_CGROUP_STAT_FILE_DIRTY, /* # of dirty pages in page cache */ >> + MEM_CGROUP_STAT_WRITEBACK, /* # of pages under writeback */ >> MEM_CGROUP_STAT_SWAP, /* # of pages, swapped out */ >> MEM_CGROUP_STAT_NSTATS, >> }; >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 8f3e514..6c18a6d 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -91,6 +91,7 @@ static const char * const mem_cgroup_stat_names[] = { >> "rss_huge", >> "mapped_file", >> "dirty", >> + "writeback", >> "swap", >> }; >> >> @@ -3812,6 +3813,10 @@ static int mem_cgroup_move_account(struct page *page, >> mem_cgroup_move_account_page_stat(from, to, nr_pages, >> MEM_CGROUP_STAT_FILE_DIRTY); >> >> + if (PageWriteback(page)) >> + mem_cgroup_move_account_page_stat(from, to, nr_pages, >> + MEM_CGROUP_STAT_WRITEBACK); >> + >> mem_cgroup_charge_statistics(from, page, anon, -nr_pages); >> >> /* caller should have done css_get */ >> diff --git a/mm/page-writeback.c b/mm/page-writeback.c >> index a09f518..2fa6a52 100644 >> --- a/mm/page-writeback.c >> +++ b/mm/page-writeback.c >> @@ -2008,11 +2008,17 @@ EXPORT_SYMBOL(account_page_dirtied); >> >> /* >> * Helper function for set_page_writeback family. >> + * >> + * The caller must hold mem_cgroup_begin/end_update_page_stat() lock >> + * while calling this function. >> + * See test_set_page_writeback for example. >> + * >> * NOTE: Unlike account_page_dirtied this does not rely on being atomic >> * wrt interrupts. >> */ >> void account_page_writeback(struct page *page) >> { >> + mem_cgroup_inc_page_stat(page, MEM_CGROUP_STAT_WRITEBACK); >> inc_zone_page_state(page, NR_WRITEBACK); >> } >> EXPORT_SYMBOL(account_page_writeback); >> @@ -2243,7 +2249,10 @@ int test_clear_page_writeback(struct page *page) >> { >> struct address_space *mapping = page_mapping(page); >> int ret; >> + bool locked; >> + unsigned long memcg_flags; >> >> + mem_cgroup_begin_update_page_stat(page, &locked, &memcg_flags); >> if (mapping) { >> struct backing_dev_info *bdi = mapping->backing_dev_info; >> unsigned long flags; >> @@ -2264,9 +2273,11 @@ int test_clear_page_writeback(struct page *page) >> ret = TestClearPageWriteback(page); >> } >> if (ret) { >> + mem_cgroup_dec_page_stat(page, MEM_CGROUP_STAT_WRITEBACK); >> dec_zone_page_state(page, NR_WRITEBACK); >> inc_zone_page_state(page, NR_WRITTEN); >> } >> + mem_cgroup_end_update_page_stat(page, &locked, &memcg_flags); >> return ret; >> } >> >> @@ -2274,7 +2285,10 @@ int test_set_page_writeback(struct page *page) >> { >> struct address_space *mapping = page_mapping(page); >> int ret; >> + bool locked; >> + unsigned long flags; >> >> + mem_cgroup_begin_update_page_stat(page, &locked, &flags); >> if (mapping) { >> struct backing_dev_info *bdi = mapping->backing_dev_info; >> unsigned long flags; >> @@ -2301,6 +2315,7 @@ int test_set_page_writeback(struct page *page) >> } >> if (!ret) >> account_page_writeback(page); >> + mem_cgroup_end_update_page_stat(page, &locked, &flags); >> return ret; >> >> } >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe cgroups" in >> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Michal Hocko > SUSE Labs -- Thanks, Sha
WARNING: multiple messages have this Message-ID (diff)
From: Sha Zhengju <handai.szj@gmail.com> To: Michal Hocko <mhocko@suse.cz> Cc: "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, "linux-mm@kvack.org" <linux-mm@kvack.org>, Cgroups <cgroups@vger.kernel.org>, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>, Glauber Costa <glommer@gmail.com>, Greg Thelen <gthelen@google.com>, Wu Fengguang <fengguang.wu@intel.com>, Andrew Morton <akpm@linux-foundation.org>, Sha Zhengju <handai.szj@taobao.com> Subject: Re: [PATCH V5 5/8] memcg: add per cgroup writeback pages accounting Date: Sat, 3 Aug 2013 17:25:01 +0800 [thread overview] Message-ID: <CAFj3OHV-VCKJfe6bv4UMvv+uj4LELDXsieRZFJD06Yrdyy=XxA@mail.gmail.com> (raw) In-Reply-To: <20130801145302.GJ5198@dhcp22.suse.cz> On Thu, Aug 1, 2013 at 10:53 PM, Michal Hocko <mhocko@suse.cz> wrote: > On Thu 01-08-13 19:54:11, Sha Zhengju wrote: >> From: Sha Zhengju <handai.szj@taobao.com> >> >> Similar to dirty page, we add per cgroup writeback pages accounting. The lock >> rule still is: >> mem_cgroup_begin_update_page_stat() >> modify page WRITEBACK stat >> mem_cgroup_update_page_stat() >> mem_cgroup_end_update_page_stat() >> >> There're two writeback interfaces to modify: test_{clear/set}_page_writeback(). >> Lock order: >> --> memcg->move_lock >> --> mapping->tree_lock >> >> Signed-off-by: Sha Zhengju <handai.szj@taobao.com> > > Looks good to me. Maybe I would suggest moving this patch up the stack > so that it might get merged earlier as it is simpler than dirty pages > accounting. Unless you insist on having the full series merged at once. I think the following three patches can be merged earlier: 1/8 memcg: remove MEMCG_NR_FILE_MAPPED 3/8 memcg: check for proper lock held in mem_cgroup_update_page_stat 5/8 memcg: add per cgroup writeback pages accounting Do I need to resent them again for you or they're enough? One more word, since dirty accounting is essential to future memcg dirty page throttling and it is not an optional feature now, I suspect whether we can merge the following two as well and leave the overhead optimization a separate series. :p 4/5 memcg: add per cgroup dirty pages accounting 8/8 memcg: Document cgroup dirty/writeback memory statistics The 2/8 ceph one still need more improvement, I'll separate it next version. > > Acked-by: Michal Hocko <mhocko@suse.cz> Thank you. > >> --- >> include/linux/memcontrol.h | 1 + >> mm/memcontrol.c | 5 +++++ >> mm/page-writeback.c | 15 +++++++++++++++ >> 3 files changed, 21 insertions(+) >> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> index f952be6..ccd35d8 100644 >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -43,6 +43,7 @@ enum mem_cgroup_stat_index { >> MEM_CGROUP_STAT_RSS_HUGE, /* # of pages charged as anon huge */ >> MEM_CGROUP_STAT_FILE_MAPPED, /* # of pages charged as file rss */ >> MEM_CGROUP_STAT_FILE_DIRTY, /* # of dirty pages in page cache */ >> + MEM_CGROUP_STAT_WRITEBACK, /* # of pages under writeback */ >> MEM_CGROUP_STAT_SWAP, /* # of pages, swapped out */ >> MEM_CGROUP_STAT_NSTATS, >> }; >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 8f3e514..6c18a6d 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -91,6 +91,7 @@ static const char * const mem_cgroup_stat_names[] = { >> "rss_huge", >> "mapped_file", >> "dirty", >> + "writeback", >> "swap", >> }; >> >> @@ -3812,6 +3813,10 @@ static int mem_cgroup_move_account(struct page *page, >> mem_cgroup_move_account_page_stat(from, to, nr_pages, >> MEM_CGROUP_STAT_FILE_DIRTY); >> >> + if (PageWriteback(page)) >> + mem_cgroup_move_account_page_stat(from, to, nr_pages, >> + MEM_CGROUP_STAT_WRITEBACK); >> + >> mem_cgroup_charge_statistics(from, page, anon, -nr_pages); >> >> /* caller should have done css_get */ >> diff --git a/mm/page-writeback.c b/mm/page-writeback.c >> index a09f518..2fa6a52 100644 >> --- a/mm/page-writeback.c >> +++ b/mm/page-writeback.c >> @@ -2008,11 +2008,17 @@ EXPORT_SYMBOL(account_page_dirtied); >> >> /* >> * Helper function for set_page_writeback family. >> + * >> + * The caller must hold mem_cgroup_begin/end_update_page_stat() lock >> + * while calling this function. >> + * See test_set_page_writeback for example. >> + * >> * NOTE: Unlike account_page_dirtied this does not rely on being atomic >> * wrt interrupts. >> */ >> void account_page_writeback(struct page *page) >> { >> + mem_cgroup_inc_page_stat(page, MEM_CGROUP_STAT_WRITEBACK); >> inc_zone_page_state(page, NR_WRITEBACK); >> } >> EXPORT_SYMBOL(account_page_writeback); >> @@ -2243,7 +2249,10 @@ int test_clear_page_writeback(struct page *page) >> { >> struct address_space *mapping = page_mapping(page); >> int ret; >> + bool locked; >> + unsigned long memcg_flags; >> >> + mem_cgroup_begin_update_page_stat(page, &locked, &memcg_flags); >> if (mapping) { >> struct backing_dev_info *bdi = mapping->backing_dev_info; >> unsigned long flags; >> @@ -2264,9 +2273,11 @@ int test_clear_page_writeback(struct page *page) >> ret = TestClearPageWriteback(page); >> } >> if (ret) { >> + mem_cgroup_dec_page_stat(page, MEM_CGROUP_STAT_WRITEBACK); >> dec_zone_page_state(page, NR_WRITEBACK); >> inc_zone_page_state(page, NR_WRITTEN); >> } >> + mem_cgroup_end_update_page_stat(page, &locked, &memcg_flags); >> return ret; >> } >> >> @@ -2274,7 +2285,10 @@ int test_set_page_writeback(struct page *page) >> { >> struct address_space *mapping = page_mapping(page); >> int ret; >> + bool locked; >> + unsigned long flags; >> >> + mem_cgroup_begin_update_page_stat(page, &locked, &flags); >> if (mapping) { >> struct backing_dev_info *bdi = mapping->backing_dev_info; >> unsigned long flags; >> @@ -2301,6 +2315,7 @@ int test_set_page_writeback(struct page *page) >> } >> if (!ret) >> account_page_writeback(page); >> + mem_cgroup_end_update_page_stat(page, &locked, &flags); >> return ret; >> >> } >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe cgroups" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Michal Hocko > SUSE Labs -- Thanks, Sha -- 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>
next prev parent reply other threads:[~2013-08-03 9:25 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-08-01 11:43 [PATCH V5 0/8] Add memcg dirty/writeback page accounting Sha Zhengju 2013-08-01 11:43 ` Sha Zhengju 2013-08-01 11:44 ` [PATCH 1/8] memcg: remove MEMCG_NR_FILE_MAPPED Sha Zhengju 2013-08-01 11:51 ` [PATCH V5 2/8] fs/ceph: vfs __set_page_dirty_nobuffers interface instead of doing it inside filesystem Sha Zhengju 2013-08-01 15:19 ` Yan, Zheng 2013-08-01 18:27 ` Sage Weil 2013-08-02 10:04 ` Sha Zhengju [not found] ` <CAFj3OHVXvtr5BDMrGatHZi7M9y+dh1ZKRMQZGjZmNBcg3pNQtw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-08-02 20:30 ` Sage Weil 2013-08-02 20:30 ` Sage Weil 2013-08-03 8:58 ` Sha Zhengju 2013-08-02 9:04 ` Sha Zhengju 2013-08-02 9:04 ` Sha Zhengju 2013-08-02 13:11 ` Yan, Zheng 2013-08-01 11:52 ` [PATCH V5 3/8] memcg: check for proper lock held in mem_cgroup_update_page_stat Sha Zhengju 2013-08-01 14:34 ` Michal Hocko 2013-08-01 14:34 ` Michal Hocko 2013-08-04 18:48 ` Greg Thelen 2013-08-04 18:48 ` Greg Thelen 2013-08-01 11:53 ` [PATCH V5 4/8] memcg: add per cgroup dirty pages accounting Sha Zhengju 2013-08-01 11:54 ` [PATCH V5 5/8] memcg: add per cgroup writeback " Sha Zhengju 2013-08-01 14:53 ` Michal Hocko [not found] ` <20130801145302.GJ5198-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org> 2013-08-03 9:25 ` Sha Zhengju [this message] 2013-08-03 9:25 ` Sha Zhengju [not found] ` <CAFj3OHV-VCKJfe6bv4UMvv+uj4LELDXsieRZFJD06Yrdyy=XxA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-08-04 10:08 ` Michal Hocko 2013-08-04 10:08 ` Michal Hocko 2013-08-22 9:46 ` Fwd: " Sha Zhengju 2013-08-22 9:46 ` Sha Zhengju 2013-08-22 9:50 ` [PATCH 1/4] memcg: remove MEMCG_NR_FILE_MAPPED Sha Zhengju 2013-08-22 9:52 ` [PATCH 2/4] memcg: check for proper lock held in mem_cgroup_update_page_stat Sha Zhengju 2013-08-22 9:53 ` [PATCH 3/4] memcg: add per cgroup writeback pages accounting Sha Zhengju 2013-08-22 22:40 ` Andrew Morton 2013-08-23 16:11 ` Sha Zhengju 2013-08-23 16:11 ` Sha Zhengju 2013-08-22 9:53 ` [PATCH 4/4] memcg: Document cgroup dirty/writeback memory statistics Sha Zhengju 2013-08-04 18:51 ` [PATCH V5 5/8] memcg: add per cgroup writeback pages accounting Greg Thelen 2013-08-04 18:51 ` Greg Thelen 2013-08-01 11:55 ` [PATCH V5 6/8] memcg: make nocpu_base available for non-hotplug Sha Zhengju 2013-08-01 12:00 ` [PATCH V5 7/8] memcg: don't account root memcg page stats if only root exists Sha Zhengju 2013-08-01 16:20 ` Johannes Weiner 2013-08-02 4:32 ` Sha Zhengju 2013-08-02 4:32 ` Sha Zhengju 2013-08-05 21:58 ` Johannes Weiner 2013-08-05 21:58 ` Johannes Weiner 2013-08-01 12:00 ` [PATCH V5 8/8] memcg: Document cgroup dirty/writeback memory statistics Sha Zhengju 2013-08-01 12:00 ` Sha Zhengju 2013-08-01 14:43 ` [PATCH V5 0/8] Add memcg dirty/writeback page accounting Michal Hocko 2013-08-03 9:30 ` Sha Zhengju
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='CAFj3OHV-VCKJfe6bv4UMvv+uj4LELDXsieRZFJD06Yrdyy=XxA@mail.gmail.com' \ --to=handai.szj-re5jqeeqqe8avxtiumwx3w@public.gmane.org \ --cc=akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org \ --cc=cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \ --cc=glommer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=gthelen-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org \ --cc=handai.szj-3b8fjiQLQpfQT0dZR+AlfA@public.gmane.org \ --cc=kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org \ --cc=linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org \ --cc=mhocko-AlSwsSmVLrQ@public.gmane.org \ /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.