| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 19 Jul 2012 15:33:05 +0900
From: Kamezawa Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To: Sha Zhengju <handai.szj@...il.com>
CC: Greg Thelen <gthelen@...gle.com>, linux-mm@...ck.org,
cgroups@...r.kernel.org, yinghan@...gle.com,
akpm@...ux-foundation.org, mhocko@...e.cz,
linux-kernel@...r.kernel.org, Sha Zhengju <handai.szj@...bao.com>
Subject: Re: [PATCH 5/7] memcg: add per cgroup dirty pages accounting
(2012/07/11 18:32), Sha Zhengju wrote:
> On 07/10/2012 05:02 AM, Greg Thelen wrote:
>> On Thu, Jun 28 2012, Sha Zhengju wrote:
>>
>>> From: Sha Zhengju<handai.szj@...bao.com>
>>>
>>> This patch adds memcg routines to count dirty pages, which allows memory controller
>>> to maintain an accurate view of the amount of its dirty memory and can provide some
>>> info for users while group's direct reclaim is working.
>>>
>>> After Kame's commit 89c06bd5(memcg: use new logic for page stat accounting), we can
>>> use 'struct page' flag to test page state instead of per page_cgroup flag. But memcg
>>> has a feature to move a page from a cgroup to another one and may have race between
>>> "move" and "page stat accounting". So in order to avoid the race we have designed a
>>> bigger lock:
>>>
>>> mem_cgroup_begin_update_page_stat()
>>> modify page information -->(a)
>>> mem_cgroup_update_page_stat() -->(b)
>>> mem_cgroup_end_update_page_stat()
>>>
>>> It requires (a) and (b)(dirty pages accounting) can stay close enough.
>>>
>>> In the previous two prepare patches, we have reworked the vfs set page dirty routines
>>> and now the interfaces are more explicit:
>>> incrementing (2):
>>> __set_page_dirty
>>> __set_page_dirty_nobuffers
>>> decrementing (2):
>>> clear_page_dirty_for_io
>>> cancel_dirty_page
>>>
>>>
>>> Signed-off-by: Sha Zhengju<handai.szj@...bao.com>
>>> ---
>>> fs/buffer.c | 17 ++++++++++++++---
>>> include/linux/memcontrol.h | 1 +
>>> mm/filemap.c | 5 +++++
>>> mm/memcontrol.c | 28 +++++++++++++++++++++-------
>>> mm/page-writeback.c | 30 ++++++++++++++++++++++++------
>>> mm/truncate.c | 6 ++++++
>>> 6 files changed, 71 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/fs/buffer.c b/fs/buffer.c
>>> index 55522dd..d3714cc 100644
>>> --- a/fs/buffer.c
>>> +++ b/fs/buffer.c
>>> @@ -613,11 +613,19 @@ EXPORT_SYMBOL(mark_buffer_dirty_inode);
>>> int __set_page_dirty(struct page *page,
>>> struct address_space *mapping, int warn)
>>> {
>>> + bool locked;
>>> + unsigned long flags;
>>> + int ret = 0;
>> '= 0' and 'ret = 0' change (below) are redundant. My vote is to remove
>> '= 0' here.
>>
>
> Nice catch. :-)
>
>>> +
>>> if (unlikely(!mapping))
>>> return !TestSetPageDirty(page);
>>>
>>> - if (TestSetPageDirty(page))
>>> - return 0;
>>> + mem_cgroup_begin_update_page_stat(page,&locked,&flags);
>>> +
>>> + if (TestSetPageDirty(page)) {
>>> + ret = 0;
>>> + goto out;
>>> + }
>>>
>>> spin_lock_irq(&mapping->tree_lock);
>>> if (page->mapping) { /* Race with truncate? */
>>> @@ -629,7 +637,10 @@ int __set_page_dirty(struct page *page,
>>> spin_unlock_irq(&mapping->tree_lock);
>>> __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
>>>
>>> - return 1;
>>> + ret = 1;
>>> +out:
>>> + mem_cgroup_end_update_page_stat(page,&locked,&flags);
>>> + return ret;
>>> }
>>>
>>> /*
>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>> index 20b0f2d..ad37b59 100644
>>> --- a/include/linux/memcontrol.h
>>> +++ b/include/linux/memcontrol.h
>>> @@ -38,6 +38,7 @@ enum mem_cgroup_stat_index {
>>> MEM_CGROUP_STAT_RSS, /* # of pages charged as anon rss */
>>> MEM_CGROUP_STAT_FILE_MAPPED, /* # of pages charged as file rss */
>>> MEM_CGROUP_STAT_SWAP, /* # of pages, swapped out */
>>> + 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 1f19ec3..5159a49 100644
>>> --- a/mm/filemap.c
>>> +++ b/mm/filemap.c
>>> @@ -140,6 +140,11 @@ void __delete_from_page_cache(struct page *page)
>>> * having removed the page entirely.
>>> */
>>> if (PageDirty(page)&& mapping_cap_account_dirty(mapping)) {
>>> + /*
>>> + * Do not change page state, so no need to use mem_cgroup_
>>> + * {begin, end}_update_page_stat to get lock.
>>> + */
>>> + mem_cgroup_dec_page_stat(page, MEM_CGROUP_STAT_FILE_DIRTY);
>> I do not understand this comment. What serializes this function and
>> mem_cgroup_move_account()?
>>
>
> The race is exist just because the two competitors share one
> public variable and one reads it and the other writes it.
> I thought if both sides(accounting and cgroup_move) do not
> change page flag, then risks like doule-counting(see below)
> will not happen.
>
> CPU-A CPU-B
> Set PG_dirty
> (delay) move_lock_mem_cgroup()
> if (PageDirty(page))
> new_memcg->nr_dirty++
> pc->mem_cgroup = new_memcg;
> move_unlock_mem_cgroup()
> move_lock_mem_cgroup()
> memcg = pc->mem_cgroup
> new_memcg->nr_dirty++
>
>
> But after second thoughts, it does have problem if without lock:
>
> CPU-A CPU-B
> if (PageDirty(page)) {
> move_lock_mem_cgroup()
> TestClearPageDirty(page))
> memcg = pc->mem_cgroup
> new_memcg->nr_dirty --
> move_unlock_mem_cgroup()
>
> memcg = pc->mem_cgroup
> new_memcg->nr_dirty--
> }
>
>
> It may occur race between clear_page_dirty() operation.
> So this time I think we need the lock again...
>
> Kame, what about your opinion...
>
I think Dirty bit is cleared implicitly here...So, having lock will be good.
Thanks,
-Kame
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists