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 75884C369DC for ; Sun, 4 May 2025 07:15:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F29976B0085; Sun, 4 May 2025 03:15:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EB0FA6B0088; Sun, 4 May 2025 03:15:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D4FB66B0089; Sun, 4 May 2025 03:15:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id B1B6E6B0085 for ; Sun, 4 May 2025 03:15:49 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 0F6551D082F for ; Sun, 4 May 2025 07:15:51 +0000 (UTC) X-FDA: 83404365702.08.A2365CA Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf28.hostedemail.com (Postfix) with ESMTP id 3E0DDC0005 for ; Sun, 4 May 2025 07:15:49 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=korg header.b=2uXzKB5t; dmarc=none; spf=pass (imf28.hostedemail.com: domain of akpm@linux-foundation.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=akpm@linux-foundation.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1746342949; 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:dkim-signature; bh=8gZwBHvVko7p9H2zFhC/+sE9AQYoEmvbnEXk1DLS2A0=; b=QF0dPzcue8T/xVU3rhYGrsiaHLwibqn2VQvDkpDgzJqhomXJ4CpLKp3FxedvXbHGapHkfT 1CyIMKjRh/jZHNNrLU/3xiP475x225SHgw22aizc5RFapvjJqlu5U2VK7UJcoQf5fZ1NZo pIP80UL5o/jUlgVcet9ke7QQ4AcfvMU= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=korg header.b=2uXzKB5t; dmarc=none; spf=pass (imf28.hostedemail.com: domain of akpm@linux-foundation.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=akpm@linux-foundation.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1746342949; a=rsa-sha256; cv=none; b=Z5GtHurgwRzrcLjG+n2UdMjWHZfAUkt29oujTe+5/rD6xUUYDR1Ssfccem863WXvsFzdSt hn25rtaAP4n+oqBwV0a3RAvP4ONaTzWw0MdGVSjGRAyRmT41plNodN4qSqtWfoiXFsMCo8 KM+jMS0mk8ZnY6B7duWVrbVsvrrudeg= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id C6CF2A4C040; Sun, 4 May 2025 07:10:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DF4ACC4CEE7; Sun, 4 May 2025 07:15:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linux-foundation.org; s=korg; t=1746342948; bh=VFl4En76y+OpLSujTCsVixWtJLfVBo8Fs18pWu67NIM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=2uXzKB5tvtLWjn/EIY9A1H8ngwh4gxek1y6s8EfSOlGHERXtBEhbis3cIKTA95NjD yjN42znkaYuXQtcC5AV7PmRQ432nN/JjnMJEbs5NSR1otBH6CPfHyYBriZnESpnGbC /m0ml7+nmhQfFTbiTpnqkt55ZUJbHolqqTwmGm24= Date: Sun, 4 May 2025 00:15:47 -0700 From: Andrew Morton To: David Hildenbrand Cc: Petr =?UTF-8?Q?Van=C4=9Bk?= , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Ryan Roberts , xen-devel@lists.xenproject.org, x86@kernel.org, stable@vger.kernel.org Subject: Re: [PATCH v2 1/1] mm: fix folio_pte_batch() on XEN PV Message-Id: <20250504001547.177b2aba8c2ffbfe63e0552e@linux-foundation.org> In-Reply-To: <9e3fb101-9a5d-43bb-924a-0df3c38333f8@redhat.com> References: <20250502215019.822-1-arkamar@atlas.cz> <20250502215019.822-2-arkamar@atlas.cz> <20250503182858.5a02729fcffd6d4723afcfc2@linux-foundation.org> <9e3fb101-9a5d-43bb-924a-0df3c38333f8@redhat.com> X-Mailer: Sylpheed 3.8.0beta1 (GTK+ 2.24.33; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Stat-Signature: zxt57eyb9jeo6gbc5swbdtinpykm8yfz X-Rspamd-Queue-Id: 3E0DDC0005 X-Rspam-User: X-Rspamd-Server: rspam05 X-HE-Tag: 1746342949-12575 X-HE-Meta: U2FsdGVkX19TD2rATVAdK/IUbYK4MS4dLSCNvghW19VS+6j9+tXPIz71yNPVL/Cnz5z5cL6VsdHAH681wFqTMQmQAMxFj6+OV9AZSR99jshSQQfZoaoVYJpFJ34KW5TQD8eqCaxCnsfoOhcGP/8Ii2NwFFXijP4za+zSSiji398em/7ANVCvdCUZBntKKCv7sR3+ASd1LqV1LFniBkdyrFN0C8sPqnPn8miPl4k8p94Uydn72+DP4kj0aHYJ/httA7P03L6ODqBKo02fMmLjsTLC9zTzGKOev8+MByjTfOT95RGAbzpRYwKbYErM4hNSyST3tlPl9Z4BerkTc4Aqij8YkybaZ+L/n1MCvQeB44xjjaZCL/Lv/bWv7kGFafA7sNDVRKE1OCy42te+NH1zOJvUF4WUdiqZQ+bQ6x5ejNwhJYK1qMJnjVMGtAUXT6S5QhERrgsUtsAlNbylm2Z+7CGuAKPzQG9pL1O6d1XFMkCIqUxnfY09OhN1WGjnauDGv1ht/amresq80iBOC+NDAjs8Nq5Qe3eHzwvqniCM7Zxc7EhyM71g0/dVGil4QUu7hny/BGIxk0ldhDFa53mMI/RChq7LkJ07mmoL7/M1tpmfqG+5B8NG9sK8hXY4AGfBzplSw2RDcQNRUOPae139Fw2BZvApoqS7ppSsiMrlrjv5MQHR5+XmrvifpGRMur5xV1FMkrB/nYShqQGiMsom0CpsGkphr8sGiyltcVy4GWFTJr3n59eAFYO49Z6vjr+gaIlZNw9PYGP3CKaBpLeG+VBEH5JUsvCVo9mGyKbZcipGaANgKOs2clLEr4qlVv0eN2L3nFF8ppJj2yakNK/aHVQIKPjm7aKfVvMVAESsbxE6SNPNquI37fkH/vUpMj1KK2v1LdS6a1DI5WKM8hQyVCvnluZ2DoLx9XZS+pb+1Z5/+Dwi4DnsTeZi5nbfTFFVNywjy1huq0uOuGoBkNP vZmaNqpY vFAehOkmcmTPjDxFtWWww4krCQHTXOtS1P4RjAX452icU+qAVHk5iE1pVtnPsQk5VjoaZ9CqHsCFNtYeqe7pg33sP9/+DV5TtpJ9349VhUpUkyTimtD8ksssOJ3vy1dBqRgp682S8SBU4vazD/A53EUkvYHgu7kcWwkzh61jEit+qkoPlCJus8B2GwoLt2W4HyTSYAitT9gyNcg0dIptkufK3dalhQiXpRW2VJ8PEFlcXEgrv/Z73+0kE4FRuPIFENqb+0VCCm6K4nxrDmuHXyAdWziymcjtPcEhCCCuQm8JYtGV5AaVA2+6E39HdB2zDqYx645VoNUzaK1BBifJ+qeyu1Q== 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: List-Subscribe: List-Unsubscribe: On Sun, 4 May 2025 08:47:45 +0200 David Hildenbrand wrote: > > > > Methinks max_nr really wants to be unsigned long. > > We only batch within a single PTE table, so an integer was sufficient. > > The unsigned value is the result of a discussion with Ryan regarding similar/related > (rmap) functions: > > " > Personally I'd go with signed int (since > that's what all the counters in struct folio that we are manipulating are, > underneath the atomic_t) then check that nr_pages > 0 in > __folio_rmap_sanity_checks(). > " > > https://lore.kernel.org/linux-mm/20231204142146.91437-14-david@redhat.com/T/#ma0bfff0102f0f2391dfa94aa22a8b7219b92c957 > > As soon as we let "max_nr" be an "unsigned long", also the return value > should be an "unsigned long", and everybody calling that function. > > In this case here, we should likely just use whatever type "max_nr" is. > > Not sure myself if we should change that here to unsigned long or long. Some > callers also operate with the negative values IIRC (e.g., adjust the RSS by doing -= nr). "rss -= nr" doesn't require, expect or anticipate that `nr' can be negative! > > > That will permit the > > cleanup of quite a bit of truncation, extension, signedness conversion > > and general type chaos in folio_pte_batch()'s various callers. > > > And... > > > > Why does folio_nr_pages() return a signed quantity? It's a count. > > A partial answer is in 1ea5212aed068 ("mm: factor out large folio handling > from folio_nr_pages() into folio_large_nr_pages()"), where I stumbled over the > reason for a signed value myself and at least made the other > functions be consistent with folio_nr_pages(): > > " > While at it, let's consistently return a "long" value from all these > similar functions. Note that we cannot use "unsigned int" (even though > _folio_nr_pages is of that type), because it would break some callers that > do stuff like "-folio_nr_pages()". Both "int" or "unsigned long" would > work as well. > > " > > Note that folio_nr_pages() returned a "long" since the very beginning. Probably using > a signed value for consistency because also mapcounts / refcounts are all signed. Geeze. Can we step back and look at what we're doing? Anything which counts something (eg, has "nr" in the identifier) cannot be negative. It's that damn "int" thing. I think it was always a mistake that the C language's go-to type is a signed one. It's a system programming language and system software rarely deals with negative scalars. Signed scalars are the rare case. I do expect that the code in and around here would be cleaner and more reliable if we were to do a careful expunging of inappropriately signed variables. > > > > > And why the heck is folio_pte_batch() inlined? It's larger then my > > first hard disk and it has five callsites! > > :) > > In case of fork/zap we really want it inlined because > > (1) We want to optimize out all of the unnecessary checks we added for other users > > (2) Zap/fork code is very sensitive to function call overhead > > Probably, as that function sees more widespread use, we might want a > non-inlined variant that can be used in places where performance doesn't > matter all that much (although I am not sure there will be that many). a quick test. before: text data bss dec hex filename 12380 470 0 12850 3232 mm/madvise.o 52975 2689 24 55688 d988 mm/memory.o 25305 1448 2096 28849 70b1 mm/mempolicy.o 8573 924 4 9501 251d mm/mlock.o 20950 5864 16 26830 68ce mm/rmap.o (120183) after: text data bss dec hex filename 11916 470 0 12386 3062 mm/madvise.o 52990 2697 24 55711 d99f mm/memory.o 25161 1448 2096 28705 7021 mm/mempolicy.o 8381 924 4 9309 245d mm/mlock.o 20806 5864 16 26686 683e mm/rmap.o (119254) so uninlining saves a kilobyte of text - less than I expected but almost 1%. Quite a lot of the inlines in internal.h could do with having a critical eye upon them.