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 E2952C0015E for ; Fri, 28 Jul 2023 09:00:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 51CE88D0001; Fri, 28 Jul 2023 05:00:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4CD206B0074; Fri, 28 Jul 2023 05:00:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 394B68D0001; Fri, 28 Jul 2023 05:00:28 -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 274C46B0072 for ; Fri, 28 Jul 2023 05:00:28 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id E40A61408E6 for ; Fri, 28 Jul 2023 09:00:27 +0000 (UTC) X-FDA: 81060424494.29.4AFECAA Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf17.hostedemail.com (Postfix) with ESMTP id CDC154002E for ; Fri, 28 Jul 2023 09:00:25 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=none; spf=pass (imf17.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1690534826; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rnAsgMILAtDi0kyf+w3kWqpB1Q75byCHrtNBX6IF+oQ=; b=WW5RMUIBl+k0ksP9dJL91PevcEiWdBAw6eN0IBdlJsTTCtpY4iG8r0+C3Z4aYLoVVQZhwx sE8IF0xRBqS6LZwfFommGfvl7wW4E5nIXbeD/qh/IQF5j7GptAQxwEZbni/SHPeL7zPnP5 oLesuZVvXlplb9Br9dO1fDpPuB92/H4= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=none; spf=pass (imf17.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1690534826; a=rsa-sha256; cv=none; b=g14mvhjY+0TOTlUqsYpFzd9vwh4NJJ+4/7wwI5S3FlLwJuK0NC2xlwi3+eu/vrfFSLJ9UY muQPuqMwJ3vhjiNWe8RHBSgMRAmQp9BGUJB1+gCqD3WbXJZao2zTtvKiGnS/tJzqWxz2KU oIHUONRVkCW5RpnV74EC6Jk5S26f7us= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BDCB32F4; Fri, 28 Jul 2023 02:01:07 -0700 (PDT) Received: from [10.57.77.173] (unknown [10.57.77.173]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 240D23F67D; Fri, 28 Jul 2023 02:00:23 -0700 (PDT) Message-ID: <3c26aa4e-fe11-09d2-c2fb-63546ba80893@arm.com> Date: Fri, 28 Jul 2023 10:00:21 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH v3 2/3] mm: Implement folio_remove_rmap_range() To: Yu Zhao Cc: Matthew Wilcox , "Huang, Ying" , Andrew Morton , Yin Fengwei , David Hildenbrand , Yang Shi , Zi Yan , linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20230720112955.643283-1-ryan.roberts@arm.com> <20230720112955.643283-3-ryan.roberts@arm.com> <87r0ouw39n.fsf@yhuang6-desk2.ccr.corp.intel.com> From: Ryan Roberts In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: CDC154002E X-Rspam-User: X-Stat-Signature: ktyjpxc4mxm69kzszqjwpd3hpmjwf6kp X-Rspamd-Server: rspam01 X-HE-Tag: 1690534825-404278 X-HE-Meta: U2FsdGVkX1/dJVhPVoB1J5d1+kqcXcM9urTEhiSNOVR27FynukUzyIp0wD+wqUGahPQImMx7tzF0vjpvTlXWQ2l81+maDvo8k20fveUD/qrj08U3yhzWc7Rz46S7NQhRe3sx7//V1/DK0Kor79zfum6r0Y62EblKmscDytFuzttmdBL+TwT0KNB8pKODluPFYV4H2UNzOM0wEOyeN6tH7rdh2cqpWEIWiZKyD0AFXj8Yec8UPdn2moYkj4QwWWdkBWkqi9SzQ17YwIAJ2dd2qFugNPPxqy3NrBs1hRFYmbjp531oAizB59r539Pm7yk9FX5bHbkEHfqpyRLfJrWK+ginqTN5seY+aduzEnQwIWePvrcK2BxnNAp8jdQ/WoPfqgX5aRasgLzVsTCXsa/R8s2yKGcYRPREeQ5bcliStG5BijV594Bne73L2SgugXEEpswqE6NCxyyaa3bPnM2DDfConMNVOMq7tSiKSWg1edEKpGsiHyDX3z2CMM8E/DOU7fS60ZNyfxiozDpGUjqyHkqgamif2/mjjSLF8doyyMbJGmKh6f1qqrhI+X5lGRdI7fqDKU+jSt61EQQuCDCyg+vTrnNCgS6gjuImtPScGeaE0j2+VeLod7xYdQPK0U5HVsHTiF79RbAqK5SmiAnhkcbMJLBqIeHWJpeM4vmyuV2glFg9F5rZcw0nqzJBt17O/iJC39Md7QR5iw3rCjDHF1pUVPUSkF29KBRA+VePq7nuf3UjQ2NdjAu83WlDRDwjR3G9KX9eYnsvNSY8D7Ly735cpVAbCUakLPFsAXDTRY+t+f14MYSA5eevSNZwyX+bFJZjnpaZyrvt4z1PC29cDVD172Q3SQY01asqKZq9sWcWlKt6/KR7n7gITO1xmBUTC88kQbgLHEepJ2xq4S3QScf8QvXzbbkOkyfmZE2RAQ9zLGjI31cqMCYBUOSIpu69ae2lkFrIOSrV2J1sn9f qgG6D/0B k8JXDeOf49lm63wzeoU5Etgz2uG2/0dfRgkmMjh0P2J5U4w+Bt1u8sbIXNoTdfFBq6cHmuVCCg4aPIjfeWp8huZzMABAh3Em5HyIgSop2LOqZ+/C/QgrKclM0Id030gHXYUE0d3Gp9UCbKHbzQuGGxXgNg2kDeoBm4QkHPPHDJL6ytfLaGuk1pAh9J9IB7eWRSmINiJp+QHLB0lznTmNihdUl0Mp5a+Eq2P4+h9E18/BWWZ7B9qkziuKFf9ohbXSFjI4N4FNwf6z41Q3TIrCudPn/MA== 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 27/07/2023 17:38, Yu Zhao wrote: > On Thu, Jul 27, 2023 at 1:26 AM Ryan Roberts wrote: >> >> On 27/07/2023 03:35, Matthew Wilcox wrote: >>> On Thu, Jul 27, 2023 at 09:29:24AM +0800, Huang, Ying wrote: >>>> Matthew Wilcox writes: >>>>> I think that can make sense. Because we limit to a single page table, >>>>> specifying 'nr = 1 << PMD_ORDER' is the same as 'compound = true'. >>>>> Just make it folio, page, nr, vma. I'd actually prefer it as (vma, >>>>> folio, page, nr), but that isn't the convention we've had in rmap up >>>>> until now. >>>> >>>> IIUC, even if 'nr = 1 << PMD_ORDER', we may remove one PMD 'compound' >>>> mapping, or 'nr' PTE mapping. So, we will still need 'compound' (or >>>> some better name) as parameter. >>> >>> Oh, this is removing ... so you're concerned with the case where we've >>> split the PMD into PTEs, but all the PTEs are still present in a single >>> page table? OK, I don't have a good answer to that. Maybe that torpedoes >>> the whole idea; I'll think about it. >> >> This is exactly why I think the approach I've already taken is the correct one; >> a 'range' makes no sense when you are dealing with 'compound' pages because you >> are accounting the entire folio. So surely its better to reflect that by only >> accounting small pages in the range version of the API. > > If the argument is the compound case is a separate one, then why not a > separate API for it? > > I don't really care about whether we think 'range' makes sense for > 'compound' or not. What I'm saying is: > 1. if they are considered one general case, then one API with the > compound parameter. > 2. if they are considered two specific cases, there should be two APIs. > This common design pattern is cleaner IMO. Option 2 definitely makes sense to me and I agree that it would be cleaner to have 2 separate APIs, one for small-page accounting (which can accept a range within a folio) and one for large-page accounting (i.e. compound=true in today's API). But... 1) That's not how the rest of the rmap API does it 2) This would be a much bigger change since I'm removing an existing API and replacing it with a completely new one (there are ~20 call sites to fix up). I was trying to keep the change small and manageable by maintaining the current API but moving all the small-page logic to the new API, so the old API is a wrapper in that case. 3) You would also need an API for the hugetlb case, which page_remove_rmap() handles today. Perhaps that could also be done by the new API that handles the compound case. But then you are mixing and matching your API styles - one caters for 1 specific case, and the other caters for 2 cases and figures out which one. > > Right now we have an overlap (redundancy) -- people would have to do > two code searches: one for page_remove_rmap() and the other for > folio_remove_rmap_range(nr=1), and this IMO is a bad design pattern. I'm open to doing the work to remove this redundancy, but I'd like to hear concensus on this thread that its the right approach first. Although personally I don't see a problem with what I've already done; If you want to operate on a page (inc the old concept of a "compound page" and a hugetlb page) call the old one. If you want to operate on a range of pages in a folio, call the new one. Thanks, Ryan