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 425C7C25B74 for ; Sun, 2 Jun 2024 16:51:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CE1716B0092; Sun, 2 Jun 2024 12:51:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C90AD6B00AD; Sun, 2 Jun 2024 12:51:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B583E6B00AE; Sun, 2 Jun 2024 12:51:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 96BD16B0092 for ; Sun, 2 Jun 2024 12:51:58 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 0F9E6140345 for ; Sun, 2 Jun 2024 16:51:58 +0000 (UTC) X-FDA: 82186540716.23.1D456DD Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf28.hostedemail.com (Postfix) with ESMTP id 99826C0013 for ; Sun, 2 Jun 2024 16:51:55 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=J1gDEQCl; spf=pass (imf28.hostedemail.com: domain of sj@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1717347116; 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=WDzAqGDnk9ew290kcVRN3Wo1Cq77jf2/MuU6xSgCXic=; b=cVJUuuRBh/x29nkENzhpw9lpmpdGq8KhfYSgR/fSJAx944TBwEha5LbREn6IAN9fbkrq8C E70l5rSv7pSFFUqvaRI44qhfGk53Qqy1pSFyMltZ1R1sbAZ7b8cNgG+H9bFIUR2YrSGdq0 7ciZNlXQ76fdRFUSYfdeYHKg3+HHch8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1717347116; a=rsa-sha256; cv=none; b=L8YPxPkgjp0lBNQ9Xfoj/oEUc/wm9vreQQpGv8n8yaWlSBSfFyBlN008qv+EjQOsE9KGuu LlhaG3WGbFP0HPD1OAW1CZVqCJxexZedouHQ9oHnribWH9+jU5jaaKgkDJzxVWsNcb3mqe gGx5o+r3dlkUvmvNpwmNsFsirS6WRpY= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=J1gDEQCl; spf=pass (imf28.hostedemail.com: domain of sj@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id E7DE6CE0691; Sun, 2 Jun 2024 16:51:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9393CC2BBFC; Sun, 2 Jun 2024 16:51:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1717347111; bh=KELyIxp7C1ZSE4EnYb9KTlzQqzR9hlMFEMS1zC3RQxU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=J1gDEQClGBBLQL3FBD1Ih4eMbaU0SRJ2Nxtj92djkUjE1Dm0yHTIFW7w8Lr4UnlHF o1oPw8oNwTHAeDr6FM6fuhDiX+BaTwYA40XnaJttl81TPoLD6hAVQS++Ya/I/MJd0I 3OzWKg9GJIxSjO/WF5vuX6imlEdZWF+a9OqQV88M0OlDW4RUblrjF136+3+RiEBeMM 5b+0MKxXpkLSur0C6sLiUeKdRXZ2vPU3oYyfSnN9/yaHyAY74Mml/TXsX39ZKUsugs 6TT4j4WgEowExdBf7UBjkW5bCFiA3h2+mRrJ59icvisKqEGyfn3k2pZaquZeme8vzw bH8VbCH7aLF7A== From: SeongJae Park To: Alex Rusuf Cc: SeongJae Park , damon@lists.linux.dev, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v2 1/2] mm/damon/core: add 'struct kdamond' abstraction layer Date: Sun, 2 Jun 2024 09:51:48 -0700 Message-Id: <20240602165148.92368-1-sj@kernel.org> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240602161557.927788-1-yorha.op@gmail.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Stat-Signature: 93jauzh4jx6okp76ujdoitjwyu83s3bj X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 99826C0013 X-HE-Tag: 1717347115-851831 X-HE-Meta: U2FsdGVkX18TIFQcGICXeiSc5V67S+Wo7Ed+/IwTz+18x9CYaMeYcCxPAUJceF+HprHdTzmNp/IKVIJ1whper1mukUJ8teAPm9Tqh6Ygq6zjz6rbY40oYFrEV8RJ9HHPrHFhj2bc5mFEYE5PVPSO/jyvRpA+OvQYE69s3HGWsahsOsEdF8XTDEyzM9HjNjcpwTkDJyuRtBr6vyxdJQKIMSG/STZeigkacM2VI80p1i2mUIWHmf2UIVQVQm8U4ILVhk4Ox1wjinib+bYCgg8NlOV9lwq/blU78T7bn6yxNRjjtl2aYJ57zsUVF+n+6gdD5ofApn5EkdF/r+Gy5J6o8PDts2nV3FHlTcYG7fvP88+F+HWSq1mLqxPKR+cZ17BhXkG55HR9VzbPiEUusPzfomDPd0afatSbFfqkW6pKlhdW2Gc9wNPtF1Xnb2B3BAFsj1RCO57v+5B2a6qcSKPQazg2tnb18ZxoOCtJqVtdYxRb1SnRnhbSOOuZQitWegvnGHc25GBGsgQaIrRyevqy6UTXF/RudpDqpq+ilbVctTwCri9ZGMtusFRVqSNdVjWKc8ZuGyD6U6OqduxzPvcIx4Uwy4tf5SW9qUVRDj3iQF7GGz+T0Hig/geo5cuFHcpSXKiFbSmCp7PlLaJgt+v5wnmv68ldhrUS8joK7LrDDphADDgY/iaQYwKlTTr/H2lT230YBYiLareu4VYx+eEz0xp75aVoWc7ZtXDNyf4jiP6N8ta5IIkJ3H0L33xajhzfAqZIwQm4QzHbHqwNVesMoBl31AsVeoiZFo/qF7x5Pia8ZVcwvRyB3hmQ+0OLVLUYhaHqfRBHT46tkzk5cnHE7BsgRwT9Z3rPKUYwE6/96xwECQqbcV9l/GGNtVyxB9Cuc2oy+dB6OQ5HV/jtnc4e+bxs6w+9PbR5csNkeDeF4JDRYl+x0j024+KsktDTWwwjdcUXIbSW/BEJ6pKonXq SqMnaZuy 22rRhcIxXXXQ8yFVowLA+qh0r5/E2LK1vWIR5EsohxHimuyDFa71PHJGJI6Us1HoR+086lT92PcExU08zke/b8y5n+HIv+/P8v6ICBKo38SCSzm73Zwqk46f2C4UWDLky+0kmO3pZDXKMqve3/5oS4Zb7+issFXTzV4UeclYM9wD3n07Yl2b1pdRu/CcESEEwQxF2TRzJZXqDxDmw+fNEBui3wFIseZpZiI7bIRvW28U4ks2Qz1vi8Z7uVtSY3tE1J+z5YIFPIw3YqtI= 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 Sun, 2 Jun 2024 19:15:57 +0300 Alex Rusuf wrote: > Hi SJ, > > > Hi Alex, > > > > On Fri, 31 May 2024 15:23:19 +0300 Alex Rusuf wrote: > > > > > In current implementation kdamond tracks only 1 > > > context, that is kdamond _is_ damon_ctx what makes > > > it very difficult to implement multiple contexts. > > > > > > This patch adds another level of abstraction, that is > > > 'struct kdamond' - structure which represents kdamond itself. > > > It holds references to all contexts organized in list. > > > > > > Few fields like ->kdamond_started and ->kdamond_lock > > > (just ->lock for 'struct kdamond') also has been moved > > > to 'struct kdamond', because they have nothing to do > > > with the context itself, but with the whole kdamond > > > daemon. > > > > > > Signed-off-by: Alex Rusuf > > > --- > > > include/linux/damon.h | 73 ++++++++--- > > > mm/damon/core.c | 249 ++++++++++++++++++++++------------- > > > mm/damon/lru_sort.c | 31 +++-- > > > mm/damon/modules-common.c | 36 +++-- > > > mm/damon/modules-common.h | 3 +- > > > mm/damon/reclaim.c | 30 +++-- > > > mm/damon/sysfs.c | 268 ++++++++++++++++++++++++-------------- > > > 7 files changed, 463 insertions(+), 227 deletions(-) > > > [...] > > > +/** > > > + * damon_kdamond_running() - Return true if kdamond is running > > > + * false otherwise. > > > + */ > > > +bool damon_kdamond_running(struct kdamond *kdamond) > > > +{ > > > + bool running; > > > + > > > + mutex_lock(&kdamond->lock); > > > + running = kdamond->self != NULL; > > > + mutex_unlock(&kdamond->lock); > > > + return running; > > > } > > > > Seems this function is used by only DAMON sysfs interface. Don't implement > > unnecessary public function. Implement it in mm/damon/sysfs.c please. > > DebugFS interface also uses this. I think dbgfs.c is using damon_nr_running_ctxs() now? De-duplicating code is nice, but I think that can be done in a separate patch. Moreover, DAMON debugfs interface is deprecated now. I'd like to keep changes to the file as minimum as possible. [...] > > > +static inline struct damon_ctx *damon_lru_sort_ctx(void) > > > +{ > > > + return damon_first_ctx(kdamond); > > > +} > > > + > > > +static inline struct damon_target *damon_lru_sort_target(void) > > > +{ > > > + return damon_first_target( > > > + damon_lru_sort_ctx()); > > > +} > > > > Do we really need above two functions? Can't we simply add 'kdamond' global > > variable but still uses 'ctx' and 'target' as before, except interface changed > > function calls? > > We can, I just tried to use 'readable' interface, I thought > that using raw pointers from kdamond will be less readable. I agree this will eventually be better code. But let's do such things in separte patches. Keeping individual patch simple and small would help poor reviewers :) [...] > > > @@ -1065,15 +1055,15 @@ static struct damon_sysfs_cmd_request damon_sysfs_cmd_request; > > > static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr, > > > char *buf) > > > { > > > - struct damon_sysfs_kdamond *kdamond = container_of(kobj, > > > + struct damon_sysfs_kdamond *sys_kdamond = container_of(kobj, > > > struct damon_sysfs_kdamond, kobj); > > > > This makes reviewing bit difficult. Let's keep current name for now. > > > > > - struct damon_ctx *ctx = kdamond->damon_ctx; > > > + struct kdamond *kdamond = sys_kdamond->kdamond; > > > > Keep 'kdamond' variable name as is, and use shorter name for this variable, > > say, kd, or do kdamond->kdamond. > > It usually makes reading code difficult, these are two different abstraction of > kdamond, but the name will almost be the same, I don't think this is a good idea. > > > > > > bool running; > > > > > > - if (!ctx) > > > + if (!kdamond) > > > > "if (!kd)" or "if (!kdamond->kdamond)" > > And this is an example, I can't say which one belong to sysfs's abstraction > and which represents core's kdamond. With "sys_" prefix or "sysfs_" sometimes, > it is much easier to read. I agree that. But from my perspective, these name changes are not essential for the purpose of this patch but only make review time consuming. Do one thing with minimum change, please. You could do the code cleanup in other patches. [...] > > > > >From this point, I think below changes could be significantly changed in next > > version of this patchset if you agree to above comments. So I'm stop reviewing > > this patch from here for now. Please let me know if you have different opinion > > about it. > > Thank you very much for your comments, I really appreciate that you spent time to > review this. I'll try to improve the patch-set in next versions! No problem, and looking forward to the next version! Thanks, SJ [...]