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 2A873C27C6E for ; Fri, 14 Jun 2024 08:22:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 495D06B011A; Fri, 14 Jun 2024 04:22:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 41E786B011F; Fri, 14 Jun 2024 04:22:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2A5986B011B; Fri, 14 Jun 2024 04:22:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 3C34A6B0123 for ; Fri, 14 Jun 2024 04:20:07 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id D730C80567 for ; Fri, 14 Jun 2024 08:20:06 +0000 (UTC) X-FDA: 82228796412.10.B23286F Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf25.hostedemail.com (Postfix) with ESMTP id 8FB42A000B for ; Fri, 14 Jun 2024 08:20:04 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=OsaKF8hx; spf=pass (imf25.hostedemail.com: domain of rppt@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=rppt@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718353204; 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:dkim-signature; bh=b8O+RIYmI1Afk5+FEjW7zvbz7ERMCFC3cDYpaIVVFZ8=; b=pZJIVh5tDyRhJLQmwkB5xJ9I4q3YimBpxgSV4queGKxemJB9e9kmGE23CuVcWvTQi/YibQ FBHHTHfkmMYrmZRbXEJQCrzt5EkwDdsr1sCIDxtOfV3AMahhM6wqTwhNzLsIuFDGj3o0zE Afj1t9byV+BAhvKYl4YKhoaQg1x5Elo= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=OsaKF8hx; spf=pass (imf25.hostedemail.com: domain of rppt@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=rppt@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718353204; a=rsa-sha256; cv=none; b=EzulUa4Y0EcgXFIF3ZbqgeKgLpfHY+crJifNUyV3i74fFogsqXls6UDDCGXxu5C1fJVAVD MGIDZeU+2Z8AcIOU9N8iDhCBHGHAy+6Tja9AiFYlriueQmkJyO6GUCkbl/RBGBmXpksMhk AQ3t33cyhIDVJSk1nXo9bVgKmJljErg= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 38EB9CE28AA; Fri, 14 Jun 2024 08:20:01 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9CA18C2BD10; Fri, 14 Jun 2024 08:19:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1718353200; bh=TfxsNSJanNhH9ZFOQCKMNV6ScMRC1YpRzhWw8GYJeO0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=OsaKF8hxEn4ezvshOUa7jmZq4TwH2/vyh3QvveaZyX2trfbpmpapKC1xpwjtAqu36 Q5wa02/1/cKX9/nlO2xSx5bD6yCnH2AoyiAjNVtOrukBB9qzXU0WtDc4mIhDxLd0/m jUar4GCbOyfc859tCNKgcJ1a6R/aDZPdRe6Qk44zIwgHyu85LmLQQcwdKt67+ze7cE RCXvs4jYaALFRbt+m7TVY2lLePkdTmnZh+6GEtleonUjjdH7roNuSvZJh4t7q54oKK iRoaRzEPwdI1lq8kRLL3zAvJ1J0LaNFhnlFC0eBcUAuwQfW2h2dK+doaV86COI59Eq yOkPNK7ShDU+A== Date: Fri, 14 Jun 2024 11:17:52 +0300 From: Mike Rapoport To: Linus Torvalds Cc: Borislav Petkov , Jan Beulich , Narasimhan V , "Paul E. McKenney" , stable@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [GIT PULL] memblock:fix validation of NUMA coverage Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Stat-Signature: pjdtaztda8b9r7y7oqhd4butouoomgsj X-Rspam-User: X-Rspamd-Queue-Id: 8FB42A000B X-Rspamd-Server: rspam02 X-HE-Tag: 1718353204-551004 X-HE-Meta: U2FsdGVkX18oknoLaxbmjsdeRUSkS7YofQNFRAy4u8y9hFCvT6qxhL7Oup4JqKlAp/QH4gkfdYQnQui2DLpne/n3lGauvxjOwBOtcOY259FwqrV4jtutAw0K8JhNa7f5AA+B3fEN9cAWvNchVyhDeLSSCUB1RbJX07ZlEkz7FbpYQZZC5CY5j0tzCFtveH2dE5B/9nTHrkT7N1ptrsjw1tC18TP1IVpteMLkLHuwDNWKow0M++cNXthpMOAmqAqIF3IwyxR1mcGb/1fcgpbYWIIP0LXDOC56iQPAv5CoE9+E0Y9ZcIamYr6lpfBASnDV/m+hbvGTm40kzh9knh5y7zjvbdS6nBFhlwX/qKOOFiFkf15+2Rg+098TdttR+AOVVwXRSj/tSlYKDVCEdgH0cG320CUCqpLcormGIL+iQF8BL4GoidS04Lb+sLit1Iq7Dv1aSve2gRYiy39wOtw0njJ4Kvysv2oLNUS4iQRFr7Crbl3XvpvuUU0rFie8Z+L8MVfnh8AYKNGxJRBcGsP2btk42+XKj8XRTyoPhn8yuaDgf/AbSzmAA4ePhHva53VXr5LRPnTbzIeomkipodfaeBFQ7s64JHzU5G110J+TjHGEtG27NgRr21/IuDRATv3qLrm5S7YNexQ2OJUQiuhLR0YKvO6/hy/jw0PgfF4MByvo/s0ZrMbt+kr6Mj5oOU+PLDSJSoH6E0tC62AOBEf/K26zOpJzL5ee2RSnwCQa+scRetPItIxE47VxYLIrc6V0yGmqgKTBMq39waic3V8KzY7D0gtr/Kzlca43SJog5363NTqEAVk4WrzS7LP1QD2/ld3+i5FhrMCptfsiCR+3qS8UedLJzhiOWV/BxSwfgBZw7mlfTgb6ruwKiAZTOIyRJuRV5s+9Q3mlOeIjl+PamSyzH32A3QaoM16ODyITN0QXwBGIEWQWj/SyjT/c6SrLXDT5jtiqSzcYHCcLxmW tIZy59jt 6ze8REreNM6TpEf3nc2m3AFD6dLvqR+BAjD8jxn3bw0TtOQ4fyuCtf3AMcy2tFnedlN/3vPqyBPo7ieoAgCDYxno5Rvt+nDM+2m0TUw/MXHNqXRlbkhu/8Kcwp2yIy1Y3DfSOTcChALDmZiQfcjO+gWnGNtdXh/mSU4vAUHLdHy0noYQVlhn/+CXqoLk0T9KYNV4pwVHDKLPHSM43A/yhKxME2jwBHOzkTvUuqSliKpJ8N3nAvN3983xcBiWPWyQMqQ+jhtIIx44006Ex99DUrUNPjVPckv2q9SIjZFGsfPUoeoyDO+HLwoIT6vsOlgN8vY1b9lJHwqS1XBIZtjtvYQpBuw== 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 Thu, Jun 13, 2024 at 10:38:28AM -0700, Linus Torvalds wrote: > On Thu, 13 Jun 2024 at 10:09, Linus Torvalds > wrote: > > > > Is there some broken scripting that people have started using (or have > > been using for a while and was recently broken)? > > ... and then when I actually pull the code, I note that the problem > where it checked _one_ bogus value has just been replaced with > checking _another_ bogus value. > > Christ. > > What if people use a node ID that is simply outside the range > entirely, instead of one of those special node IDs? > > And now for memblock_set_node() you should apparently use NUMA_NO_NODE > to not get a warning, but for memblock_set_region_node() apparently > the right random constant to use is MAX_NUMNODES. > > Does *any* of this make sense? No. > > How about instead of having two random constants - and not having any > range checking that I see - just have *one* random constant for "I > have no range", call that NUMA_NO_NODE, and then have a simple helper > for "do I have a valid range", and make that be > > static inline bool numa_valid_node(int nid) > { return (unsigned int)nid < MAX_NUMNODES; } > > or something like that? Notice that now *all* of > > - NUMA_NO_NODE (explicitly no node) > > - MAX_NUMNODES (randomly used no node) > > - out of range node (who knows wth firmware tables do?) > > will get the same result from that "numa_valid_node()" function. > > And at that point you don't need to care, you don't need to warn, and > you don't need to have these insane rules where "sometimes you *HAVE* > to use NUMA_NO_NODE, or we warn, in other cases MAX_NUMNODES is the > thing". > > Please? IOW, instead of adding a warning for fragile code, then change > some caller to follow the new rules, JUST FIX THE STUPID FRAGILITY! > > Or hey, just do > > #define NUMA_NO_NODE MAX_NUMNODES > > and have two names for the *same* constant, instead fo having two > different constants with strange semantic differences that seem to > make no sense and where the memblock code itself seems to go > back-and-forth on it in different contexts. A single constant is likely to backfire because I remember seeing checks like 'if (nid < 0)' so redefining NUMA_NO_NODE will require auditing all those. But a helper function works great. I could only lightly test it as I don't have a fleet of machines with variety of memory layouts, so I'm planning to push it into -next early next week (with subject replaced by a more informative one) >From 319eddd74b372cae840782c7d53832ab30533a6b Mon Sep 17 00:00:00 2001 From: "Mike Rapoport (IBM)" Date: Fri, 14 Jun 2024 11:05:43 +0300 Subject: [PATCH] memblock: FIX THE STUPID FRAGILITY Introduce numa_valid_node(nid) that verifies that nid is a valid node ID and use that instead of comparing nid parameter with either NUMA_NO_NODE or MAX_NUMNODES. This makes the checks for valid node IDs consistent and more robust and allows to get rid of multiple WARNings. Suggested-by: Linus Torvalds Signed-off-by: Mike Rapoport (IBM) --- include/linux/numa.h | 5 +++++ mm/memblock.c | 28 +++++++--------------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/include/linux/numa.h b/include/linux/numa.h index 1d43371fafd2..eb19503604fe 100644 --- a/include/linux/numa.h +++ b/include/linux/numa.h @@ -15,6 +15,11 @@ #define NUMA_NO_NODE (-1) #define NUMA_NO_MEMBLK (-1) +static inline bool numa_valid_node(int nid) +{ + return nid >= 0 && nid < MAX_NUMNODES; +} + /* optionally keep NUMA memory info available post init */ #ifdef CONFIG_NUMA_KEEP_MEMINFO #define __initdata_or_meminfo diff --git a/mm/memblock.c b/mm/memblock.c index 08e9806b1cf9..e81fb68f7f88 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -754,7 +754,7 @@ bool __init_memblock memblock_validate_numa_coverage(unsigned long threshold_byt /* calculate lose page */ for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) { - if (nid == NUMA_NO_NODE) + if (!numa_valid_node(nid)) nr_pages += end_pfn - start_pfn; } @@ -1061,7 +1061,7 @@ static bool should_skip_region(struct memblock_type *type, return false; /* only memory regions are associated with nodes, check it */ - if (nid != NUMA_NO_NODE && nid != m_nid) + if (numa_valid_node(nid) && nid != m_nid) return true; /* skip hotpluggable memory regions if needed */ @@ -1118,10 +1118,6 @@ void __next_mem_range(u64 *idx, int nid, enum memblock_flags flags, int idx_a = *idx & 0xffffffff; int idx_b = *idx >> 32; - if (WARN_ONCE(nid == MAX_NUMNODES, - "Usage of MAX_NUMNODES is deprecated. Use NUMA_NO_NODE instead\n")) - nid = NUMA_NO_NODE; - for (; idx_a < type_a->cnt; idx_a++) { struct memblock_region *m = &type_a->regions[idx_a]; @@ -1215,9 +1211,6 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid, int idx_a = *idx & 0xffffffff; int idx_b = *idx >> 32; - if (WARN_ONCE(nid == MAX_NUMNODES, "Usage of MAX_NUMNODES is deprecated. Use NUMA_NO_NODE instead\n")) - nid = NUMA_NO_NODE; - if (*idx == (u64)ULLONG_MAX) { idx_a = type_a->cnt - 1; if (type_b != NULL) @@ -1303,7 +1296,7 @@ void __init_memblock __next_mem_pfn_range(int *idx, int nid, if (PFN_UP(r->base) >= PFN_DOWN(r->base + r->size)) continue; - if (nid == MAX_NUMNODES || nid == r_nid) + if (!numa_valid_node(nid) || nid == r_nid) break; } if (*idx >= type->cnt) { @@ -1339,10 +1332,6 @@ int __init_memblock memblock_set_node(phys_addr_t base, phys_addr_t size, int start_rgn, end_rgn; int i, ret; - if (WARN_ONCE(nid == MAX_NUMNODES, - "Usage of MAX_NUMNODES is deprecated. Use NUMA_NO_NODE instead\n")) - nid = NUMA_NO_NODE; - ret = memblock_isolate_range(type, base, size, &start_rgn, &end_rgn); if (ret) return ret; @@ -1452,9 +1441,6 @@ phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, enum memblock_flags flags = choose_memblock_flags(); phys_addr_t found; - if (WARN_ONCE(nid == MAX_NUMNODES, "Usage of MAX_NUMNODES is deprecated. Use NUMA_NO_NODE instead\n")) - nid = NUMA_NO_NODE; - if (!align) { /* Can't use WARNs this early in boot on powerpc */ dump_stack(); @@ -1467,7 +1453,7 @@ phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size, if (found && !memblock_reserve(found, size)) goto done; - if (nid != NUMA_NO_NODE && !exact_nid) { + if (numa_valid_node(nid) && !exact_nid) { found = memblock_find_in_range_node(size, align, start, end, NUMA_NO_NODE, flags); @@ -1987,7 +1973,7 @@ static void __init_memblock memblock_dump(struct memblock_type *type) end = base + size - 1; flags = rgn->flags; #ifdef CONFIG_NUMA - if (memblock_get_region_node(rgn) != MAX_NUMNODES) + if (numa_valid_node(memblock_get_region_node(rgn))) snprintf(nid_buf, sizeof(nid_buf), " on node %d", memblock_get_region_node(rgn)); #endif @@ -2181,7 +2167,7 @@ static void __init memmap_init_reserved_pages(void) start = region->base; end = start + region->size; - if (nid == NUMA_NO_NODE || nid >= MAX_NUMNODES) + if (!numa_valid_node(nid)) nid = early_pfn_to_nid(PFN_DOWN(start)); reserve_bootmem_region(start, end, nid); @@ -2272,7 +2258,7 @@ static int memblock_debug_show(struct seq_file *m, void *private) seq_printf(m, "%4d: ", i); seq_printf(m, "%pa..%pa ", ®->base, &end); - if (nid != MAX_NUMNODES) + if (numa_valid_node(nid)) seq_printf(m, "%4d ", nid); else seq_printf(m, "%4c ", 'x'); -- 2.43.0 > Linus -- Sincerely yours, Mike.