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 364C3C27C75 for ; Wed, 12 Jun 2024 07:23:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9E4FB6B0180; Wed, 12 Jun 2024 03:23:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 96DC76B0181; Wed, 12 Jun 2024 03:23:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7C0E36B0182; Wed, 12 Jun 2024 03:23:46 -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 5BD2A6B0180 for ; Wed, 12 Jun 2024 03:23:46 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id D988B1C1BA4 for ; Wed, 12 Jun 2024 07:23:45 +0000 (UTC) X-FDA: 82221396810.18.CBB4055 Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by imf17.hostedemail.com (Postfix) with ESMTP id D9C814000E for ; Wed, 12 Jun 2024 07:23:43 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="ZY4K/2JP"; spf=pass (imf17.hostedemail.com: domain of richard.weiyang@gmail.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=richard.weiyang@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718177024; h=from:from:sender:reply-to: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=zzoZ2C0guQAGIJY+WDpPhVUMExli5mSXJ8fvKFTkBhw=; b=dMP1xXCdOLSwkSl8kMYePviC2OXnro9AN3Ry86wS3u2YoQNkIeU4jammD4//UjLj+fYNOf y1GCznBG3hQn/WIzelKsgUWcKLc2DFf7zAsyZclgWaodj8QhVIUU1gxGRIiaOEJ7ZxU4eC ZEWCAHvre5uCsF3ert4ox3KQVCepgdk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718177024; a=rsa-sha256; cv=none; b=r941hLEaYijA4nNehePgs48/0StkvVElxS+74ZanGmDGkOBz0nPhdDib2ThJ/ZwJKzCH+Q 7RuUwfLy+/l4hCjiHSxDliwlO9I1XCAM/aCML0DshFQdw4Kk0TURL756SYg5PPFexhn2FK Xga7bNQL0WeciNqUmsKZhSAZtfbfPrI= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="ZY4K/2JP"; spf=pass (imf17.hostedemail.com: domain of richard.weiyang@gmail.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=richard.weiyang@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ed1-f54.google.com with SMTP id 4fb4d7f45d1cf-579fa270e53so3034642a12.3 for ; Wed, 12 Jun 2024 00:23:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1718177022; x=1718781822; darn=kvack.org; h=user-agent:in-reply-to:content-disposition:mime-version:references :reply-to:message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=zzoZ2C0guQAGIJY+WDpPhVUMExli5mSXJ8fvKFTkBhw=; b=ZY4K/2JPWkAvrWXlJYCWbPeUIiMVszBO0hmb5usViVj4BY012BAR4IREKu+nTjl1pe Z2lUbpenKQyirBhEAVV22y7fyqC605riNNzP4gv1YHO+5+FN9Xf17Vk12a70Vt+vh+EP iEDY0tZtNAlsuD1NsZiG8RveXKmTApDWPGPl4eG5h23beZ6XFlIx4L6a8Z/4nWSQIAai vXbl86xfXjBoFKFn4fbXDBbEebCXz7FBOpONeOQf9hnnLZNOgk8w7X/5BPQYCJfCkcG6 Qo3ef9RJPvpiyVoBRLNcKW5CigNWLyyMnzYSusvmROVTS0YpLVoyBvnM4hEEUEeG9Exp Wcyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718177022; x=1718781822; h=user-agent:in-reply-to:content-disposition:mime-version:references :reply-to:message-id:subject:cc:to:from:date:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=zzoZ2C0guQAGIJY+WDpPhVUMExli5mSXJ8fvKFTkBhw=; b=EY+gLv8ucvTzb9R+AdvMaJ5WhO/9nfkDrLTYq7yuc1M+hUw2KGUzPm6Q0XM4y4KEVJ VCP43u/Kkcp+rlHn6o6LfnOEL9IxATYn/JBGrGBWZ6vPL4+GU1xC3hbk3MgQRE206HVe 9hIT5BZNFsr6UEHSE9HxhfOguhks2xQ4/cb3E/rQI1S4GaBXBZcuB3WarEB4E3t5Ryf2 6DgkPD+dKVOn7hpQDIbnzMMWZJ6E5jfp0sPhp3oRiH/9ecQaqqB2cm+1wD4Oy0Bi9IMP tUvsj8eNGPCSeqdNF+eEeCjO7+JFv7i/MnuSXZP97dlPofn0W8ntfdO5IfASxL21qhB3 vQ1w== X-Forwarded-Encrypted: i=1; AJvYcCUriKKn4s+YkxNixkEaUjWky55KxEfYG8RtHzaeSX9QpzGklYby4DAxFq57+rsh/JxZR8iHCDueA9lN8MxHHu3k9y4= X-Gm-Message-State: AOJu0Yx0JJqun+hhL0/l4K2lcHkRpVxdDNeSGf1+8kLrAy2RhLjPvr7E DE531p4n2/lnI4HH/G98d50DRaxGLeYg3ZkTjSZbOCjH16pBjDVz X-Google-Smtp-Source: AGHT+IGNIbOiKuBi8XBaHBBWZFuuSUm5/fxYrK1eNRgIcIp/HAFQQAj2B1ndW70Hhj705NIJRAC8gQ== X-Received: by 2002:a50:99d9:0:b0:57c:610a:6e85 with SMTP id 4fb4d7f45d1cf-57caa9bc77emr933866a12.27.1718177022047; Wed, 12 Jun 2024 00:23:42 -0700 (PDT) Received: from localhost ([185.92.221.13]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-57c7156e6c2sm6912641a12.9.2024.06.12.00.23.40 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Wed, 12 Jun 2024 00:23:41 -0700 (PDT) Date: Wed, 12 Jun 2024 07:23:40 +0000 From: Wei Yang To: Steven Rostedt Cc: Wei Yang , 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: <20240612072340.hsdxusmszixebvrt@master> Reply-To: Wei Yang References: <20240606150143.876469296@goodmis.org> <20240606150316.751642266@goodmis.org> <20240611144029.h7egl4aif5mjlrwf@master> <20240611111218.71e57e0f@gandalf.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240611111218.71e57e0f@gandalf.local.home> User-Agent: NeoMutt/20170113 (1.7.2) X-Rspamd-Server: rspam03 X-Stat-Signature: mdbhfxuszathfwq9e4j6zjtcx4b9owgt X-Rspamd-Queue-Id: D9C814000E X-Rspam-User: X-HE-Tag: 1718177023-184310 X-HE-Meta: U2FsdGVkX1+6Q5qM6mYghRMSm6rDkBckvTSn4NnsOu2RJhd1QJ5TAI3FHipiTDmU1gmKuj8pgFZSHYlPwmoWOfKqu1GuGNWBrEE4tVAJcb7Xuxu0KWn7wntdn9uJdtCzTC4Br3hOUNBBPCz1dzrnhT9FMatuPQ2B2JGV/8RyQssx83+2MB70dYsP+6xt4QBqiqUKP/3h/yNvndOdbtzyLGDtO/d9muoPFiWvVqpfhiI65KErvlP5DWuB0pwH3ywFNOGUO1pGkEUAjnwd9x8+DU1Ly2EqQMGOdMZhCOBd6qML11XrHsiGMUiAEMPE2lHU8oSrXJAK+/Gp8fA3PPs7184cbwRFoL14WeoewO7IAbwA0oN35FmiU/eMzO01w4yEcilr1cm1CoHOC96L+iPMkVhwbjHy4YoWIzCc/yVDzDwcZkBPkmYqyH+1pQJnUa+NTlEYeKHp9OhzMQylMFfIycJiYGIZBbYYak5cdy8qrtbBW8eQAM8/SHYcRXjx8xd9Avpyv4GB4q6kappBAtYTE3tU9rvurGVsJqQA05OhApbufIGQW7iy4xXkTIo47s67tc+UpncH0BSEGQdrKCJD0X4iopOmK5uzDjJjN2EQoADGOd7vpDxAPCp5IzWNlTFnuXlqB1rQ0eFdF1IxO/3zhY0XA14XE1bxRnb0xBUtq4jI9uJhwvYb8GpRmomP4sE1GOfgRlAaXHW70BHUkK+W1E7FiZ61MziA1NnHK/QWMdGagBzeR4dTSVh0j0KDU6gktyUWsiECZb9sR82OIlSzDfcBtj3uyQaWS8EFlS68+VqA5XQg58RQgD7eoKnKAD+q47nmE5QuW8KIt1PeAWKhfk/GtHdsaFj8axkKyBK1fKQ+HnJwRSIfUjhceWvwOFQLgPYvhS55JGm2SG6gr5Ef+y7qPovOn7CZzF25pvpUumLTGhckqPTG4224HDzlRkvkhF1wdMgoF0Syq1cIgxo EVKIGgH0 jmzPlhomkI0jsPgSCpY3LHceK0Em7GMJQ+GoENE1cR8lMA/HmxL4fgtd7DrKp59RG0xvdpgDPEu0+kL+ctrqjOHHXy6VwTasM/XNyPZFaE3JnYjSjaO8q9hMaJKincUIdbc94qI9wxGPVog3OuoY2cAS/x5dIXSHymiKzbP+H7A0AhfIb332ahkwdVfAUOSov9QeT75kgi13R+PElDgeeVwdUolYu9S10l3X+Uxi6mVV9DiTlEWTnrKeAh/LOvASrasEjXBMPzcMmcz57w2t24kQ4r1G5Kwx0HJHpBPjJrod/lvXElxWCqexJuIoaKEkGsbGoGPYAisWuo5ahpeBpyq2Vi+gwoYaf2GXGOZj3AtQOrDL45sI7zbbBVMCgqPvc/Q7sp75phmp9kSY2FloWSe2lg8v3Wvh6xYCvN9+JehaX8tnCdKKyshc9P1jc0ece0vuXm6TYVyPSipR6YCLR4KB+7vDsjMMQoyEL0SDsaGIJbQaauWzrlqCZz7RWT40ITydIsq0BU/PBFb/U9/HX1UQZ3KQolnpYHt+ncdMokIBWNb8NfK/p50G4CrclP9mszRVs/yu/AAWm2Gg8r5i75nJ0aC9V5dUI+zQdRslm8ylNq8KYKfW5GBuidkYaby4fUGMyFEghfQX/3s4asDtavg6yKkSCT3MC1KbVbz1wUFyIRYdXeiYENKcRZzLxMbz+fLM1h7fhbfwz9Yw= 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, Jun 11, 2024 at 11:12:18AM -0400, Steven Rostedt wrote: >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. > Both start and size. When you look at the definition of memblock_region, both are defined as 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(). >> >> >+ >> >+ 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. >> >+ 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. >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(). >> >> >+ 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 >> > >> > >> -- Wei Yang Help you, Help me