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 51642C5AE59 for ; Thu, 5 Jun 2025 19:25:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DD86B6B00C4; Thu, 5 Jun 2025 15:25:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D87D26B00C5; Thu, 5 Jun 2025 15:25:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C774B6B00C6; Thu, 5 Jun 2025 15:25:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id A522A6B00C4 for ; Thu, 5 Jun 2025 15:25:27 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 499F5141448 for ; Thu, 5 Jun 2025 19:25:27 +0000 (UTC) X-FDA: 83522325894.10.285E01C Received: from mail-yw1-f169.google.com (mail-yw1-f169.google.com [209.85.128.169]) by imf03.hostedemail.com (Postfix) with ESMTP id 8BA3C20010 for ; Thu, 5 Jun 2025 19:25:25 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="EN3IZ/9i"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.128.169 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1749151525; 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=FprG7nruJwqKDxMeetenSg6au9m16AMAZ5r0mOGn5VE=; b=C/o84svZ5g2aLmruqUPfVkDP8WneDFg2Kc92gF3fg8yKpncF6tDmTV+C2ZT6R96NxRRZLL YWE7+wkmqZ5n5zZIfXdlRLoMqkXzO8YhCD/Z9zix4jh1vIxuJNa5zz3TU7vEPNdTcZmjAX aRy9K37tetoFmsleeh2bGYDlwb9NmEE= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1749151525; a=rsa-sha256; cv=none; b=X7tAtIIPKEh1M+5EDARjSykFJKbyhjksqgx/Hjz+muqsTw6MiZj8pYNK0GDHIU5rxFFmn2 yniM3ZrSsqZtaGNalStoVGXG6l+HZOBpkmPv8/HpyWBVcMqsb2kBATf9OUdqpnVhOWHJr1 OSciwP8bgJp6qj1CIfKh001OE2E6RAo= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="EN3IZ/9i"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.128.169 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com Received: by mail-yw1-f169.google.com with SMTP id 00721157ae682-70e5d953c0bso16368237b3.1 for ; Thu, 05 Jun 2025 12:25:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1749151524; x=1749756324; darn=kvack.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=FprG7nruJwqKDxMeetenSg6au9m16AMAZ5r0mOGn5VE=; b=EN3IZ/9im6I3o4y058HyW4iMvvCYzB+GLBdNtj6bzQhOjAfjoPcpOUYGMdy0kKKPNr DMY/NWBWYzzMLnEnDa+8K+VhnQsN+gsc1RefNb+4yIMBxHeH71ynzbFjpdFExEbRRoDp toRaNF0cNX2pYefvm6XavgiWo1a3Oc5CB9+2WJGKsgAcyUzpnb7ClXd3wuUzLiE5NnH0 x5oHtN1DpZkCNZOvbPkuWdtyIG64JRZK8VLCknvuIlPNFBzof1SLb7zZ9HZ3oYSG6A1+ pFaQQboz24TroiQ/LmmMXEpEUDMN0xkVlZ0hdZAUoj/ARn4f4XN+pXGTBTfxK+FN8fky 2sZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1749151524; x=1749756324; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=FprG7nruJwqKDxMeetenSg6au9m16AMAZ5r0mOGn5VE=; b=YPUlSjaFR8GwODz5gnIQYP/179mkzg3j/tSePcXF1rlNVw9gsNOIKt8mlvpCR1wiA8 WKWJAG5pudYkAguDd7iU0Jleu5o8Hzko30SXdhVKUwguJVDwc57Kh2IP1eJ6vKGqZfxe 6MZ/BCWP9cT4jcomqeGG9Izlj8sT49p3WSy6jmsASwS8yGNimBVVrqniTGTd4+JW9sdp h9ld2LfaNPqocL1oxlJBMXO047A+GjTuAacUL64lT4zwXTrTxcA0OYTcnZbDVS5TofTt d9leiuhVCHSos5wXcm2gn0pFn62menOPDdehPxzNzmpoeL+MUV2akhnOA1TbLvdEiV5l Cluw== X-Forwarded-Encrypted: i=1; AJvYcCWObQZQIVxkjHL6Thz/Dymq+myB6ZUykKJxflQ2zttIz2CjMOx1qsJG5y40KqY94lr/4AbzdMPvsQ==@kvack.org X-Gm-Message-State: AOJu0Yzw5dVeFy6yCH7We5sfPvvVeOSZrrjIsDDw7BOBwHDmmpdA0KJZ UPz3XRrUCdNymg8M3+aDaxJ1dCwz9mv2g/c5jIIeyWwGhYDlpc6CyAiW X-Gm-Gg: ASbGncsvIe1gX7iXGh6GRFxFXD7Hk/8Q9fEgMZtat9ylvjjIqVS4yvq2fubm9t1fpzn 2XWez2tIlrEaUzQLh0u799adGNcFqP0SOKVpaD7XKcy8qkUDjTdeb27T1+INODAnAYNJDYeeYE5 4OuhZZGZpLZSEQp1L/QUjSCddXXL/+3nzKjqjA7t0pc90KVDvsDJkGyUaxB4XhsQsGTKw1bufVt L4Zaz5Ff00aUS7DFGvaYdBoK5IhwhUlNWuVQ1cQLeNhVI8Eo42pOg7lMeLGD/k2fACCtWmoR89j rlaO0M/jqfs79kEqfha+Wkq5hGqZ2x5iz86YsUKgJ+25gyEyWr0= X-Google-Smtp-Source: AGHT+IGk++Z15BxbtjJ9K7UZBhMrTiCpmJvFSUyXRDZkfKc9GS6MedIlLmJfAwhoGdBkkpcPFYk5Cw== X-Received: by 2002:a05:690c:688b:b0:70d:f673:1412 with SMTP id 00721157ae682-710f751e254mr12464717b3.0.1749151524484; Thu, 05 Jun 2025 12:25:24 -0700 (PDT) Received: from localhost ([2a03:2880:25ff:50::]) by smtp.gmail.com with ESMTPSA id 00721157ae682-70f8ad2439asm35895997b3.125.2025.06.05.12.25.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Jun 2025 12:25:23 -0700 (PDT) From: Joshua Hahn To: SeongJae Park Cc: 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 12:25:20 -0700 Message-ID: <20250605192521.306529-1-joshua.hahnjy@gmail.com> X-Mailer: git-send-email 2.47.1 In-Reply-To: <20250605161129.82107-1-sj@kernel.org> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 8BA3C20010 X-Stat-Signature: grxf5tb6pkyrh851s373bdzx6msnhhk8 X-Rspam-User: X-HE-Tag: 1749151525-652331 X-HE-Meta: U2FsdGVkX19zG8fLPB1pZSNm5mAti96JLFbi1+712ymLKsxXRvHIY1Z+uaWYD1kddx+4/WviWIlVqPD1j14kDLY6HKyEBAGHUHLvavTyTQw2Dw2ueYbtNbaMuYqmQxq6zYMqLcTPDblV4A2Mug08ikDg6s3g0TwWnl6FjtY5kr/yJl9g8X6tk4+8qoG0DntON+7dRoTcrlmO0jir9itI8pQsV+AhZsYTwAY42lcnb3sU7oAICWdfXkEzykHJRc47sN5q2LrngJBUIkR/ztNIWOH80orhYD3y0Ljn5fJZfa+b5YJsjRDeumoO95HLdVyPgOq5vfypZKsO+Gs30J9tVG4/vOQRv/cfcZ7X1sj6J6mQWgKDWgriGAmrvTgN5n1TRhlZSZ20MXCU2Lx8pUc8pd+XtsgsE8IiGiaTPkSBoCSXosPrBt217Gy12g60TyVz7O9HuWL0pFoeIzuoDUCyT6mEEFstW+dtn1jurC6By9BHT2tqVMed1VVRZr6qc++Tk9omJ9JtkWD2jrXpxqHHtO9cB6g3mmBF/MA8HYPi2UeCMEKMm62LRHNXFO/ylMM0dQYOxf7y29ebHJQCoMgGPWZGYaLWtIWpUE0G5s6LyKPWFzXvrvp/F6m3hixJzDYgwiirdLuuf6+LXzFikY713ITnhxfw8TE5ZOAMpBogt9tSLT8IwgmEls7Zj0tIwhIOznjiQhcg2mnSMOElZsWnERZvuhtFgySyTrvB3lsFOlCsI5pG1NalDnFe+NmYx07azeLcjPT7UkTqSsDTdng48LAsu/G4EpGCQDd83C084qY5PEsQETRyrhx1Y9YOIVi4/p4OytIQjuL1tBZIpDYgfcqUjwo12q18sYGYN8hr6IB8kT4fh3KBFHT0bgZdUVSG3brkM9WLt2vb1iDegXOwNQkLBj44AgGDexu0weXmhyaD4Q5NDG3hB34PazYNK9cVi5/l6gA5NJXWZ+NKEyK L2T8oH/T KsRupe1UhL5MbF2g/aH6PCNRXovAr9Q1nj/N85gXc0wdkx9wmuDNvKgpfSvUvF4l2Yd0Y98pDrzNSDrMhrwyvodO/nvt5ccDjHo3g8t7551pkniYB39GT2k8EAKVm73g0pK/gIn2iUpmYBIdbgvyW3rMI7Q== 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 09:11:29 -0700 SeongJae Park wrote: > 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. I see, I think it makes more sense to me now. Thank you for explaining this, SJ! I think my confusion came from my lack of knowledge. Please do not feel the need to update the comment on this section, if you feel it is already enough : -) > > 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! Thanks SJ, it all makes a lot more sense now. Thank you for taking the time to explain things! > > > > 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 > > [...] Sent using hkml (https://github.com/sjp38/hackermail)