| 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
| ||
|
Message-ID: <50EE247B.6090405@jp.fujitsu.com>
Date: Thu, 10 Jan 2013 11:16:27 +0900
From: Kamezawa Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To: Sha Zhengju <handai.szj@...il.com>
CC: Michal Hocko <mhocko@...e.cz>, Hugh Dickins <hughd@...gle.com>,
Johannes Weiner <hannes@...xchg.org>,
linux-kernel@...r.kernel.org, cgroups@...r.kernel.org,
linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
akpm@...ux-foundation.org, gthelen@...gle.com,
fengguang.wu@...el.com, glommer@...allels.com, dchinner@...hat.com,
Sha Zhengju <handai.szj@...bao.com>
Subject: Re: [PATCH V3 4/8] memcg: add per cgroup dirty pages accounting
(2013/01/10 0:02), Sha Zhengju wrote:
> On Mon, Jan 7, 2013 at 3:25 PM, Kamezawa Hiroyuki
> <kamezawa.hiroyu@...fujitsu.com> wrote:
>> (2013/01/05 13:48), Sha Zhengju wrote:
>>>
>>> On Wed, Jan 2, 2013 at 6:44 PM, Michal Hocko <mhocko@...e.cz> wrote:
>>>>
>>>> On Wed 26-12-12 01:26:07, 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 cgroup's direct reclaim is working.
>>>>
>>>>
>>>> I guess you meant targeted resp. (hard/soft) limit reclaim here,
>>>> right? It is true that this is direct reclaim but it is not clear to me
>>>
>>>
>>> Yes, I meant memcg hard/soft reclaim here which is triggered directly
>>> by allocation and is distinct from background kswapd reclaim (global).
>>>
>>>> why the usefulnes should be limitted to the reclaim for users. I would
>>>> understand this if the users was in fact in-kernel users.
>>>>
>>>
>>> One of the reasons I'm trying to accounting the dirty pages is to get a
>>> more board overall view of memory usages because memcg hard/soft
>>> reclaim may have effect on response time of user application.
>>> Yeah, the beneficiary can be application administrator or kernel users.
>>> :P
>>>
>>>> [...]
>>>>>
>>>>> To prevent AB/BA deadlock mentioned by Greg Thelen in previous version
>>>>> (https://lkml.org/lkml/2012/7/30/227), we adjust the lock order:
>>>>> ->private_lock --> mapping->tree_lock --> memcg->move_lock.
>>>>> So we need to make mapping->tree_lock ahead of TestSetPageDirty in
>>>>> __set_page_dirty()
>>>>> and __set_page_dirty_nobuffers(). But in order to avoiding useless
>>>>> spinlock contention,
>>>>> a prepare PageDirty() checking is added.
>>>>
>>>>
>>>> But there is another AA deadlock here I believe.
>>>> page_remove_rmap
>>>> mem_cgroup_begin_update_page_stat <<< 1
>>>> set_page_dirty
>>>> __set_page_dirty_buffers
>>>> __set_page_dirty
>>>> mem_cgroup_begin_update_page_stat <<< 2
>>>> move_lock_mem_cgroup
>>>> spin_lock_irqsave(&memcg->move_lock, *flags);
>>>>
>>>> mem_cgroup_begin_update_page_stat is not recursive wrt. locking AFAICS
>>>> because we might race with the moving charges:
>>>> CPU0 CPU1
>>>> page_remove_rmap
>>>> mem_cgroup_can_attach
>>>> mem_cgroup_begin_update_page_stat (1)
>>>> rcu_read_lock
>>>> mem_cgroup_start_move
>>>>
>>>> atomic_inc(&memcg_moving)
>>>>
>>>> atomic_inc(&memcg->moving_account)
>>>> synchronize_rcu
>>>> __mem_cgroup_begin_update_page_stat
>>>> mem_cgroup_stolen <<< TRUE
>>>> move_lock_mem_cgroup
>>>> [...]
>>>> mem_cgroup_begin_update_page_stat (2)
>>>> __mem_cgroup_begin_update_page_stat
>>>> mem_cgroup_stolen <<< still TRUE
>>>> move_lock_mem_cgroup <<< DEADLOCK
>>>> [...]
>>>> mem_cgroup_end_update_page_stat
>>>> rcu_unlock
>>>> # wake up from
>>>> synchronize_rcu
>>>> [...]
>>>> mem_cgroup_move_task
>>>> mem_cgroup_move_charge
>>>> walk_page_range
>>>>
>>>> mem_cgroup_move_account
>>>>
>>>> move_lock_mem_cgroup
>>>>
>>>>
>>>> Maybe I have missed some other locking which would prevent this from
>>>> happening but the locking relations are really complicated in this area
>>>> so if mem_cgroup_{begin,end}_update_page_stat might be called
>>>> recursively then we need a fat comment which justifies that.
>>>>
>>>
>>> Ohhh...good catching! I didn't notice there is a recursive call of
>>> mem_cgroup_{begin,end}_update_page_stat in page_remove_rmap().
>>> The mem_cgroup_{begin,end}_update_page_stat() design has depressed
>>> me a lot recently as the lock granularity is a little bigger than I
>>> thought.
>>> Not only the resource but also some code logic is in the range of locking
>>> which may be deadlock prone. The problem still exists if we are trying to
>>> add stat account of other memcg page later, may I make bold to suggest
>>> that we dig into the lock again...
>>>
>>> But with regard to the current lock implementation, I doubt if we can we
>>> can
>>> account MEM_CGROUP_STAT_FILE_{MAPPED, DIRTY} in one breath and just
>>> try to get move_lock once in the beginning. IMHO we can make
>>> mem_cgroup_{begin,end}_update_page_stat() to recursive aware and what I'm
>>> thinking now is changing memcg->move_lock to rw-spinlock from the
>>> original spinlock:
>>> mem_cgroup_{begin,end}_update_page_stat() try to get the read lock which
>>> make it
>>> reenterable and memcg moving task side try to get the write spinlock.
>>> Then the race may be following:
>>>
>>> CPU0 CPU1
>>> page_remove_rmap
>>> mem_cgroup_can_attach
>>> mem_cgroup_begin_update_page_stat (1)
>>> rcu_read_lock
>>> mem_cgroup_start_move
>>>
>>> atomic_inc(&memcg_moving)
>>>
>>> atomic_inc(&memcg->moving_account)
>>> synchronize_rcu
>>> __mem_cgroup_begin_update_page_stat
>>> mem_cgroup_stolen <<< TRUE
>>> move_lock_mem_cgroup <<<< read-spinlock success
>>> [...]
>>> mem_cgroup_begin_update_page_stat (2)
>>> __mem_cgroup_begin_update_page_stat
>>> mem_cgroup_stolen <<< still TRUE
>>> move_lock_mem_cgroup <<<< read-spinlock success
>>>
>>> [...]
>>> mem_cgroup_end_update_page_stat <<< locked = true, unlock
>>> rcu_unlock
>>> # wake up from
>>> synchronize_rcu
>>> [...]
>>> mem_cgroup_move_task
>>> mem_cgroup_move_charge
>>> walk_page_range
>>>
>>> mem_cgroup_move_account
>>>
>>> move_lock_mem_cgroup <<< write-spinlock
>>>
>>>
>>> AFAICS, the deadlock seems to be avoided by both the rcu and rwlock.
>>> Is there anything I lost?
>>>
>>
>> rwlock will work with the nest but it seems ugly do updates under read-lock.
>>
>> How about this straightforward ?
>> ==
>> /*
>> * Once a thread takes memcg_move_lock() on a memcg, it can take the lock on
>> * the memcg again for nesting calls
>> */
>> static void move_lock_mem_cgroup(memcg, flags);
>> {
>> current->memcg_move_lock_nested += 1;
>> if (current->memcg_move_lock_nested > 1) {
>> VM_BUG_ON(current->move_locked_memcg != memcg);
>> return;
>> }
>> spin_lock_irqsave(&memcg_move_lock, &flags);
>> current->move_lockdev_memcg = memcg;
>> }
>>
>> static void move_unlock_mem_cgroup(memcg, flags)
>> {
>> current->memcg_move_lock_nested -= 1;
>> if (!current->memcg_move_lock_nested) {
>> current->move_locked_memcg = NULL;
>> spin_unlock_irqrestore(&memcg_move_lock,flags);
>> }
>> }
>>
> Does we need to add two
> fields(current->memcg_move_lock_nested/move_locked_memcg) to 'struct
> task'? Is it feasible?
>
> Now I'm thinking about another synchronization proposal for memcg page
> stat updater and move_account, which seems to deal with recursion
> issue and deadlock:
>
> CPU A CPU B
>
> move_lock_mem_cgroup
> old_memcg = pc->mem_cgroup
> TestSetPageDirty(page)
> move_unlock_mem_cgroup
> move_lock_mem_cgroup
> if (PageDirty)
>
> old_memcg->nr_dirty --
>
> new_memcg->nr_dirty ++
>
> pc->mem_cgroup = new_memcgy
> move_unlock_mem_cgroup
>
> old_memcg->nr_dirty ++
>
I'm sorry I couldn't catch why you call TestSetPageDirty()....and what CPUA/CPUB is
doing ? CPUA calls move_account() and CPUB updates stat ? If so, why move_account()
is allowed to set PG_dirty ??
>
> So nr_dirty of old_memcg may be minus in a very short
> period('old_memcg->nr_dirty --' by CPU B), but it will be revised soon
> by CPU A. And the final figures of memcg->nr_dirty is correct.
It seems both of new_memcg and old_memcg has an account for a page. Is it correct ?
> Meanwhile the move_lock only protect saving old_memcg and
> TestSetPageDirty in its critical section and without any irrelevant
> logic, so the lock order or deadlock can be handled easily.
>
> But I'm not sure whether I've lost some race conditions, any comments
> are welcomed. : )
>
Sorry I couldn't understand.
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