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=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,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 BC2DDC43460 for ; Thu, 15 Apr 2021 11:24:19 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 50624611AB for ; Thu, 15 Apr 2021 11:24:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 50624611AB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B34026B006C; Thu, 15 Apr 2021 07:24:16 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AD0076B0070; Thu, 15 Apr 2021 07:24:16 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 997E76B0071; Thu, 15 Apr 2021 07:24:16 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0039.hostedemail.com [216.40.44.39]) by kanga.kvack.org (Postfix) with ESMTP id 7B8CF6B006C for ; Thu, 15 Apr 2021 07:24:16 -0400 (EDT) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 35A4A181AF5D7 for ; Thu, 15 Apr 2021 11:24:16 +0000 (UTC) X-FDA: 78034367712.20.1BC0D66 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf07.hostedemail.com (Postfix) with ESMTP id 5754BA0000FD for ; Thu, 15 Apr 2021 11:24:15 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 3805AAF35; Thu, 15 Apr 2021 11:24:14 +0000 (UTC) To: Mel Gorman Cc: Linux-MM , Linux-RT-Users , LKML , Chuck Lever , Jesper Dangaard Brouer , Thomas Gleixner , Peter Zijlstra , Ingo Molnar , Michal Hocko References: <20210414133931.4555-1-mgorman@techsingularity.net> <20210414133931.4555-8-mgorman@techsingularity.net> <20210415093337.GH3697@techsingularity.net> From: Vlastimil Babka Subject: Re: [PATCH 07/11] mm/page_alloc: Remove duplicate checks if migratetype should be isolated Message-ID: Date: Thu, 15 Apr 2021 13:24:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <20210415093337.GH3697@techsingularity.net> Content-Type: text/plain; charset=iso-8859-15 Content-Language: en-US X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 5754BA0000FD X-Stat-Signature: dgk14bhf55grxrho7izcjx6d9ins6dhp Received-SPF: none (suse.cz>: No applicable sender policy available) receiver=imf07; identity=mailfrom; envelope-from=""; helo=mx2.suse.de; client-ip=195.135.220.15 X-HE-DKIM-Result: none/none X-HE-Tag: 1618485855-307244 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: On 4/15/21 11:33 AM, Mel Gorman wrote: > On Wed, Apr 14, 2021 at 07:21:42PM +0200, Vlastimil Babka wrote: >> On 4/14/21 3:39 PM, Mel Gorman wrote: >> > Both free_pcppages_bulk() and free_one_page() have very similar >> > checks about whether a page's migratetype has changed under the >> > zone lock. Use a common helper. >> >=20 >> > Signed-off-by: Mel Gorman >>=20 >> Seems like for free_pcppages_bulk() this patch makes it check for each= page on >> the pcplist >> - zone->nr_isolate_pageblock !=3D 0 instead of local bool (the perform= ance might >> be the same I guess on modern cpu though) >> - is_migrate_isolate(migratetype) for a migratetype obtained by >> get_pcppage_migratetype() which cannot be migrate_isolate so the check= is useless. >>=20 >> As such it doesn't seem a worthwhile cleanup to me considering all the= other >> microoptimisations? >>=20 >=20 > The patch was a preparation patch for the rest of the series to avoid c= ode > duplication and to consolidate checks together in one place to determin= e > if they are even correct. >=20 > Until zone_pcp_disable() came along, it was possible to have isolated P= CP > pages in the lists even though zone->nr_isolate_pageblock could be 0 du= ring > memory hot-remove so the split in free_pcppages_bulk was not necessaril= y > correct at all times. >=20 > The remaining problem is alloc_contig_pages, it does not disable > PCPs so both checks are necessary. If that also disabled PCPs > then check_migratetype_isolated could be deleted but the cost to > alloc_contig_pages might be too high. I see. Well that's unfortunate if checking zone->nr_isolate_pageblock is = not sufficient, as it was supposed to be :( But I don't think the check_migratetype_isolated() check was helping in s= uch scenario as it was, anyway. It's testing this: + if (unlikely(has_isolate_pageblock(zone) || + is_migrate_isolate(migratetype))) { In the context of free_one_page(), the 'migratetype' variable holds a val= ue that's read from pageblock in one of the callers of free_one_page(): migratetype =3D get_pcppage_migratetype(page); and because it's read outside of zone lock, it might be a MIGRATE_ISOLATE= even though after we obtain the zone lock, we might find out it's not anymore.= This is explained in commit ad53f92eb416 ("mm/page_alloc: fix incorrect isolat= ion behavior by rechecking migratetype") as scenario 1. However, in the context of free_pcppages_bulk(), the migratetype we are c= hecking in check_migratetype_isolated() is this one: int mt =3D get_pcppage_migratetype(page); That was the one determined while adding the page to pcplist, and is stor= ed in the struct page and we know it's not MIGRATE_ISOLATE otherwise the page w= ould not go to pcplist. But by rechecking this stored value, we would not be f= inding the case where the underlying pageblock's migratetype would change to MIGRATE_ISOLATE, anyway... > I'll delete this patch for now because it's relatively minor and there > should be other ways of keeping the code duplication down. >=20