From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx131.postini.com [74.125.245.131]) by kanga.kvack.org (Postfix) with SMTP id 779006B0070 for ; Tue, 14 May 2013 04:35:20 -0400 (EDT) Received: by mail-bk0-f47.google.com with SMTP id jg9so130427bkc.6 for ; Tue, 14 May 2013 01:35:18 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20130513131251.GB5246@dhcp22.suse.cz> References: <1368421410-4795-1-git-send-email-handai.szj@taobao.com> <1368421545-4974-1-git-send-email-handai.szj@taobao.com> <20130513131251.GB5246@dhcp22.suse.cz> Date: Tue, 14 May 2013 16:35:18 +0800 Message-ID: Subject: Re: [PATCH V2 3/3] memcg: simplify lock of memcg page stat account From: Sha Zhengju Content-Type: multipart/alternative; boundary=485b3970d29a635bcc04dca9836c Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko Cc: Cgroups , "linux-mm@kvack.org" , KAMEZAWA Hiroyuki , Andrew Morton , Hugh Dickins , Greg Thelen , Sha Zhengju --485b3970d29a635bcc04dca9836c Content-Type: text/plain; charset=ISO-8859-1 Hi Michal, Thank you for reviewing the patch from your busy work! On Mon, May 13, 2013 at 9:12 PM, Michal Hocko wrote: > On Mon 13-05-13 13:05:44, Sha Zhengju wrote: >> From: Sha Zhengju >> >> 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. There's no problem with FILE_MAPPED accounting, it will be serialized by page table lock. > > 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 --485b3970d29a635bcc04dca9836c Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

Hi Michal,

Thank you for reviewing the pat= ch from your busy work!

On Mon, May 13, 2013 at 9:12 PM, Michal Hock= o <mhocko@suse.cz> wrote:
&g= t; On Mon 13-05-13 13:05:44, Sha Zhengju wrote:
>> From: Sha Zhengju <han= dai.szj@taobao.com>
>>
>> After removing duplicate= d information like PCG_* flags in
>> 'struct page_cgroup'(= commit 2ff76f1193), there's a problem between
>> "move" and "page stat accounting"(only FILE_MA= PPED 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 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0CPU-B
>> TestSet PG_dirty
>&g= t; (delay) =A0 =A0 =A0 =A0 =A0 =A0 =A0 move_lock_mem_cgroup()
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (PageDirty(page= )) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0old_memcg->nr_dirty --
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0new_memcg->nr_dirty++
>> =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pc->mem_cgroup = =3D new_memcg;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = move_unlock_mem_cgroup()
>>
>> move_lock_mem_cgroup()
= >> memcg =3D pc->mem_cgroup
>> memcg->nr_dirty++
>> move_unlock_mem_cgroup()
>>
>> while accounting = information of new_memcg may be double-counted. So we
>> use a big= ger lock to solve this problem: =A0(commit: 89c06bd52f)
>>
>= > =A0 =A0 =A0 move_lock_mem_cgroup() <-- mem_cgroup_begin_update_page= _stat()
>> =A0 =A0 =A0 TestSetPageDirty(page)
>> =A0 =A0 =A0 update = page stats (without any checks)
>> =A0 =A0 =A0 move_unlock_mem_cgr= oup() <-- 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()), bu= t the lock
>> granularity is a little bigger that not only the cri= tical section but
>> also some code logic is in the range of locking which may be deadl= ock
>> prone. While trying to add memcg dirty page accounting, it = gets into
>> further difficulty with page cache radix-tree lock an= d 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 lo= ck nesting problem is longer possible as s390/mm has
>> reworked it out(commit:abf09bed), but it should be better
>&= gt; if we can make the lock simpler and recursive safe.
>
> Thi= s patch doesn't make the charge move locking recursive safe. It
> just tries to overcome the problem in the path where it doesn't ex= ist
> 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 includ= es 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_be= gin_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 prob= lem, 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 w= ork 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 t= o be honest...

> particular path because atomic_inc_and_test(_mapcount) will protec= t from
> double accounting but the whole dance around old_memcg seems= pointless
> to me.

There's no problem with FILE_MAP= PED accounting, it will be serialized by page table lock.

>
> I am sorry but I do not think this is the right appro= ach. IMO we should
> focus on mem_cgroup_begin_update_page_stat and m= ake it really recursive
> safe - ideally without any additional overh= ead (which sounds like a real
> challenge)
>
> [...]
> --
> Michal Hocko
&g= t; SUSE Labs



Thanks,
Sha
--485b3970d29a635bcc04dca9836c-- -- 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