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 94B25C27C7B for ; Wed, 12 Jun 2024 15:33:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2DE5D6B009C; Wed, 12 Jun 2024 11:33:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 28E606B009D; Wed, 12 Jun 2024 11:33:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 17D9F6B009E; Wed, 12 Jun 2024 11:33:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id ECF806B009C for ; Wed, 12 Jun 2024 11:33:04 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 960561A182A for ; Wed, 12 Jun 2024 15:33:04 +0000 (UTC) X-FDA: 82222629888.09.3E8063A Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf21.hostedemail.com (Postfix) with ESMTP id 2E4AC1C0021 for ; Wed, 12 Jun 2024 15:33:01 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of "SRS0=9b5n=NO=goodmis.org=rostedt@kernel.org" designates 145.40.73.55 as permitted sender) smtp.mailfrom="SRS0=9b5n=NO=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=1718206382; 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=qneIliC3htWM9rc0XE31ZI/SPOym3cdiFgZihJTIgAI=; b=du3Vnm3OSKkBRZrn3CPdn8QT+CMHB4STsMdt/XyRx18G6ZCkpRBeJZWDXtkMiG1AuoSkUF 0p0CoU/qaQ2l5vxGPURd1N2ZrfX/AVDzcb2p+zv7tMzOJzxYFIbM2wYfrYxwfSLn1uDm16 7hVaBDUSSQjxyrcT07fVRHiVerkkgZY= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of "SRS0=9b5n=NO=goodmis.org=rostedt@kernel.org" designates 145.40.73.55 as permitted sender) smtp.mailfrom="SRS0=9b5n=NO=goodmis.org=rostedt@kernel.org"; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718206382; a=rsa-sha256; cv=none; b=8Ek20TA+6XsRCyWBMK5JfdRS1knbpRc5uwkVH44INzpUICpcZpF0kXdIoKEZB5EBHTKHWR r4L530nRl07g7tsMBlB+6tSIgwh817d78SUTEC2IvSUtZGKAQRo5BKXGJWeAxmWq1ABQCX NWKmXAkSHsvl3eaQbgiC27Om37xbGns= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id C0119CE21BE; Wed, 12 Jun 2024 15:32:57 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 214E0C32786; Wed, 12 Jun 2024 15:32:53 +0000 (UTC) Date: Wed, 12 Jun 2024 11:32:51 -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: <20240612113251.43b9c7b9@rorschach.local.home> In-Reply-To: <20240612072340.hsdxusmszixebvrt@master> References: <20240606150143.876469296@goodmis.org> <20240606150316.751642266@goodmis.org> <20240611144029.h7egl4aif5mjlrwf@master> <20240611111218.71e57e0f@gandalf.local.home> <20240612072340.hsdxusmszixebvrt@master> X-Mailer: Claws Mail 3.17.8 (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-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 2E4AC1C0021 X-Stat-Signature: owmz74hcw4s5znjzirdhnsk9xjd6wg95 X-HE-Tag: 1718206381-901589 X-HE-Meta: U2FsdGVkX19hjNzetWd8mMmdHcMGQERA4M9g/SK6pAWiwlO1kHB0LmlFd/gnDhzNUX9SOXEh5mcwgfoY52fMjPbEgI2Lh3C5HbQjpYjt52pBatIAX8wP0PwHtBE4oT+d/hDwMIalSrMbXCkbKuqx6Sj7ogeOx2BIvqJHy0r5J6+xdTTki75BeFL39KjcB7eHW4Au/sD4u6E0KPAViNePuTvFX7rOVrnFi6/OO8UeV+XRh1pRlG4Okf/ZScdnvuRTeWAnPvzSQ4tWJ4EGtDpfMa/iDEd1yqqSnn4waVtpdTyLCjMAELs/pAfs/+sPY384xoj6pjks5EUZQO/AvIfbtXkYsj4kcu6oAuiZvjkiUaLVq5oG6Jn6TKv+cQVVx/b6CWDAVO8cNt4GC6M2S44rVuxVQAwL8wkez5nJ815S/Z6B+sVfvnvEjF/nkEEXXLpZd9i935Ay/7ORIU4NZz/SB2e9nEV+eahN9U7No/uBmYREF6cwwQrW0+uGt3W94mCVaH/qmGQVALPhyy4jI7Dc66+/RmfbBuyvVAAK55abV8z55cRF21oBI/CAwoefIpBDN1P313SXw+XiQeeUmB8vBVrLSW2GuuhaKtxCE9MYUqOVFxS/XsdlG2ORepH0uBtjKGF4ryisNcZD0WXZz6J84TVwO6J+Ys9ueIEyrCx41tTlMt2kHiNHUlYxgOpci/r31xLchMf7DN3Vyrdfo4HeAuXMrtPlPGa0zjh5ebUeHEm+toS4iFb2ZjWyZf8AQ2ccIUtVRLpZWKjbewDcsRx3jzPVAJ+CZMu+YQMZK44BHrXmQUZy3ggv9JeYBRKdz7D8jTLjcOzHcql+q7pXe1lHZp+IdpdxB9SbakHq/+fMHpgz7MJcQTvJ04t7veHIY01TgrCoWPF7nbLDHwMomgauQuFKHXWq5/9W/TJ4hkse8GOxDS3OVoubXGRh4bZB/8wVVHVyT60lGVCwFz9FRXt 5jkQxWL4 cB1xu6/yx8IHMBDKLxLt7jFKBZxtRSgmSmrmt+2b6bHH10cdbsDbTgFVIJWiC8rTYKLztLiSofTqK5ENmJXGWFAo7r+aYh+Iyo4+gCJywoe2jMb2+N3hia75NlWFS4MiT9e4TQYPMJKQoPKVgOvHhGYVHVznbsX44LPHVHKTM22W4XApF5gSpdQ6Mtt3ZKuUtFM5dAdjOOKSc0yjR0V7qnYJ7eBWjBJHRQE5849tRo9Ci/RMLkFf5ACD9ifZFXJfBmLZeRvlDMvsuUCVHjLOJ8j4KZ14WcReZBzUD2bcrT83zDXBwWkNUX9IZ8D9YhL9cvDqtFt6KO3GtEKb8Ozs3kvrTRvqW7XYcNiI7bTN5cp5vbX7fnMdQQjisYWP3f4q53BEEKGJ25JV0Zlg= 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 Wed, 12 Jun 2024 07:23:40 +0000 Wei Yang wrote: > >> >+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. > > > > Both start and size. When you look at the definition of memblock_region, both > are defined as phys_addr_t. I ended up keeping everything phys_addr_t. > > >> > >> >+}; > >> >+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. > > > > If Mike feel good to it, I am ok. > > >> > >> >+ > >> >+/* 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? > > > > You call reserved_mem_add() after memblock_phys_alloc(). > My suggestion is do those sanity check before calling memblock_phys_alloc(). Yeah, I did add more checks before the allocation happens. > > >> > >> >+ > >> >+ 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? > > > > I grep the kernel, some use u64, some use unsigned long. > I think it is ok to use unsigned long here. For consistency, I switched them all to phys_addr_t. > > >> >+ 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 mean do those sanity check before real allocation. Yep, I hope I caught everything (of course I need to check if the name exists first). > > >I don't know if we care what the align is. Zero is valid. > > > > memblock internal would check the alignment. If it is zero, it will change to > SMP_CACHE_BYTES with dump_stack(). I saw that and added: if (align < SMP_CACHE_BYTES) align = SMP_CACHE_BYTES; so that SMP_CACHE_BYTES will be the minimum alignment. Thanks for looking at this. -- Steve