From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BC9752253EB for ; Sat, 14 Mar 2026 00:42:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773448947; cv=none; b=HRAF7G2M+M2I3Et96mWY3IilULNGGOajv8zt8Jk9WYKzew70650+zuyQPeTRz3vG5uHZFY07dtWJfUFCq4q4TqtUbBy1Kcp1h2GF6gScM0X5QrHA7X9F8wCeR3Hpe4samfjB+vw9Y27r1COUtVpPfiGJVL7sP/UvX8dyO67OxIc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773448947; c=relaxed/simple; bh=ePAgfPiEp28gS3ZigJ1iIT61qD0VqK2kCenISk7TpYI=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=fFuoUfUBa9ulDknDVateFjVxe25betZIv6g0g6mLZVq7AwzbUKsBXiA5Ay9ianfiYAvLROZ3KCHdWZjLtM+emKgrGnU3x3tJHXIlaf6dfclbgUGNpxUA4wQT3wbqZKMap3ekPxTF/aJYTR/I3Ro6l9fSEpYfPh+kmKJjcYZbM60= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Z6poEW9+; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Z6poEW9+" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 31BE8C19421; Sat, 14 Mar 2026 00:42:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1773448947; bh=ePAgfPiEp28gS3ZigJ1iIT61qD0VqK2kCenISk7TpYI=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Z6poEW9+snTWcDGfIaO7P/4K9QmzGjMan/mIuzIEISA3IAYJz6TPrG5hKt79TjLao abzFi5WH6g3H9w94w7nsp3QhOCRu+0/lc/DAGZ45b09V4R6QCcKIL99pmG58eR1xeV OGCEAbTU9GNvxb8EsBWUval1ct568Xy7ojYmI8YlvDk3yUDju/EPicjtgwj/Uyl3R0 a299qWcagFUENjKBrzoE4AwqKIIsy982BxJb7ZCdwOaPoWLiqgufl9TJ48lqiTklWW iiVfTsCUhX6hZGZzalkWhGiw8sbDRjH3s2BuL4yNUEouDH8Eb1keQ0fcNWp/EiCusk Qg14FRjvght/Q== From: SeongJae Park To: "Guilherme G. Piccoli" Cc: SeongJae Park , rppt@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-dev@igalia.com, kernel@gpiccoli.net, Andrew Morton , Steven Rostedt Subject: Re: [PATCH V2 2/2] mm/memblock: Add reserve_mem debugfs info Date: Fri, 13 Mar 2026 17:42:23 -0700 Message-ID: <20260314004224.80693-1-sj@kernel.org> X-Mailer: git-send-email 2.47.3 In-Reply-To: <62b3bcca-7e4b-09ce-4996-5b67f066e123@igalia.com> References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit On Fri, 13 Mar 2026 18:09:10 -0300 "Guilherme G. Piccoli" wrote: > Hi SJ and Mike, thanks for both your review! I'll respond the 2 messages > here. > > First regarding Mike's concern from the other email: > > >> +#include > >This will break tests in tools/testing/memblock, please add a stub there. > > Sure, will do it! > > On 04/03/2026 22:44, SeongJae Park wrote: > >[...] > >> There is no easy way to determine if this kernel parameter is properly > >> set though; the kernel doesn't show information about this memory in > >> memblock debugfs, neither in /proc/iomem nor dmesg. This is a relevant > >> information for tools like kdumpst[0], to determine if it's reliable > >> to use the reserved area as ramoops persistent storage; checking only > >> /proc/cmdline is not sufficient as it doesn't tell if the reservation > >> effectively succeeded or not. > > > > Asking out of curiosity. The previous patch has added the error log for > > failure cases. Could checking the kernel log to see if it was failed be an > > option? > > > > I think this debugfs approach is easier to check for the user space, though. > > If that is the reason of this patch, adding the clarity would be nice for a > > theoretical case that debugfs cannot be mounted. > > > > Exactly that! Not only debugfs way is more convenient for checking that, > but it feels a bit weird for me to assume the reservation worked by > checking for errors and if none, all fine. It's valid, but as a matter > of taste, I prefer checking the file. And not only that: imagine we have > more than one setting in the cmdline, for other usages like ftrace. > Easier to have the debugfs showing the succeeding ones, by name and > size, ready to be used heheh Makes sense to me, thank you for kindly clarifying this! :) > > Do you have a suggestion on how should I mention this in the commit > message? Is that discussion enough maybe, for future reference? I feel > mentioning that userspace will use the information from the file seems > enough for a commit message; but I'm totally open for your suggestions > on how to improve the wording here The above clarification on this thread is good enough for me. > > > >> + if (!(IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK) || reserved_mem_count)) > >> + return 0; > > > > One trivial comment. I'd slightly prefer having one less parentheses level as > > a tradeoff of having one more exclamation mark, e.g., > > > > if (!IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK) && !reserved_mem_count) > > > > Good! Unless Mike is against that, I can easily change it. > > > > > > So, we get two level of nested ifdef... I'm wondering if returning earlier > > when CONFIG_ARCH_KEEP_MEMBLOCK is undefined is easier to read. E.g., > > > > #ifndef CONFIG_ARCH_KEEP_MEMBLOCK > > return 0; > > #endif > > > > debugfs_create_file("memory", 0444, root, > > [...] > > > >> return 0; > > > > Very trivial comment. Why don't you keep the original blank line above the > > return statemtnt? > > > > About the ifdefs, as per Mike's comment, I'll rework it in a helper. And > the blank like, I have no answer heh > Likely a braino? I can certainly keep it. Sounds good! > > Thanks again you two for the suggestions, :) Thanks, SJ [...]