Re: [PATCH V3 4/8] memcg: add per cgroup dirty pages accounting

[Sha Zhengju <handai.szj@gmail.com>]

On Mon, Jan 7, 2013 at 3:25 PM, Kamezawa Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> (2013/01/05 13:48), Sha Zhengju wrote:
>>
>> On Wed, Jan 2, 2013 at 6:44 PM, Michal Hocko <mhocko@suse.cz> wrote:
>>>
>>> On Wed 26-12-12 01:26:07, Sha Zhengju wrote:
>>>>
>>>> From: Sha Zhengju <handai.szj@taobao.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 ++


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.
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. : )


--
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>