Hi Michal,
Thank you for reviewing the patch from your busy work!
On Mon, May 13, 2013 at 9:12 PM, Michal Hocko <mhocko@xxxxxxx> wrote:
> On Mon 13-05-13 13:05:44, Sha Zhengju wrote:
>> From: Sha Zhengju <handai.szj@xxxxxxxxxx>
>>
>> After removing duplicated information like PCG_* flags in
>> 'struct page_cgroup'(commit 2ff76f1193), there's a problem between
>> "move" and "page stat accounting"(only FILE_MAPPED is supported now
>> but other stats will be added in future, and here I'd like to take
>> dirty page as an example):
>>
>> Assume CPU-A does "page stat accounting" and CPU-B does "move"
>>
>> CPU-A CPU-B
>> TestSet PG_dirty
>> (delay) move_lock_mem_cgroup()
>> if (PageDirty(page)) {
>> old_memcg->nr_dirty --
>> new_memcg->nr_dirty++
>> }
>> pc->mem_cgroup = new_memcg;
>> move_unlock_mem_cgroup()
>>
>> move_lock_mem_cgroup()
>> memcg = pc->mem_cgroup
>> memcg->nr_dirty++
>> move_unlock_mem_cgroup()
>>
>> while accounting information of new_memcg may be double-counted. So we
>> use a bigger lock to solve this problem: (commit: 89c06bd52f)
>>
>> move_lock_mem_cgroup() <-- mem_cgroup_begin_update_page_stat()
>> TestSetPageDirty(page)
>> update page stats (without any checks)
>> move_unlock_mem_cgroup() <-- mem_cgroup_begin_update_page_stat()
>>
>>
>> But this method also has its pros and cons: at present we use two layers
>> of lock avoidance(memcg_moving and memcg->moving_account) then spinlock
>> on memcg (see mem_cgroup_begin_update_page_stat()), but the lock
>> granularity is a little bigger that not only the critical section but
>> also some code logic is in the range of locking which may be deadlock
>> prone. While trying to add memcg dirty page accounting, it gets into
>> further difficulty with page cache radix-tree lock and even worse
>> mem_cgroup_begin_update_page_stat() requires nesting
>> (https://lkml.org/lkml/2013/1/2/48). However, when the current patch is
>> preparing, the lock nesting problem is longer possible as s390/mm has
>> reworked it out(commit:abf09bed), but it should be better
>> if we can make the lock simpler and recursive safe.
>
> This patch doesn't make the charge move locking recursive safe. It
> just tries to overcome the problem in the path where it doesn't exist
> anymore. mem_cgroup_begin_update_page_stat would still deadlock if it
> was re-entered.
Referring to deadlock or recursive, I think one of the reasons is that the scope of lock is too large and includes some complicated codes in. So this patch is trying to make lock regions as small as possible to lower possibility of recursion. Yeah, mem_cgroup_begin_update_page_stat still can't re-entered after this patch, but if we can avoid re-enter calls at the very beginning, it can also solve our problem, doesn't it?
>
> It makes PageCgroupUsed usage even more tricky because it uses it out of
> lock_page_cgroup context. It seems that this would work in this
This is why I investigate all those four to find whether using PageCgroupUsed here is race safe... it's really a little trick to be honest...
> particular path because atomic_inc_and_test(_mapcount) will protect from
> double accounting but the whole dance around old_memcg seems pointless
> to me.
>
> I am sorry but I do not think this is the right approach. IMO we should
> focus on mem_cgroup_begin_update_page_stat and make it really recursive
> safe - ideally without any additional overhead (which sounds like a real
> challenge)
>
> [...]
> --
> Michal Hocko
> SUSE Labs
Thanks,
Sha
> I am sorry but I do not think this is the right approach. IMO we should
> focus on mem_cgroup_begin_update_page_stat and make it really recursive
> safe - ideally without any additional overhead (which sounds like a real
> challenge)
>
> [...]
> --
> Michal Hocko
> SUSE Labs
Thanks,
Sha