From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 016F2C33CAD for ; Mon, 13 Jan 2020 12:49:11 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id C10342081E for ; Mon, 13 Jan 2020 12:49:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C10342081E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 60E2C8E0005; Mon, 13 Jan 2020 07:49:10 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5E5BC8E0003; Mon, 13 Jan 2020 07:49:10 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4FB2E8E0005; Mon, 13 Jan 2020 07:49:10 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0123.hostedemail.com [216.40.44.123]) by kanga.kvack.org (Postfix) with ESMTP id 3C08D8E0003 for ; Mon, 13 Jan 2020 07:49:10 -0500 (EST) Received: from smtpin26.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with SMTP id B51993D19 for ; Mon, 13 Jan 2020 12:49:09 +0000 (UTC) X-FDA: 76372591218.26.day29_6cdf64c318c2c X-HE-Tag: day29_6cdf64c318c2c X-Filterd-Recvd-Size: 5634 Received: from out30-56.freemail.mail.aliyun.com (out30-56.freemail.mail.aliyun.com [115.124.30.56]) by imf23.hostedemail.com (Postfix) with ESMTP for ; Mon, 13 Jan 2020 12:49:07 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R181e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01f04427;MF=alex.shi@linux.alibaba.com;NM=1;PH=DS;RN=15;SR=0;TI=SMTPD_---0Tne44Ij_1578919732; Received: from IT-FVFX43SYHV2H.local(mailfrom:alex.shi@linux.alibaba.com fp:SMTPD_---0Tne44Ij_1578919732) by smtp.aliyun-inc.com(127.0.0.1); Mon, 13 Jan 2020 20:48:53 +0800 Subject: Re: [PATCH v7 02/10] mm/memcg: fold lru_lock in lock_page_lru To: Konstantin Khlebnikov , cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, mgorman@techsingularity.net, tj@kernel.org, hughd@google.com, daniel.m.jordan@oracle.com, yang.shi@linux.alibaba.com, willy@infradead.org, shakeelb@google.com, hannes@cmpxchg.org Cc: Michal Hocko , Vladimir Davydov References: <1577264666-246071-1-git-send-email-alex.shi@linux.alibaba.com> <1577264666-246071-3-git-send-email-alex.shi@linux.alibaba.com> <36d7e390-a3d1-908c-d181-4a9e9c8d3d98@yandex-team.ru> <952d02c2-8aa5-40bb-88bb-c43dee65c8bc@linux.alibaba.com> <2ba8a04e-d8e0-1d50-addc-dbe1b4d8e0f1@yandex-team.ru> From: Alex Shi Message-ID: Date: Mon, 13 Jan 2020 20:47:25 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <2ba8a04e-d8e0-1d50-addc-dbe1b4d8e0f1@yandex-team.ru> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: =E5=9C=A8 2020/1/13 =E4=B8=8B=E5=8D=885:55, Konstantin Khlebnikov =E5=86=99= =E9=81=93: >>>> >>>> index c5b5f74cfd4d..0ad10caabc3d 100644 >>>> --- a/mm/memcontrol.c >>>> +++ b/mm/memcontrol.c >>>> @@ -2572,12 +2572,11 @@ static void cancel_charge(struct mem_cgroup = *memcg, unsigned int nr_pages) >>>> =C2=A0=C2=A0 =C2=A0 static void lock_page_lru(struct page *page, int= *isolated) >>>> =C2=A0=C2=A0 { >>>> -=C2=A0=C2=A0=C2=A0 pg_data_t *pgdat =3D page_pgdat(page); >>>> - >>>> -=C2=A0=C2=A0=C2=A0 spin_lock_irq(&pgdat->lru_lock); >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (PageLRU(page)) { >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pg_data_t *pgdat =3D pag= e_pgdat(page); >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct = lruvec *lruvec; >>>> =C2=A0=C2=A0 +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 spin_lock_i= rq(&pgdat->lru_lock); >>> >>> That's wrong. Here PageLRU must be checked again under lru_lock. >> Hi, Konstantin, >> >> For logical remain, we can get the lock and then release for !PageLRU. >> but I still can figure out the problem scenario. Would like to give mo= re hints? >=20 > That's trivial race: page could be isolated from lru between >=20 > if (PageLRU(page)) > and > spin_lock_irq(&pgdat->lru_lock); yes, it could be a problem. guess the following change could helpful: I will update it in new version. Thanks a lot! Alex -static void lock_page_lru(struct page *page, int *isolated) -{ - pg_data_t *pgdat =3D page_pgdat(page); - - spin_lock_irq(&pgdat->lru_lock); - if (PageLRU(page)) { - struct lruvec *lruvec; - - lruvec =3D mem_cgroup_page_lruvec(page, pgdat); - ClearPageLRU(page); - del_page_from_lru_list(page, lruvec, page_lru(page)); - *isolated =3D 1; - } else - *isolated =3D 0; -} - -static void unlock_page_lru(struct page *page, int isolated) -{ - pg_data_t *pgdat =3D page_pgdat(page); - - if (isolated) { - struct lruvec *lruvec; - - lruvec =3D mem_cgroup_page_lruvec(page, pgdat); - VM_BUG_ON_PAGE(PageLRU(page), page); - SetPageLRU(page); - add_page_to_lru_list(page, lruvec, page_lru(page)); - } - spin_unlock_irq(&pgdat->lru_lock); -} - static void commit_charge(struct page *page, struct mem_cgroup *memcg, bool lrucare) { - int isolated; + struct lruvec *lruvec =3D NULL; VM_BUG_ON_PAGE(page->mem_cgroup, page); @@ -2612,8 +2617,16 @@ static void commit_charge(struct page *page, struc= t mem_cgroup *memcg, * In some cases, SwapCache and FUSE(splice_buf->radixtree), the = page * may already be on some other mem_cgroup's LRU. Take care of i= t. */ - if (lrucare) - lock_page_lru(page, &isolated); + if (lrucare) { + lruvec =3D lock_page_lruvec_irq(page); + if (likely(PageLRU(page))) { + ClearPageLRU(page); + del_page_from_lru_list(page, lruvec, page_lru(pag= e)); + } else { + unlock_page_lruvec_irq(lruvec); + lruvec =3D NULL; + } + } /* * Nobody should be changing or seriously looking at @@ -2631,8 +2644,15 @@ static void commit_charge(struct page *page, struc= t mem_cgroup *memcg, */ page->mem_cgroup =3D memcg; - if (lrucare) - unlock_page_lru(page, isolated); + if (lrucare && lruvec) { + unlock_page_lruvec_irq(lruvec); + lruvec =3D lock_page_lruvec_irq(page); + + VM_BUG_ON_PAGE(PageLRU(page), page); + SetPageLRU(page); + add_page_to_lru_list(page, lruvec, page_lru(page)); + unlock_page_lruvec_irq(lruvec); + } }