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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 63E4AC38145 for ; Thu, 8 Sep 2022 08:40:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D18B88D0001; Thu, 8 Sep 2022 04:40:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CC7ED6B0073; Thu, 8 Sep 2022 04:40:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BDE3D8D0001; Thu, 8 Sep 2022 04:40:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id AF3FB6B0072 for ; Thu, 8 Sep 2022 04:40:19 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 796241219B7 for ; Thu, 8 Sep 2022 08:40:19 +0000 (UTC) X-FDA: 79888271358.25.AE06C67 Received: from outbound-smtp47.blacknight.com (outbound-smtp47.blacknight.com [46.22.136.64]) by imf07.hostedemail.com (Postfix) with ESMTP id F134140066 for ; Thu, 8 Sep 2022 08:40:17 +0000 (UTC) Received: from mail.blacknight.com (pemlinmail01.blacknight.ie [81.17.254.10]) by outbound-smtp47.blacknight.com (Postfix) with ESMTPS id 5879DFACDC for ; Thu, 8 Sep 2022 09:40:16 +0100 (IST) Received: (qmail 21798 invoked from network); 8 Sep 2022 08:40:16 -0000 Received: from unknown (HELO techsingularity.net) (mgorman@techsingularity.net@[84.203.198.246]) by 81.17.254.9 with ESMTPSA (AES256-SHA encrypted, authenticated); 8 Sep 2022 08:40:16 -0000 Date: Thu, 8 Sep 2022 09:40:08 +0100 From: Mel Gorman To: Zhenhua Huang Cc: akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Joonsoo Kim Subject: Re: [PATCH] mm/page_owner.c: remove redudant drain_all_pages Message-ID: <20220908084008.tmerssqksyrg3knl@techsingularity.net> References: <1662537673-9392-1-git-send-email-quic_zhenhuah@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <1662537673-9392-1-git-send-email-quic_zhenhuah@quicinc.com> ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662626419; a=rsa-sha256; cv=none; b=Q55n2TMelrT8dM3fJAdpLqBjjO3HDZ+tP4TiM+530apT2Ze1xohw9KtsPsOOHLK5bxHsqD 2pMSXGFyPkZwkp1VIUWomKpQfiNM5zZ0AztOlBviJtt8X4FwYXp7csUGMDTC1wrfEiZqUj BRtrZ2kGqkV0zQNxd7x9hqevUKzX8sU= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=none; spf=pass (imf07.hostedemail.com: domain of mgorman@techsingularity.net designates 46.22.136.64 as permitted sender) smtp.mailfrom=mgorman@techsingularity.net; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662626419; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=nzJzA+nxMXxeNwoxnxg+5h3VujvYaJ9k0ViLz+0cTqM=; b=Mpo1gZF9dagaNFlo9tnJpvkXqLNWUMQUn9usXZE0kuX4a/77FKOSpmnWvwuBlsLE7rwVhN lcW7PrBS3Un3BodtQYdymgxnfMPrx0coy2E43uf+4XHESUcRCJYPeyhpkGVFlDWimDtf45 99BcotD5Um32px4B9bpF48fv/vTcqHI= X-Stat-Signature: no48bnghrfg541543wq1tk5pxx6wj4n8 X-Rspamd-Queue-Id: F134140066 X-Rspamd-Server: rspam11 X-Rspam-User: Authentication-Results: imf07.hostedemail.com; dkim=none; spf=pass (imf07.hostedemail.com: domain of mgorman@techsingularity.net designates 46.22.136.64 as permitted sender) smtp.mailfrom=mgorman@techsingularity.net; dmarc=none X-HE-Tag: 1662626417-939347 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 Wed, Sep 07, 2022 at 04:01:13PM +0800, Zhenhua Huang wrote: > Page owner info of pages in pcp list have already been reset: > free_unref_page > -> free_unref_page_prepare > -> free_pcp_prepare > -> free_pages_prepare which do page owner > reset > -> free_unref_page_commit which add pages into pcp list > It can also be confirmed from dump that page owner info of pcp pages are > correct. Hence there is no more need to drain when reading. > > Signed-off-by: Zhenhua Huang This is subtle because there is no comment explaining why drain_all_pages is called and git history does not help. I agree that the page owner information has already been reset and has been since the very beginning but I do not think that is *why* drain_all_pages is called here. After the drain_all_pages, there is a fairly standard PFN walker with this in it; /* Find an allocated page */ for (; pfn < max_pfn; pfn++) { .... page = pfn_to_page(pfn); if (PageBuddy(page)) { unsigned long freepage_order = buddy_order_unsafe(page); if (freepage_order < MAX_ORDER) pfn += (1UL << freepage_order) - 1; continue; } .... } The PFN walker is trying to skip free pages efficiently and PCP pages are not buddy pages so the order is unknown. The order *can* be known but it's risky to try detecting it. I suspect the drain_all_pages is called to move PCP pages to the buddy list so they can identified as buddy pages and skipped and has nothing to do with resetting the page owner. If that is correct then I think it is overkill to drain the PCP lists to marginally improve the efficiency of the PFN walker and the drain is subject to a race. Just because the PCP lists are drained does not mean a new PCP page will be added during the PFN walk. Furthermore, PCP pages get skipped because PAGE_EXT_OWNER_ALLOCATED is cleared so it's not about scan safety. The drain is a guaranteed expensive operation that is unlikely to be offset by a slight increase in efficiently of the PFN walker when skipping free pages so the drain_all_pages should be dropped. I believe the patch itself is correct but the changelog needs to be changed. With a changelog stating that the patch is removing an expensive and unnecessary operation as PCP pages are safely skipped; Acked-by: Mel Gorman But just in case -- Joonsoo, can you clarify why drain_all_pages was originally called? -- Mel Gorman SUSE Labs