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 CB039C27C75 for ; Tue, 11 Jun 2024 15:12:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 60B3E6B0083; Tue, 11 Jun 2024 11:12:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 593B26B009E; Tue, 11 Jun 2024 11:12:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 40C4E6B00A4; Tue, 11 Jun 2024 11:12:10 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 2237A6B0083 for ; Tue, 11 Jun 2024 11:12:10 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id D052FA164F for ; Tue, 11 Jun 2024 15:12:09 +0000 (UTC) X-FDA: 82218948378.15.8F0D20D Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf04.hostedemail.com (Postfix) with ESMTP id 0A2E140009 for ; Tue, 11 Jun 2024 15:12:07 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=none; spf=pass (imf04.hostedemail.com: domain of "SRS0=hOOK=NN=goodmis.org=rostedt@kernel.org" designates 139.178.84.217 as permitted sender) smtp.mailfrom="SRS0=hOOK=NN=goodmis.org=rostedt@kernel.org"; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718118728; a=rsa-sha256; cv=none; b=ykg0re3Ov8l7H7UqykDPslo+OY5nrummSMR2UdBPZVHO6+OVmiFVRmEnS1pFVLLBOHK9iA ecPTVc5HbPvgF9s3DWMhsnKDuR1iSR19840fhuGeqQKxPQ0JCgB/JUeLlX5h9t2o+gj56b gsh/ldm+pH5lf7OaJcrfm+yKjHA2suI= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=none; spf=pass (imf04.hostedemail.com: domain of "SRS0=hOOK=NN=goodmis.org=rostedt@kernel.org" designates 139.178.84.217 as permitted sender) smtp.mailfrom="SRS0=hOOK=NN=goodmis.org=rostedt@kernel.org"; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718118728; 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=QJXqUroZHHFG+pzzjHxat/GHykPE18ON3KIJq6B3rMo=; b=W2T8GMUVp6Y8Y6jbdEBn/BPMPDc2bYg0+3OymEVNnnSD55tNIktRqckDeDPyMrNrS5locn 2jy35aLdX7vXrzkgY0fidPSBWZouCJeTdKy1+dnt2Vbo8IUSdYuYwmQOBZ1G1NjPowi0bT 7RsYs7JYnvoRiJu67zBo/TNd54m7YLE= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 0FDD560ED5; Tue, 11 Jun 2024 15:12:07 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 68D9BC2BD10; Tue, 11 Jun 2024 15:12:03 +0000 (UTC) Date: Tue, 11 Jun 2024 11:12:18 -0400 From: Steven Rostedt To: Wei Yang Cc: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, Masami Hiramatsu , Mark Rutland , Mathieu Desnoyers , Andrew Morton , "Liam R. Howlett" , Vlastimil Babka , Lorenzo Stoakes , linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , Peter Zijlstra , Kees Cook , Tony Luck , "Guilherme G. Piccoli" , linux-hardening@vger.kernel.org, Guenter Roeck , Ross Zwisler , wklin@google.com, Vineeth Remanan Pillai , Joel Fernandes , Suleiman Souhlal , Linus Torvalds , Catalin Marinas , Will Deacon , Ard Biesheuvel , Mike Rapoport Subject: Re: [PATCH v2 1/2] mm/memblock: Add "reserve_mem" to reserved named memory at boot up Message-ID: <20240611111218.71e57e0f@gandalf.local.home> In-Reply-To: <20240611144029.h7egl4aif5mjlrwf@master> References: <20240606150143.876469296@goodmis.org> <20240606150316.751642266@goodmis.org> <20240611144029.h7egl4aif5mjlrwf@master> X-Mailer: Claws Mail 3.20.0git84 (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-Rspamd-Queue-Id: 0A2E140009 X-Rspam-User: X-Rspamd-Server: rspam12 X-Stat-Signature: 3wya5b39ecsff4ea9ncupdgkfunxxgqt X-HE-Tag: 1718118727-839233 X-HE-Meta: U2FsdGVkX1/iDSVHq1a/mPKlx1jx43zxajlS8kkYOuUn/trP1ZCTahrDvCinWODLmbgseMWBh2ZLcrpvNKSKPhg5llnK1ttjiBP8EsJkFgq0zWJU+PykbpDSBl9FFyGIWLhdG9Lyvi/mWkcd0wi047ypll4U9sZ4wi9TZi15CvQpWcPsdtxsnMoXLKCJPLyjr5F74THiLKcGdEWSGiXTx78S/nwvEiuJzkNnPKasgnFLxpVxgcknaNc+EYSzwB013CUuMBIjRwxt9jQMxP3KShoosORVVa4O+iz7BsK5y9uEt4jAKdhsNxh9zkRRX4EP0t8O+2pK+pwa/dcaEmAXMlpew1zJNeoqsLGEwckYbYv25IY4BYn9Cc63lW42zUTTHgY1RIxWN35snNzTz1WjAASHuXnzmLX9Ec5OmXy4BFQCtZX6dWFWquNbjvaKc5dt5WT1vVFJjSUfUC8/6wKqUEv4vSNpyCWJBRxr80aAQoremC3sU+7V0J5gOHMlrv8UaNS4KMKsVsNKcnxbrU319UiYTxEu6z/Ed0XaIXliSDvcUvNFcyouizPO6a+xzYPSnU9qt+LRIx9IfRU9GK+Otrm0YMe1KL8yB5g9rR8M4vX1vP/3zuznHTIZc7CljFhNEkWwwqEv/PUCYREzP7ZAXYvTugiBvr9vTUaisZ+lhQw6tzkwVRbjBbuo1bQp3PiOzuQ7ZR7Ru94Cv+1X6POHCSwdJrIAsSKCNTyjiMwRiVWlDEq+r3eEFfSzxbnt56/bFxp2YWHCpTZgLEvKD9ZV4eGI47/3cxlVyc+K+abE3m/DN6DFwjJBq2VMLX6BvQeEav0d63hlKYsBERyWGPVCgNA1SLEtkX+X6CfcxkQLEVA+z54bssXysJKIgAqp8EBD3agepoCgDWTaCVKKX7HJL6VwOyRQXZH495tuDPjBsjw5PTmREIaSbhYQmCpEUODuqcp+m4Alu0lRyvIFbc1 T6cSufwk a8gV62uS8wRHRHLTJBW2/nTdtEXV2EQaduc/XVflFeHUMBC93L9KEm1NvuBLbUf6Qnb/gnJfnMaqFQzyVjXVD/w1O63j2IebwdyS3mraTtaOQ/Hxh+x3FZFtbVgyucTvl05WXI8KiPXaUxyy5bdJ+nzWF+UPss1v9gH+FmN8uyvXt1FDGh0FFQpVILy5wiscCHy8LCtOFuCL3YQ7WSrC9SxqE62P7NLZLxrIf+87mh3SKR8j9y4h97xIDeY97RugWoFCkV5gxMQ+UIj2p2TMMGfaiX/eXcUBfLmiW/qH+cwwoVLspfA4liEjmSg/ygLElLs+5d8b5wUCrFHMqbKwnqK7weXb+RpfPci9FGH3T+ieW6GmuZB4P8JxMF/+HBnYoKamih8prGQ4ps1k= 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 Tue, 11 Jun 2024 14:40:29 +0000 Wei Yang wrote: Missed this just before sending out v3 :-p > >diff --git a/mm/memblock.c b/mm/memblock.c > >index d09136e040d3..a8bf0ee9e2b4 100644 > >--- a/mm/memblock.c > >+++ b/mm/memblock.c > >@@ -2244,6 +2244,103 @@ void __init memblock_free_all(void) > > totalram_pages_add(pages); > > } > > > >+/* Keep a table to reserve named memory */ > >+#define RESERVE_MEM_MAX_ENTRIES 8 > >+#define RESERVE_MEM_NAME_SIZE 16 > ^ > Suggest to align with previous line. It is. But because the patch adds a "+", it pushed the "8" out another tab. > > >+struct reserve_mem_table { > >+ char name[RESERVE_MEM_NAME_SIZE]; > >+ unsigned long start; > >+ unsigned long size; > > phys_addr_t looks more precise? For just the start variable, correct? I'm OK with updating that. > > >+}; > >+static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES]; > >+static int reserved_mem_count; > > Seems no matter we use this feature or not, these memory would be occupied? Yes, because allocation may screw it up as well. I could add a CONFIG around it, so that those that do not want this could configure it out. But since it's just a total of (16 + 8 + 8) * 8 = 256 bytes, I'm not sure it's much of a worry to add the complexities to save that much space. As the code to save it may likely be bigger. > > >+ > >+/* Add wildcard region with a lookup name */ > >+static int __init reserved_mem_add(unsigned long start, unsigned long size, > >+ const char *name) > >+{ > >+ struct reserve_mem_table *map; > >+ > >+ if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE) > >+ return -EINVAL; > >+ > >+ if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES) > >+ return -1; > > return ENOSPC? Not good at it, but a raw value maybe not a good practice. This is what gets returned by the command line parser. It only cares if it is zero or not. > > Also, we'd better do this check before allocation. What allocation? > > >+ > >+ map = &reserved_mem_table[reserved_mem_count++]; > >+ map->start = start; > >+ map->size = size; > >+ strscpy(map->name, name); > >+ return 0; > >+} > >+ > >+/** > >+ * reserve_mem_find_by_name - Find reserved memory region with a given name > >+ * @name: The name that is attached to a reserved memory region > >+ * @start: If found, holds the start address > >+ * @size: If found, holds the size of the address. > >+ * > >+ * Returns: 1 if found or 0 if not found. > >+ */ > >+int reserve_mem_find_by_name(const char *name, unsigned long *start, unsigned long *size) > >+{ > >+ struct reserve_mem_table *map; > >+ int i; > >+ > >+ for (i = 0; i < reserved_mem_count; i++) { > >+ map = &reserved_mem_table[i]; > >+ if (!map->size) > >+ continue; > >+ if (strcmp(name, map->name) == 0) { > >+ *start = map->start; > >+ *size = map->size; > >+ return 1; > >+ } > >+ } > >+ return 0; > >+} > >+ > >+/* > >+ * Parse early_reserve_mem=nn:align:name > > early_reserve_mem or reserve_mem ? Oops, that was the original name. I'll change that. > > >+ */ > >+static int __init reserve_mem(char *p) > >+{ > >+ phys_addr_t start, size, align; Hmm, I wonder if I should change size and align to unsigned long? > >+ char *oldp; > >+ int err; > >+ > >+ if (!p) > >+ return -EINVAL; > >+ > >+ oldp = p; > >+ size = memparse(p, &p); > >+ if (p == oldp) > >+ return -EINVAL; > >+ > >+ if (*p != ':') > >+ return -EINVAL; > >+ > >+ align = memparse(p+1, &p); > >+ if (*p != ':') > >+ return -EINVAL; > >+ > > Better to check if the name is valid here. You mean that it has text and is not blank? > > Make sure command line parameters are valid before doing the allocation. You mean that size is non zero? I don't know if we care what the align is. Zero is valid. > > >+ start = memblock_phys_alloc(size, align); > >+ if (!start) > >+ return -ENOMEM; > >+ > >+ p++; > >+ err = reserved_mem_add(start, size, p); > >+ if (err) { > >+ memblock_phys_free(start, size); > >+ return err; > >+ } > >+ > >+ p += strlen(p); > >+ > >+ return *p == '\0' ? 0: -EINVAL; > > We won't free the memory if return -EINVAL? I guess we can do this check before the allocation, like you suggested. Thanks for the review. -- Steve > > >+} > >+__setup("reserve_mem=", reserve_mem); > >+ > > #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK) > > static const char * const flagname[] = { > > [ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG", > >-- > >2.43.0 > > > > >