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 B7595C5B543 for ; Thu, 5 Jun 2025 16:11:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2806C8D0053; Thu, 5 Jun 2025 12:11:35 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2578B8D0007; Thu, 5 Jun 2025 12:11:35 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 194708D0053; Thu, 5 Jun 2025 12:11:35 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id DCB278D0007 for ; Thu, 5 Jun 2025 12:11:34 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 8BA6E80D49 for ; Thu, 5 Jun 2025 16:11:34 +0000 (UTC) X-FDA: 83521837308.06.7396E17 Received: from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31]) by imf12.hostedemail.com (Postfix) with ESMTP id C2C1A40004 for ; Thu, 5 Jun 2025 16:11:32 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="pY/uBsa3"; spf=pass (imf12.hostedemail.com: domain of sj@kernel.org designates 172.234.252.31 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1749139893; 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-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=n56NN40U8L6yF+lUKAeDQpk05xVypOaEsLJzmi3UJdA=; b=lSRdzYlm9fT6Um4dL8uUF5JZsvM1DUOZNb6wh+HblqwKeF5WrDz2dHIpUa3k+KPsK0Xohb GIxG1hAXue97y7s1hbT7xbCl5JUuY2D8NgOvSh/7A57nNI1IhC41DPfJXHJBNa8am1xzSV TkXDuOaRIMdp7z1qKCX3PT+DHBP1qSg= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="pY/uBsa3"; spf=pass (imf12.hostedemail.com: domain of sj@kernel.org designates 172.234.252.31 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1749139893; a=rsa-sha256; cv=none; b=y/ihtCRbgyNxgdPbISq4DnChSt/fTL/DP/h65PNV4Wa0R7lrH6A6y+4ETiP/TKEp5Ic/6r cARQdOtaTgfGV6mC9eRFPpdgFW2dTt+HH33L5FfegvfXfemJAO0L6krD0AwwQ6UywYDzAK MjEXjdc5d5l764QBWUhlWQvoXYSGzR8= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id 8AA1B43749; Thu, 5 Jun 2025 16:11:31 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3A7C2C4CEE7; Thu, 5 Jun 2025 16:11:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1749139891; bh=E7VA2CPsBD+xZsq1Va/C0TK1ZuXdJAIigt4d1i/YPfs=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=pY/uBsa3EXGps7nkVHf6ZlFan87fzB+jEHAbV7I1VbGwvXPet2AechYidp/w9NIBL 0DdwUlJvEupi8vlu2KTKZy6gOO1ls60Ab8KXOPXK0zR8SnQ+mZyPhr7jfT7mmTYtJ8 aHq1sqwk5WkEJW6DeMDH7KoniALPUVbO9UcC5z17Z1F/03TuLu8KpUEOMv65eI+L+s JBUkDF57OcmrwuXBNU2N3erkXquV3vR7Htz6EHEu+hnFQu2AvwyeOCWlzF8Fi8dpyx DMZeSSfJhPLmR103c+oy6Iimw61akqox7AymWQUhfdCE2asX5Z73xLoiOElb5fFXQ+ y/RCccouvgSnA== From: SeongJae Park To: Joshua Hahn Cc: SeongJae Park , Andrew Morton , damon@lists.linux.dev, kernel-team@meta.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v2 1/4] mm/damon: introduce DAMON_STAT module Date: Thu, 5 Jun 2025 09:11:29 -0700 Message-Id: <20250605161129.82107-1-sj@kernel.org> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250605152508.3275113-1-joshua.hahnjy@gmail.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: C2C1A40004 X-Stat-Signature: hz9qpjy6jzxifbmgfaoxaig4ci8xkoi9 X-Rspam-User: X-Rspamd-Server: rspam04 X-HE-Tag: 1749139892-8072 X-HE-Meta: U2FsdGVkX18MCfyk1z1XCBM1+YEjUpcSLPLPfnpVfJjvQmqSIKCAGM7YT0pRFriIeEbis/bxU5Otit1DVxPCR/cMr/DTjsPBglJnNL3gwcbkaxP03QPHVqXu633L2lteqwNIdzvTUnDRYXHige/GNa2/+KGXLzJqOCxqkhETmkQS8xCf1mUjrpi8Dws0Hsbnbtp8eM0oohg29rnqS3s+NzwvJaqbEqXFoRqj9Z8kORxAYRE188lIEBrBqS0oG4RM2/vxN4msdVuRqh/flgRJ4JtaO8TTdohG65yd88an0Ri9cx6+wZ8fRSmzpTsMXnSKxYqN64fxjazaYsrpkXQEVHbKFgWz+mLFMhE1uZZB+onwRRpsuwqnXpIIgzc9DB0jwsf2oOZmnuio4epcwQainqLjy5pkrGVdvc000kQ3iJ2W3r5SGMYYYH+MZp76D//2emojcusxVfkRROIbyp8RK0X9gmtG7rNN0rBw+Lno56f8nbDCbBWq/U030KO9uGaoHmp+YZ2mL+eauBhfOcxDfLJzTqptC7W+FTPuaRzdD/jti1sMbcgZa+I/Hnr86AkCqe9tXTo1UBnRw7HiDFckXOThK6gcJeDdeN67OlFN/jjY5QevXbNpKSwOeIA7XT4j7qS5syjUb8srhJmwrMqUrhbUT02RTBnfClF5NIYLeBHUI8iGZhJYMecZCeBFQuoZcNGngRThQI9rXelP1PTIdw+AOu/avvO2BcUa7k71nGLGGMNQhBJY9QLebRZNs9UJ5szNbsJLmxMRUJnvFS/Y8Z9U0auOcvDTH1NSVWTvTvjh3JZnasrTyaXMfQG5kuShjkaLxXKlS0LDQ+mA/FzdsfihavsdXFWzG9eAdn+7BFdwVBbDtaBHur21hTyB0l51t6genmfrRXPd0NPw6dQQmNRlgPDr2/gccmGK+M9lORFAeven7i9JoNVYXPpXqtVrP8i5z+WY1NCJc8CKdJI q96w0HJZ sqGrD0qM6AvxvqDIZ3lU2hucMHtCkSDjfal4dj1N4z015xQBSFOoBuvLAYNICGSrqtxwD0sPPumUHGe99mYh55seNFL7A9+ZUgnEZgo9mtO9ur+PEfnTsY847UbjP4LIqKF9DPJe3a+bEIB0Kk2Cx5wvTzj9qW6H4Fe9YshZ8byaXuuzdzsbE7PTOurM/TF7+AQUKWXeBgdRaG5dBFiTIT0DLddlgiKWIiIumz1jcOtQLyjMGbcX0i3US92tJXHmRfYxj4z+1Sf8pd/IRHhuxbRQKSpMwkBBXXEoprbLds/hmKvU3jkt7pUbTTfvBi5LngrZsV0BTE7QS2ECLO8RxDlp/OlzLOtjEDCxK3eZxI3UxHuWRVH1H7IA0Vg== 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, 5 Jun 2025 08:25:07 -0700 Joshua Hahn wrote: > On Wed, 4 Jun 2025 11:31:24 -0700 SeongJae Park wrote: [...] > Hi SJ, thank you for this patch! I have been looking forward to it : -) > I had a few questions about the init function: Hi Joshua, I'm more than happy to get your questions :) > > [...snip...] > > > +static int damon_stat_start(void) > > +{ > > + damon_stat_context = damon_stat_build_ctx(); > > + if (!damon_stat_context) > > + return -ENOMEM; > > + return damon_start(&damon_stat_context, 1, true); > > +} > > + > > +static void damon_stat_stop(void) > > +{ > > + damon_stop(&damon_stat_context, 1); > > + damon_destroy_ctx(damon_stat_context); > > +} > > + > > +static bool damon_stat_init_called; > > + > > +static int damon_stat_enabled_store( > > + const char *val, const struct kernel_param *kp) > > +{ > > + bool is_enabled = enabled; > > + int err; > > + > > + err = kstrtobool(val, &enabled); > > + if (err) > > + return err; > > + > > + if (is_enabled == enabled) > > + return 0; > > + > > + if (!damon_stat_init_called) > > + /* > > + * probably called from command line parsing (parse_args()). > > + * Cannot call damon_new_ctx(). Let damon_stat_init() handle. > > + */ > > + return 0; > > I was hoping you could educate me about how damon_stat_init_called works here. > I think my confusion comes from my lack of knowledge about kernel modules : -) > In the cover letter, you wrote that DAMON_STAT is a static kernel module. > My understanding was that this would mean damon_stat_init would always be > called, To my understanding, the function is called back only when the parameteer value is being changed. Such changes could be made in runtime via parameter files, and in boot time via the kernel command line. If there is not kernel command line for setting the parameter, this callback function is not called. > so I was wondering under what condition it would not be initialized. The enabled parameter value is initialized at build time, based on CONFIG_DAMON_STAT_ENABLED_DEFAULT. So, the parameter value will always be initialized. > I see the comment you wrote above, but was still a little bit confused. The kernel command line parameters parsing is called in pretty early stage of the bootup, before slab is ready. Hence, if enabled parmeter is set by the kernel command line, damon_stat_enabled_store() is called in the early stage, and fails from damon_stat_start(), since it needs slab, to initialize DAMON contexts. For more details of such failure, you could refer to a previous issue report[1]. Meanwhile, damon_stat_init() and module init functions are called later, when slab is ready. We therefore check the case, and defer real handling of enabled to damon_stat_init() in the case. Thank you for this question, I find the comment has rooms to improve. I'll try to make this better documented or easier to read. > > Also, should we perhaps call damon_stat_init() if !damon_stat_init_called? > That way, the first caller would just eat up the time it takes to run > damon_stat_start(). damon_stat_init() is a module init function, and hence it will be called in boot time, regardless of enabled parameter setup on kernel command line. In other words, it will be always invoked once, with !damon_stat_init_called. And it will call damon_stat_start(), unless enabled is unset via CONFIG_DAMON_STAT_ENABLED_DEFAULT or kernel command line. So, the current implementation is working as you suggested, to my understanding. Please let me know if I'm missing something. > > One other thought I have is that if this config checks for whether > damon_stat_init was called, this can be moved to the beginning of the function > before the other checks are run, but that is just my thought : -) Feel free > to keep the input check first, since having this at the beginning of the > function would mean incorrect inputs would be silently ignored. In the kernel command line based parameter setup scenario, later damon_stat_init() call should see the updated 'enabled' variable value. Hence, the user input value check should be done here, regardless of if this is called before or after damon_stat_init(). So I find no needs to change the code for now. Nonetheless, I believe this code has many rooms to improve, and I'm always getting more than glad to get this kind of improvement ideas. Thank you, Joshua. Please feel free to let me know if you get another idea later. I hope I answered your questions, but please let me know if I'm missing something! > > Thank you SJ! I hope you have a great day! You too. Friday is coming! [1] https://lore.kernel.org/linux-mm/20220604192222.1488-1-sj@kernel.org/ Thanks, SJ [...]