From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sha Zhengju Subject: Re: [PATCH 2/2] memcg: add per cgroup dirty pages accounting Date: Thu, 28 Jun 2012 19:32:59 +0800 Message-ID: <4FEC40EB.2000000@gmail.com> References: <1339761611-29033-1-git-send-email-handai.szj@taobao.com> <1339761717-29070-1-git-send-email-handai.szj@taobao.com> <4FDC28F0.8050805@jp.fujitsu.com> <4FE2D2F4.2020202@jp.fujitsu.com> <4FE3A998.3000606@jp.fujitsu.com> Mime-Version: 1.0 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=lrtohKA21GwnNlwpwvDPoW5VSzuSYXLMk4D45D1RhlA=; b=bYjXxgwbw4C3wdn/ziAa0WSYy0u6kk+N8Sbeom7n3rWybEACykBn+eSQQ2iBP+pUgZ ylGAnWIMWZMxmQGxklFHdVfbemiqgoYGdU5m2s/R2y5TndPAzxC05R8v7T+HAPrZIxGV uXHSNS9O3O2ZK7XC5nTdFfT7iQ8zv+T1lHzIfirLFWLCHDCS8w1SBsXc9jVDCFxIaAxu 3d2vsJ3CNRNMK09kFZ3vHFH6sv4oJjsigqo5m+Ki8UoaUt9Q5fS2/g5HJaXdCmuGPkUT Yrw9jrzrSw04QE8ET4IMEBMAuXFaKuFEv+Z8rV0pvmJMzWx1rUb+H+OcsJ6JSLEkpLnZ m+rA== In-Reply-To: <4FE3A998.3000606-+CUm20s59erQFUHtdCDX3A@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="utf-8"; format="flowed" To: Kamezawa Hiroyuki Cc: Greg Thelen , Sha Zhengju , linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, yinghan-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, mhocko-AlSwsSmVLrQ@public.gmane.org On 06/22/2012 07:09 AM, Kamezawa Hiroyuki wrote: > (2012/06/22 1:02), Greg Thelen wrote: >> On Thu, Jun 21 2012, Kamezawa Hiroyuki wrote: >> >>> (2012/06/19 23:31), Sha Zhengju wrote: >>>> On Sat, Jun 16, 2012 at 2:34 PM, Kamezawa Hiroyuki >>>> wrote: >>>>> (2012/06/16 0:32), Greg Thelen wrote: >>>>>> >>>>>> On Fri, Jun 15 2012, Sha Zhengju wrote: >>>>>> >>>>>>> This patch adds memcg routines to count dirty pages. I notice t= hat >>>>>>> the list has talked about per-cgroup dirty page limiting >>>>>>> (http://lwn.net/Articles/455341/) before, but it did not get=20 >>>>>>> merged. >>>>>> >>>>>> >>>>>> Good timing, I was just about to make another effort to get some= of >>>>>> these patches upstream. Like you, I was going to start with som= e=20 >>>>>> basic >>>>>> counters. >>>>>> >>>>>> Your approach is similar to what I have in mind. While it is=20 >>>>>> good to >>>>>> use the existing PageDirty flag, rather than introducing a new >>>>>> page_cgroup flag, there are locking complications (see below) to= =20 >>>>>> handle >>>>>> races between moving pages between memcg and the pages being=20 >>>>>> {un}marked >>>>>> dirty. >>>>>> >>>>>>> I've no idea how is this going now, but maybe we can add per cg= roup >>>>>>> dirty pages accounting first. This allows the memory controller= to >>>>>>> maintain an accurate view of the amount of its memory that is d= irty >>>>>>> and can provide some infomation while group's direct reclaim is= =20 >>>>>>> working. >>>>>>> >>>>>>> After commit 89c06bd5 (memcg: use new logic for page stat=20 >>>>>>> accounting), >>>>>>> we do not need per page_cgroup flag anymore and can directly us= e >>>>>>> struct page flag. >>>>>>> >>>>>>> >>>>>>> Signed-off-by: Sha Zhengju >>>>>>> --- >>>>>>> include/linux/memcontrol.h | 1 + >>>>>>> mm/filemap.c | 1 + >>>>>>> mm/memcontrol.c | 32=20 >>>>>>> +++++++++++++++++++++++++------- >>>>>>> mm/page-writeback.c | 2 ++ >>>>>>> mm/truncate.c | 1 + >>>>>>> 5 files changed, 30 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/include/linux/memcontrol.h=20 >>>>>>> b/include/linux/memcontrol.h >>>>>>> index a337c2e..8154ade 100644 >>>>>>> --- a/include/linux/memcontrol.h >>>>>>> +++ b/include/linux/memcontrol.h >>>>>>> @@ -39,6 +39,7 @@ enum mem_cgroup_stat_index { >>>>>>> MEM_CGROUP_STAT_FILE_MAPPED, /* # of pages charged as= =20 >>>>>>> file rss */ >>>>>>> MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */ >>>>>>> MEM_CGROUP_STAT_DATA, /* end of data requires=20 >>>>>>> synchronization */ >>>>>>> + MEM_CGROUP_STAT_FILE_DIRTY, /* # of dirty pages in pag= e=20 >>>>>>> cache */ >>>>>>> MEM_CGROUP_STAT_NSTATS, >>>>>>> }; >>>>>>> >>>>>>> diff --git a/mm/filemap.c b/mm/filemap.c >>>>>>> index 79c4b2b..5b5c121 100644 >>>>>>> --- a/mm/filemap.c >>>>>>> +++ b/mm/filemap.c >>>>>>> @@ -141,6 +141,7 @@ void __delete_from_page_cache(struct page=20 >>>>>>> *page) >>>>>>> * having removed the page entirely. >>>>>>> */ >>>>>>> if (PageDirty(page)&& =20 >>>>>>> mapping_cap_account_dirty(mapping)) { >>>>>>> + mem_cgroup_dec_page_stat(page, >>>>>>> MEM_CGROUP_STAT_FILE_DIRTY); >>>>>> >>>>>> >>>>>> You need to use mem_cgroup_{begin,end}_update_page_stat around=20 >>>>>> critical >>>>>> sections that: >>>>>> 1) check PageDirty >>>>>> 2) update MEM_CGROUP_STAT_FILE_DIRTY counter >>>>>> >>>>>> This protects against the page from being moved between memcg wh= ile >>>>>> accounting. Same comment applies to all of your new calls to >>>>>> mem_cgroup_{dec,inc}_page_stat. For usage pattern, see >>>>>> page_add_file_rmap. >>>>>> >>>>> >>>>> If you feel some difficulty with=20 >>>>> mem_cgroup_{begin,end}_update_page_stat(), >>>>> please let me know...I hope they should work enough.... >>>>> >>>> >>>> Hi, Kame >>>> >>>> While digging into the bigger lock of=20 >>>> mem_cgroup_{begin,end}_update_page_stat(), >>>> I find the reality is more complex than I thought. Simply stated, >>>> modifying page info >>>> and update page stat may be wide apart and in different level (eg. >>>> mm&fs), so if we >>>> use the big lock it may lead to scalability and maintainability=20 >>>> issues. >>>> >>>> For example: >>>> mem_cgroup_begin_update_page_stat() >>>> modify page information =3D> =20 >>>> TestSetPageDirty in=E3=80=80ceph_set_page_dirty() (fs/ceph/addr.c) >>>> XXXXXX =3D> other fs=20 >>>> operations >>>> mem_cgroup_update_page_stat() =3D> account_page_dirtied= ()=20 >>>> in=E3=80=80mm/page-writeback.c >>>> mem_cgroup_end_update_page_stat(). >>>> >>>> We can choose to get lock in higher level meaning vfs set_page_dir= ty() >>>> but this may span >>>> too much and can also have some missing cases. >>>> What's your opinion of this problem? >>>> >>> >>> yes, that's sad....If set_page_dirty() is always called under=20 >>> lock_page(), the >>> story will be easier (we'll take lock_page() in move side.) >>> but the comment on set_page_dirty() says it's not true.....Now, I=20 >>> haven't found a magical >>> way for avoiding the race. >>> (*) If holding lock_page() in move_account() can be a generic=20 >>> solution, it will be good. >>> A proposal from me is a small-start. You can start from adding= =20 >>> hooks to a >>> generic >>> functions as set_page_dirty() and __set_page_dirty_nobuffers(),=20 >>> clear_page_dirty_for_io(). >>> >>> And see what happens. I guess we can add WARN_ONCE() against caller= s=20 >>> of update_page_stat() >>> who don't take mem_cgroup_begin/end_update_page_stat() >>> (by some new check, for example, checking !rcu_read_lock_held() in=20 >>> update_stat()) >>> >>> I think we can make TODO list and catch up remaining things one by = one. >>> >>> Thanks, >>> -Kame >> >> This might be a crazy idea. Synchronization of PageDirty with the >> page->memcg->nr_dirty counter is a challenge because page->memcg can= be >> reassigned due to inter-memcg page moving. > > Yes. That's the heart of the problem. > >> Could we avoid moving dirty pages between memcg? > > How to detect it is the proebm here.... > >> Specifically, could we make them clean before moving. > > I considered that but a case > > CPU-A CPU-B > wait_for_page_cleaned > ..... SetPageDirty() > account-memcg-nr_dirty > > is problematic. _If_ > > CPU-A > lock_page() > move_page_for_accounting() > unlock_page() > > can help 99% of cases, I think this is a choice. But I haven't=20 > investigated > how many callers of set_page_dirty() holds locks.... > (I guess CleraPageDirty() callers are under lock_page() always...by=20 > quick look.) > > If most of callers calls lock_page() or=20 > mem_cgroup_begin/end_update....I think > adding WARNING(!page_locked(page) || !rcu_read_locked()) to=20 > update_stat() will > be a proof of concept and automatically shows what we should do more.= =2E. > >> This problem feels similar to page migration. This would slow >> down inter-memcg page movement, because it would require writeback. = But >> I'm suspect that this is an infrequent operation. > > I agree. But, IIUC, the reason page-migration waits for the end of I/= O=20 > is that migrating > pages under I/O (in being copied by devices) seems crazy. So, just=20 > lock_page() > will be an enough help.... > Hi, Kame I've checked some set_page_dirty callers and found that dozes of them=20 don't lock the page. =46ollowing is some comments of __set_page_dirty_nobuffers: * Most callers have locked the page, which pins the address_space in=20 memory. * But zap_pte_range() does not lock the page, however in that case th= e * mapping is pinned by the vma's ->vm_file reference. So lock_page() may not be enough too. Meanwhile, the move side have already token mem_cgroup_begin/end_updat= e=20 lock for =46ILE_MAPPED page accounting and it may be too heavy to hold another p= age=20 lock. I try to rework vfs set dirty page routines to make SetPageDirty and=20 dirty page accounting be in generic interfaces and still use mem_cgroup_begin/end_update lock. I= =20 also add writeback page accounting in similar way but more easier. I've sent out the patch set. Please feel free to point out any mistakes= =2E Thanks, Sha From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx206.postini.com [74.125.245.206]) by kanga.kvack.org (Postfix) with SMTP id E27FF6B0062 for ; Thu, 28 Jun 2012 07:33:05 -0400 (EDT) Received: by pbbrp2 with SMTP id rp2so3572160pbb.14 for ; Thu, 28 Jun 2012 04:33:05 -0700 (PDT) Message-ID: <4FEC40EB.2000000@gmail.com> Date: Thu, 28 Jun 2012 19:32:59 +0800 From: Sha Zhengju MIME-Version: 1.0 Subject: Re: [PATCH 2/2] memcg: add per cgroup dirty pages accounting References: <1339761611-29033-1-git-send-email-handai.szj@taobao.com> <1339761717-29070-1-git-send-email-handai.szj@taobao.com> <4FDC28F0.8050805@jp.fujitsu.com> <4FE2D2F4.2020202@jp.fujitsu.com> <4FE3A998.3000606@jp.fujitsu.com> In-Reply-To: <4FE3A998.3000606@jp.fujitsu.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: owner-linux-mm@kvack.org List-ID: To: Kamezawa Hiroyuki Cc: Greg Thelen , Sha Zhengju , linux-mm@kvack.org, cgroups@vger.kernel.org, yinghan@google.com, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, mhocko@suse.cz On 06/22/2012 07:09 AM, Kamezawa Hiroyuki wrote: > (2012/06/22 1:02), Greg Thelen wrote: >> On Thu, Jun 21 2012, Kamezawa Hiroyuki wrote: >> >>> (2012/06/19 23:31), Sha Zhengju wrote: >>>> On Sat, Jun 16, 2012 at 2:34 PM, Kamezawa Hiroyuki >>>> wrote: >>>>> (2012/06/16 0:32), Greg Thelen wrote: >>>>>> >>>>>> On Fri, Jun 15 2012, Sha Zhengju wrote: >>>>>> >>>>>>> This patch adds memcg routines to count dirty pages. I notice that >>>>>>> the list has talked about per-cgroup dirty page limiting >>>>>>> (http://lwn.net/Articles/455341/) before, but it did not get >>>>>>> merged. >>>>>> >>>>>> >>>>>> Good timing, I was just about to make another effort to get some of >>>>>> these patches upstream. Like you, I was going to start with some >>>>>> basic >>>>>> counters. >>>>>> >>>>>> Your approach is similar to what I have in mind. While it is >>>>>> good to >>>>>> use the existing PageDirty flag, rather than introducing a new >>>>>> page_cgroup flag, there are locking complications (see below) to >>>>>> handle >>>>>> races between moving pages between memcg and the pages being >>>>>> {un}marked >>>>>> dirty. >>>>>> >>>>>>> I've no idea how is this going now, but maybe we can add per cgroup >>>>>>> dirty pages accounting first. This allows the memory controller to >>>>>>> maintain an accurate view of the amount of its memory that is dirty >>>>>>> and can provide some infomation while group's direct reclaim is >>>>>>> working. >>>>>>> >>>>>>> After commit 89c06bd5 (memcg: use new logic for page stat >>>>>>> accounting), >>>>>>> we do not need per page_cgroup flag anymore and can directly use >>>>>>> struct page flag. >>>>>>> >>>>>>> >>>>>>> Signed-off-by: Sha Zhengju >>>>>>> --- >>>>>>> include/linux/memcontrol.h | 1 + >>>>>>> mm/filemap.c | 1 + >>>>>>> mm/memcontrol.c | 32 >>>>>>> +++++++++++++++++++++++++------- >>>>>>> mm/page-writeback.c | 2 ++ >>>>>>> mm/truncate.c | 1 + >>>>>>> 5 files changed, 30 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/include/linux/memcontrol.h >>>>>>> b/include/linux/memcontrol.h >>>>>>> index a337c2e..8154ade 100644 >>>>>>> --- a/include/linux/memcontrol.h >>>>>>> +++ b/include/linux/memcontrol.h >>>>>>> @@ -39,6 +39,7 @@ enum mem_cgroup_stat_index { >>>>>>> MEM_CGROUP_STAT_FILE_MAPPED, /* # of pages charged as >>>>>>> file rss */ >>>>>>> MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */ >>>>>>> MEM_CGROUP_STAT_DATA, /* end of data requires >>>>>>> synchronization */ >>>>>>> + MEM_CGROUP_STAT_FILE_DIRTY, /* # of dirty pages in page >>>>>>> cache */ >>>>>>> MEM_CGROUP_STAT_NSTATS, >>>>>>> }; >>>>>>> >>>>>>> diff --git a/mm/filemap.c b/mm/filemap.c >>>>>>> index 79c4b2b..5b5c121 100644 >>>>>>> --- a/mm/filemap.c >>>>>>> +++ b/mm/filemap.c >>>>>>> @@ -141,6 +141,7 @@ void __delete_from_page_cache(struct page >>>>>>> *page) >>>>>>> * having removed the page entirely. >>>>>>> */ >>>>>>> if (PageDirty(page)&& >>>>>>> mapping_cap_account_dirty(mapping)) { >>>>>>> + mem_cgroup_dec_page_stat(page, >>>>>>> MEM_CGROUP_STAT_FILE_DIRTY); >>>>>> >>>>>> >>>>>> You need to use mem_cgroup_{begin,end}_update_page_stat around >>>>>> critical >>>>>> sections that: >>>>>> 1) check PageDirty >>>>>> 2) update MEM_CGROUP_STAT_FILE_DIRTY counter >>>>>> >>>>>> This protects against the page from being moved between memcg while >>>>>> accounting. Same comment applies to all of your new calls to >>>>>> mem_cgroup_{dec,inc}_page_stat. For usage pattern, see >>>>>> page_add_file_rmap. >>>>>> >>>>> >>>>> If you feel some difficulty with >>>>> mem_cgroup_{begin,end}_update_page_stat(), >>>>> please let me know...I hope they should work enough.... >>>>> >>>> >>>> Hi, Kame >>>> >>>> While digging into the bigger lock of >>>> mem_cgroup_{begin,end}_update_page_stat(), >>>> I find the reality is more complex than I thought. Simply stated, >>>> modifying page info >>>> and update page stat may be wide apart and in different level (eg. >>>> mm&fs), so if we >>>> use the big lock it may lead to scalability and maintainability >>>> issues. >>>> >>>> For example: >>>> mem_cgroup_begin_update_page_stat() >>>> modify page information => >>>> TestSetPageDirty ina??ceph_set_page_dirty() (fs/ceph/addr.c) >>>> XXXXXX => other fs >>>> operations >>>> mem_cgroup_update_page_stat() => account_page_dirtied() >>>> ina??mm/page-writeback.c >>>> mem_cgroup_end_update_page_stat(). >>>> >>>> We can choose to get lock in higher level meaning vfs set_page_dirty() >>>> but this may span >>>> too much and can also have some missing cases. >>>> What's your opinion of this problem? >>>> >>> >>> yes, that's sad....If set_page_dirty() is always called under >>> lock_page(), the >>> story will be easier (we'll take lock_page() in move side.) >>> but the comment on set_page_dirty() says it's not true.....Now, I >>> haven't found a magical >>> way for avoiding the race. >>> (*) If holding lock_page() in move_account() can be a generic >>> solution, it will be good. >>> A proposal from me is a small-start. You can start from adding >>> hooks to a >>> generic >>> functions as set_page_dirty() and __set_page_dirty_nobuffers(), >>> clear_page_dirty_for_io(). >>> >>> And see what happens. I guess we can add WARN_ONCE() against callers >>> of update_page_stat() >>> who don't take mem_cgroup_begin/end_update_page_stat() >>> (by some new check, for example, checking !rcu_read_lock_held() in >>> update_stat()) >>> >>> I think we can make TODO list and catch up remaining things one by one. >>> >>> Thanks, >>> -Kame >> >> This might be a crazy idea. Synchronization of PageDirty with the >> page->memcg->nr_dirty counter is a challenge because page->memcg can be >> reassigned due to inter-memcg page moving. > > Yes. That's the heart of the problem. > >> Could we avoid moving dirty pages between memcg? > > How to detect it is the proebm here.... > >> Specifically, could we make them clean before moving. > > I considered that but a case > > CPU-A CPU-B > wait_for_page_cleaned > ..... SetPageDirty() > account-memcg-nr_dirty > > is problematic. _If_ > > CPU-A > lock_page() > move_page_for_accounting() > unlock_page() > > can help 99% of cases, I think this is a choice. But I haven't > investigated > how many callers of set_page_dirty() holds locks.... > (I guess CleraPageDirty() callers are under lock_page() always...by > quick look.) > > If most of callers calls lock_page() or > mem_cgroup_begin/end_update....I think > adding WARNING(!page_locked(page) || !rcu_read_locked()) to > update_stat() will > be a proof of concept and automatically shows what we should do more... > >> This problem feels similar to page migration. This would slow >> down inter-memcg page movement, because it would require writeback. But >> I'm suspect that this is an infrequent operation. > > I agree. But, IIUC, the reason page-migration waits for the end of I/O > is that migrating > pages under I/O (in being copied by devices) seems crazy. So, just > lock_page() > will be an enough help.... > Hi, Kame I've checked some set_page_dirty callers and found that dozes of them don't lock the page. Following is some comments of __set_page_dirty_nobuffers: * Most callers have locked the page, which pins the address_space in memory. * But zap_pte_range() does not lock the page, however in that case the * mapping is pinned by the vma's ->vm_file reference. So lock_page() may not be enough too. Meanwhile, the move side have already token mem_cgroup_begin/end_update lock for FILE_MAPPED page accounting and it may be too heavy to hold another page lock. I try to rework vfs set dirty page routines to make SetPageDirty and dirty page accounting be in generic interfaces and still use mem_cgroup_begin/end_update lock. I also add writeback page accounting in similar way but more easier. I've sent out the patch set. Please feel free to point out any mistakes. 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754071Ab2F1LdI (ORCPT ); Thu, 28 Jun 2012 07:33:08 -0400 Received: from mail-pz0-f46.google.com ([209.85.210.46]:42989 "EHLO mail-pz0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751473Ab2F1LdF (ORCPT ); Thu, 28 Jun 2012 07:33:05 -0400 Message-ID: <4FEC40EB.2000000@gmail.com> Date: Thu, 28 Jun 2012 19:32:59 +0800 From: Sha Zhengju User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110922 Thunderbird/3.1.15 MIME-Version: 1.0 To: Kamezawa Hiroyuki CC: Greg Thelen , Sha Zhengju , linux-mm@kvack.org, cgroups@vger.kernel.org, yinghan@google.com, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, mhocko@suse.cz Subject: Re: [PATCH 2/2] memcg: add per cgroup dirty pages accounting References: <1339761611-29033-1-git-send-email-handai.szj@taobao.com> <1339761717-29070-1-git-send-email-handai.szj@taobao.com> <4FDC28F0.8050805@jp.fujitsu.com> <4FE2D2F4.2020202@jp.fujitsu.com> <4FE3A998.3000606@jp.fujitsu.com> In-Reply-To: <4FE3A998.3000606@jp.fujitsu.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/22/2012 07:09 AM, Kamezawa Hiroyuki wrote: > (2012/06/22 1:02), Greg Thelen wrote: >> On Thu, Jun 21 2012, Kamezawa Hiroyuki wrote: >> >>> (2012/06/19 23:31), Sha Zhengju wrote: >>>> On Sat, Jun 16, 2012 at 2:34 PM, Kamezawa Hiroyuki >>>> wrote: >>>>> (2012/06/16 0:32), Greg Thelen wrote: >>>>>> >>>>>> On Fri, Jun 15 2012, Sha Zhengju wrote: >>>>>> >>>>>>> This patch adds memcg routines to count dirty pages. I notice that >>>>>>> the list has talked about per-cgroup dirty page limiting >>>>>>> (http://lwn.net/Articles/455341/) before, but it did not get >>>>>>> merged. >>>>>> >>>>>> >>>>>> Good timing, I was just about to make another effort to get some of >>>>>> these patches upstream. Like you, I was going to start with some >>>>>> basic >>>>>> counters. >>>>>> >>>>>> Your approach is similar to what I have in mind. While it is >>>>>> good to >>>>>> use the existing PageDirty flag, rather than introducing a new >>>>>> page_cgroup flag, there are locking complications (see below) to >>>>>> handle >>>>>> races between moving pages between memcg and the pages being >>>>>> {un}marked >>>>>> dirty. >>>>>> >>>>>>> I've no idea how is this going now, but maybe we can add per cgroup >>>>>>> dirty pages accounting first. This allows the memory controller to >>>>>>> maintain an accurate view of the amount of its memory that is dirty >>>>>>> and can provide some infomation while group's direct reclaim is >>>>>>> working. >>>>>>> >>>>>>> After commit 89c06bd5 (memcg: use new logic for page stat >>>>>>> accounting), >>>>>>> we do not need per page_cgroup flag anymore and can directly use >>>>>>> struct page flag. >>>>>>> >>>>>>> >>>>>>> Signed-off-by: Sha Zhengju >>>>>>> --- >>>>>>> include/linux/memcontrol.h | 1 + >>>>>>> mm/filemap.c | 1 + >>>>>>> mm/memcontrol.c | 32 >>>>>>> +++++++++++++++++++++++++------- >>>>>>> mm/page-writeback.c | 2 ++ >>>>>>> mm/truncate.c | 1 + >>>>>>> 5 files changed, 30 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/include/linux/memcontrol.h >>>>>>> b/include/linux/memcontrol.h >>>>>>> index a337c2e..8154ade 100644 >>>>>>> --- a/include/linux/memcontrol.h >>>>>>> +++ b/include/linux/memcontrol.h >>>>>>> @@ -39,6 +39,7 @@ enum mem_cgroup_stat_index { >>>>>>> MEM_CGROUP_STAT_FILE_MAPPED, /* # of pages charged as >>>>>>> file rss */ >>>>>>> MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */ >>>>>>> MEM_CGROUP_STAT_DATA, /* end of data requires >>>>>>> synchronization */ >>>>>>> + MEM_CGROUP_STAT_FILE_DIRTY, /* # of dirty pages in page >>>>>>> cache */ >>>>>>> MEM_CGROUP_STAT_NSTATS, >>>>>>> }; >>>>>>> >>>>>>> diff --git a/mm/filemap.c b/mm/filemap.c >>>>>>> index 79c4b2b..5b5c121 100644 >>>>>>> --- a/mm/filemap.c >>>>>>> +++ b/mm/filemap.c >>>>>>> @@ -141,6 +141,7 @@ void __delete_from_page_cache(struct page >>>>>>> *page) >>>>>>> * having removed the page entirely. >>>>>>> */ >>>>>>> if (PageDirty(page)&& >>>>>>> mapping_cap_account_dirty(mapping)) { >>>>>>> + mem_cgroup_dec_page_stat(page, >>>>>>> MEM_CGROUP_STAT_FILE_DIRTY); >>>>>> >>>>>> >>>>>> You need to use mem_cgroup_{begin,end}_update_page_stat around >>>>>> critical >>>>>> sections that: >>>>>> 1) check PageDirty >>>>>> 2) update MEM_CGROUP_STAT_FILE_DIRTY counter >>>>>> >>>>>> This protects against the page from being moved between memcg while >>>>>> accounting. Same comment applies to all of your new calls to >>>>>> mem_cgroup_{dec,inc}_page_stat. For usage pattern, see >>>>>> page_add_file_rmap. >>>>>> >>>>> >>>>> If you feel some difficulty with >>>>> mem_cgroup_{begin,end}_update_page_stat(), >>>>> please let me know...I hope they should work enough.... >>>>> >>>> >>>> Hi, Kame >>>> >>>> While digging into the bigger lock of >>>> mem_cgroup_{begin,end}_update_page_stat(), >>>> I find the reality is more complex than I thought. Simply stated, >>>> modifying page info >>>> and update page stat may be wide apart and in different level (eg. >>>> mm&fs), so if we >>>> use the big lock it may lead to scalability and maintainability >>>> issues. >>>> >>>> For example: >>>> mem_cgroup_begin_update_page_stat() >>>> modify page information => >>>> TestSetPageDirty in　ceph_set_page_dirty() (fs/ceph/addr.c) >>>> XXXXXX => other fs >>>> operations >>>> mem_cgroup_update_page_stat() => account_page_dirtied() >>>> in　mm/page-writeback.c >>>> mem_cgroup_end_update_page_stat(). >>>> >>>> We can choose to get lock in higher level meaning vfs set_page_dirty() >>>> but this may span >>>> too much and can also have some missing cases. >>>> What's your opinion of this problem? >>>> >>> >>> yes, that's sad....If set_page_dirty() is always called under >>> lock_page(), the >>> story will be easier (we'll take lock_page() in move side.) >>> but the comment on set_page_dirty() says it's not true.....Now, I >>> haven't found a magical >>> way for avoiding the race. >>> (*) If holding lock_page() in move_account() can be a generic >>> solution, it will be good. >>> A proposal from me is a small-start. You can start from adding >>> hooks to a >>> generic >>> functions as set_page_dirty() and __set_page_dirty_nobuffers(), >>> clear_page_dirty_for_io(). >>> >>> And see what happens. I guess we can add WARN_ONCE() against callers >>> of update_page_stat() >>> who don't take mem_cgroup_begin/end_update_page_stat() >>> (by some new check, for example, checking !rcu_read_lock_held() in >>> update_stat()) >>> >>> I think we can make TODO list and catch up remaining things one by one. >>> >>> Thanks, >>> -Kame >> >> This might be a crazy idea. Synchronization of PageDirty with the >> page->memcg->nr_dirty counter is a challenge because page->memcg can be >> reassigned due to inter-memcg page moving. > > Yes. That's the heart of the problem. > >> Could we avoid moving dirty pages between memcg? > > How to detect it is the proebm here.... > >> Specifically, could we make them clean before moving. > > I considered that but a case > > CPU-A CPU-B > wait_for_page_cleaned > ..... SetPageDirty() > account-memcg-nr_dirty > > is problematic. _If_ > > CPU-A > lock_page() > move_page_for_accounting() > unlock_page() > > can help 99% of cases, I think this is a choice. But I haven't > investigated > how many callers of set_page_dirty() holds locks.... > (I guess CleraPageDirty() callers are under lock_page() always...by > quick look.) > > If most of callers calls lock_page() or > mem_cgroup_begin/end_update....I think > adding WARNING(!page_locked(page) || !rcu_read_locked()) to > update_stat() will > be a proof of concept and automatically shows what we should do more... > >> This problem feels similar to page migration. This would slow >> down inter-memcg page movement, because it would require writeback. But >> I'm suspect that this is an infrequent operation. > > I agree. But, IIUC, the reason page-migration waits for the end of I/O > is that migrating > pages under I/O (in being copied by devices) seems crazy. So, just > lock_page() > will be an enough help.... > Hi, Kame I've checked some set_page_dirty callers and found that dozes of them don't lock the page. Following is some comments of __set_page_dirty_nobuffers: * Most callers have locked the page, which pins the address_space in memory. * But zap_pte_range() does not lock the page, however in that case the * mapping is pinned by the vma's ->vm_file reference. So lock_page() may not be enough too. Meanwhile, the move side have already token mem_cgroup_begin/end_update lock for FILE_MAPPED page accounting and it may be too heavy to hold another page lock. I try to rework vfs set dirty page routines to make SetPageDirty and dirty page accounting be in generic interfaces and still use mem_cgroup_begin/end_update lock. I also add writeback page accounting in similar way but more easier. I've sent out the patch set. Please feel free to point out any mistakes. Thanks, Sha