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=-11.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=ham 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 79100C433DF for ; Wed, 12 Aug 2020 11:44:02 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 386552080C for ; Wed, 12 Aug 2020 11:44:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 386552080C 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 A7F5E8D0003; Wed, 12 Aug 2020 07:44:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A31648D0001; Wed, 12 Aug 2020 07:44:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 91D538D0003; Wed, 12 Aug 2020 07:44:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0148.hostedemail.com [216.40.44.148]) by kanga.kvack.org (Postfix) with ESMTP id 7A0738D0001 for ; Wed, 12 Aug 2020 07:44:01 -0400 (EDT) Received: from smtpin10.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id DB6EE180AD807 for ; Wed, 12 Aug 2020 11:44:00 +0000 (UTC) X-FDA: 77141732640.10.bear96_100d04d26feb Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin10.hostedemail.com (Postfix) with ESMTP id 9D33C16A4AD for ; Wed, 12 Aug 2020 11:44:00 +0000 (UTC) X-HE-Tag: bear96_100d04d26feb X-Filterd-Recvd-Size: 11547 Received: from out30-45.freemail.mail.aliyun.com (out30-45.freemail.mail.aliyun.com [115.124.30.45]) by imf47.hostedemail.com (Postfix) with ESMTP for ; Wed, 12 Aug 2020 11:43:58 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R191e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04407;MF=alex.shi@linux.alibaba.com;NM=1;PH=DS;RN=19;SR=0;TI=SMTPD_---0U5Z4q4C_1597232629; Received: from IT-FVFX43SYHV2H.local(mailfrom:alex.shi@linux.alibaba.com fp:SMTPD_---0U5Z4q4C_1597232629) by smtp.aliyun-inc.com(127.0.0.1); Wed, 12 Aug 2020 19:43:50 +0800 Subject: Re: [PATCH v17 14/21] mm/compaction: do page isolation first in compaction To: Alexander Duyck Cc: Andrew Morton , Mel Gorman , Tejun Heo , Hugh Dickins , Konstantin Khlebnikov , Daniel Jordan , Yang Shi , Matthew Wilcox , Johannes Weiner , kbuild test robot , linux-mm , LKML , cgroups@vger.kernel.org, Shakeel Butt , Joonsoo Kim , Wei Yang , "Kirill A. Shutemov" , Rong Chen References: <1595681998-19193-1-git-send-email-alex.shi@linux.alibaba.com> <1595681998-19193-15-git-send-email-alex.shi@linux.alibaba.com> <241ca157-104f-4f0d-7d5b-de394443788d@linux.alibaba.com> <8dbd004e-8eba-f1ec-a5eb-5dc551978936@linux.alibaba.com> From: Alex Shi Message-ID: <9581db48-cef3-788a-7f5a-8548fee56c13@linux.alibaba.com> Date: Wed, 12 Aug 2020 19:43:17 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 X-Rspamd-Queue-Id: 9D33C16A4AD X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam02 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/8/11 =E4=B8=8B=E5=8D=8810:47, Alexander Duyck =E5=86=99=E9= =81=93: > On Tue, Aug 11, 2020 at 1:23 AM Alex Shi w= rote: >> >> >> >> =E5=9C=A8 2020/8/10 =E4=B8=8B=E5=8D=8810:41, Alexander Duyck =E5=86=99= =E9=81=93: >>> On Mon, Aug 10, 2020 at 6:10 AM Alex Shi = wrote: >>>> >>>> >>>> >>>> =E5=9C=A8 2020/8/7 =E4=B8=8B=E5=8D=8810:51, Alexander Duyck =E5=86=99= =E9=81=93: >>>>> I wonder if this entire section shouldn't be restructured. This is = the >>>>> only spot I can see where we are resetting the LRU flag instead of >>>>> pulling the page from the LRU list with the lock held. Looking over >>>>> the code it seems like something like that should be possible. I am >>>>> not sure the LRU lock is really protecting us in either the >>>>> PageCompound check nor the skip bits. It seems like holding a >>>>> reference on the page should prevent it from switching between >>>>> compound or not, and the skip bits are per pageblock with the LRU b= its >>>>> being per node/memcg which I would think implies that we could have >>>>> multiple LRU locks that could apply to a single skip bit. >>>> >>>> Hi Alexander, >>>> >>>> I don't find problem yet on compound or skip bit usage. Would you cl= arify the >>>> issue do you concerned? >>>> >>>> Thanks! >>> >>> The point I was getting at is that the LRU lock is being used to >>> protect these and with your changes I don't think that makes sense >>> anymore. >>> >>> The skip bits are per-pageblock bits. With your change the LRU lock i= s >>> now per memcg first and then per node. As such I do not believe it >>> really provides any sort of exclusive access to the skip bits. I stil= l >>> have to look into this more, but it seems like you need a lock per >>> either section or zone that can be used to protect those bits and dea= l >>> with this sooner rather than waiting until you have found an LRU page= . >>> The one part that is confusing though is that the definition of the >>> skip bits seems to call out that they are a hint since they are not >>> protected by a lock, but that is exactly what has been happening here= . >>> >> >> The skip bits are safe here, since even it race with other skip action= , >> It will still skip out. The skip action is try not to compaction too m= uch, >> not a exclusive action needs avoid race. >=20 > That would be the case if it didn't have the impact that they > currently do on the compaction process. What I am getting at is that a > race was introduced when you placed this test between the clearing of > the LRU flag and the actual pulling of the page from the LRU list. So > if you tested the skip bits before clearing the LRU flag then I would > be okay with the code, however because it is triggering an abort after Hi Alexander, Thanks a lot for comments and suggestions! I have tried your suggestion: Signed-off-by: Alex Shi --- mm/compaction.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/mm/compaction.c b/mm/compaction.c index b99c96c4862d..6c881dee8c9a 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -988,6 +988,13 @@ static bool too_many_isolated(pg_data_t *pgdat) if (__isolate_lru_page_prepare(page, isolate_mode) !=3D 0) goto isolate_fail_put; + /* Try get exclusive access under lock */ + if (!skip_updated) { + skip_updated =3D true; + if (test_and_set_skip(cc, page, low_pfn)) + goto isolate_fail_put; + } + /* Try isolate the page */ if (!TestClearPageLRU(page)) goto isolate_fail_put; @@ -1006,13 +1013,6 @@ static bool too_many_isolated(pg_data_t *pgdat) lruvec_memcg_debug(lruvec, page); - /* Try get exclusive access under lock */ - if (!skip_updated) { - skip_updated =3D true; - if (test_and_set_skip(cc, page, low_pfn)) - goto isolate_abort; - } - /* * Page become compound since the non-locked check, * and it's on LRU. It can only be a THP so the order -- Performance of case-lru-file-mmap-read in vm-scalibity is dropped a bit. = not helpful > the LRU flag is cleared then you are creating a situation where > multiple processes will be stomping all over each other as you can > have each thread essentially take a page via the LRU flag, but only > one thread will process a page and it could skip over all other pages > that preemptively had their LRU flag cleared. It increase a bit crowd here, but lru_lock do reduce some them, and skip_= bit could stop each other in a array check(bitmap). So compare to whole node=20 lru_lock, the net profit is clear in patch 17. >=20 > If you take a look at the test_and_set_skip the function only acts on > the pageblock aligned PFN for a given range. WIth the changes you have > in place now that would mean that only one thread would ever actually > call this function anyway since the first PFN would take the LRU flag > so no other thread could follow through and test or set the bit as Is this good for only one process could do test_and_set_skip? is that=20 the 'skip' meaning to be? > well. The expectation before was that all threads would encounter this > test and either proceed after setting the bit for the first PFN or > abort after testing the first PFN. With you changes only the first > thread actually runs this test and then it and the others will likely > encounter multiple failures as they are all clearing LRU bits > simultaneously and tripping each other up. That is why the skip bit > must have a test and set done before you even get to the point of > clearing the LRU flag. It make the things warse in my machine, would you like to have a try by y= ourself? >=20 >>> The point I was getting at with the PageCompound check is that instea= d >>> of needing the LRU lock you should be able to look at PageCompound as >>> soon as you call get_page_unless_zero() and preempt the need to set >>> the LRU bit again. Instead of trying to rely on the LRU lock to >>> guarantee that the page hasn't been merged you could just rely on the >>> fact that you are holding a reference to it so it isn't going to >>> switch between being compound or order 0 since it cannot be freed. It >>> spoils the idea I originally had of combining the logic for >>> get_page_unless_zero and TestClearPageLRU into a single function, but >>> the advantage is you aren't clearing the LRU flag unless you are >>> actually going to pull the page from the LRU list. >> >> Sorry, I still can not follow you here. Compound code part is unchange= d >> and follow the original logical. So would you like to pose a new code = to >> see if its works? >=20 > No there are significant changes as you reordered all of the > operations. Prior to your change the LRU bit was checked, but not > cleared before testing for PageCompound. Now you are clearing it > before you are testing if it is a compound page. So if compaction is > running we will be seeing the pages in the LRU stay put, but the > compound bit flickering off and on if the compound page is encountered > with the wrong or NULL lruvec. What I was suggesting is that the The lruvec could be wrong or NULL here, that is the base stone of whole patchset. > PageCompound test probably doesn't need to be concerned with the lock > after your changes. You could test it after you call > get_page_unless_zero() and before you call > __isolate_lru_page_prepare(). Instead of relying on the LRU lock to > protect us from the page switching between compound and not we would > be relying on the fact that we are holding a reference to the page so > it should not be freed and transition between compound or not. >=20 I have tried the patch as your suggested, it has no clear help on perform= ance on above vm-scaliblity case. Maybe it's due to we checked the same thing before lock already. diff --git a/mm/compaction.c b/mm/compaction.c index b99c96c4862d..cf2ac5148001 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -985,6 +985,16 @@ static bool too_many_isolated(pg_data_t *pgdat) if (unlikely(!get_page_unless_zero(page))) goto isolate_fail; + /* + * Page become compound since the non-locked check, + * and it's on LRU. It can only be a THP so the order + * is safe to read and it's 0 for tail pages. + */ + if (unlikely(PageCompound(page) && !cc->alloc_contig)) { + low_pfn +=3D compound_nr(page) - 1; + goto isolate_fail_put; + } + if (__isolate_lru_page_prepare(page, isolate_mode) !=3D 0) goto isolate_fail_put; @@ -1013,16 +1023,6 @@ static bool too_many_isolated(pg_data_t *pgdat) goto isolate_abort; } - /* - * Page become compound since the non-locked check, - * and it's on LRU. It can only be a THP so the order - * is safe to read and it's 0 for tail pages. - */ - if (unlikely(PageCompound(page) && !cc->alloc_contig)) { - low_pfn +=3D compound_nr(page) - 1; - SetPageLRU(page); - goto isolate_fail_put; - } } else rcu_read_unlock(); Thanks Alex