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 76B15C433EF for ; Tue, 23 Nov 2021 11:26:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D74046B0071; Tue, 23 Nov 2021 06:26:09 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D23416B0072; Tue, 23 Nov 2021 06:26:09 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C39CE6B0073; Tue, 23 Nov 2021 06:26:09 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0185.hostedemail.com [216.40.44.185]) by kanga.kvack.org (Postfix) with ESMTP id B41BB6B0071 for ; Tue, 23 Nov 2021 06:26:09 -0500 (EST) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 7638888CF4 for ; Tue, 23 Nov 2021 11:25:59 +0000 (UTC) X-FDA: 78839965596.07.A470F75 Received: from out30-130.freemail.mail.aliyun.com (out30-130.freemail.mail.aliyun.com [115.124.30.130]) by imf13.hostedemail.com (Postfix) with ESMTP id 619341044801 for ; Tue, 23 Nov 2021 11:25:54 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R411e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e01424;MF=xhao@linux.alibaba.com;NM=1;PH=DS;RN=4;SR=0;TI=SMTPD_---0UxwD6l6_1637666752; Received: from B-X3VXMD6M-2058.local(mailfrom:xhao@linux.alibaba.com fp:SMTPD_---0UxwD6l6_1637666752) by smtp.aliyun-inc.com(127.0.0.1); Tue, 23 Nov 2021 19:25:53 +0800 From: Xin Hao Reply-To: xhao@linux.alibaba.com Subject: Re: [PATCH V2 1/2] mm/damon/dbgfs: Modify Damon dbfs interface dependency in Kconfig To: SeongJae Park Cc: akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20211123103855.12592-1-sj@kernel.org> Message-ID: Date: Tue, 23 Nov 2021 19:25:52 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20211123103855.12592-1-sj@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed X-Stat-Signature: iwrozmzeiegdogcdm3q6i64wwpe3c6y3 Authentication-Results: imf13.hostedemail.com; dkim=none; spf=pass (imf13.hostedemail.com: domain of xhao@linux.alibaba.com designates 115.124.30.130 as permitted sender) smtp.mailfrom=xhao@linux.alibaba.com; dmarc=pass (policy=none) header.from=alibaba.com X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 619341044801 X-HE-Tag: 1637666754-918180 Content-Transfer-Encoding: quoted-printable 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: Hi Park: On 11/23/21 6:38 PM, SeongJae Park wrote: > On Sun, 21 Nov 2021 22:07:04 +0800 Xin Hao wro= te: > >> If you want to support "DAMON_DBGFS" in config file, it only depends o= n >> any one of "DAMON_VADDR" and "DAMON_PADDR", and sometimes we just want= to >> use damon virtual address function, but it is unreasonable to include = "DAMON_PADDR" >> in config file which cause the damon/paddr.c be compiled, so there fix= it. > Seems the above lines are not well wrapped[1]. > > [1] https://docs.kernel.org/process/submitting-patches.html?highlight=3D= 75#the-canonical-patch-format > >> Signed-off-by: Xin Hao >> --- >> include/linux/damon.h | 12 ++++++++++++ >> mm/damon/Kconfig | 2 +- >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/damon.h b/include/linux/damon.h >> index 8a73e825e0d5..00ad96f2ec10 100644 >> --- a/include/linux/damon.h >> +++ b/include/linux/damon.h >> @@ -463,11 +463,23 @@ int damon_stop(struct damon_ctx **ctxs, int nr_c= txs); >> #ifdef CONFIG_DAMON_VADDR >> void damon_va_set_primitives(struct damon_ctx *ctx); >> bool damon_va_target_valid(void *t); >> +#else >> +static inline void damon_va_set_primitives(struct damon_ctx *ctx) {} >> +static inline bool damon_va_target_valid(void *t) >> +{ >> + return false; >> +} >> #endif /* CONFIG_DAMON_VADDR */ >> =20 >> #ifdef CONFIG_DAMON_PADDR >> void damon_pa_set_primitives(struct damon_ctx *ctx); >> bool damon_pa_target_valid(void *t); >> +#else >> +static inline void damon_pa_set_primitives(struct damon_ctx *ctx) {} >> +static inline bool damon_pa_target_valid(void *t) >> +{ >> + return false; >> +} >> #endif /* CONFIG_DAMON_PADDR */ >> =20 >> #endif /* _DAMON_H */ >> diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig >> index 5bcf05851ad0..971ffc496596 100644 >> --- a/mm/damon/Kconfig >> +++ b/mm/damon/Kconfig >> @@ -54,7 +54,7 @@ config DAMON_VADDR_KUNIT_TEST >> =20 >> config DAMON_DBGFS >> bool "DAMON debugfs interface" >> - depends on DAMON_VADDR && DAMON_PADDR && DEBUG_FS >> + depends on DAMON_VADDR || DAMON_PADDR && DEBUG_FS > Due to the lack of parentheses, this allows enabling DAMON_DBGFS while = DEBUG_FS > is disabled. > > Sorry to say this but this makes me unsure if this patch is sufficientl= y > tested. I am also unsure if this patch is carefully handling all possi= ble > corner cases in appropriate ways. For example, on kernels that doesn't= having > 'CONFIG_DAMON_PADDR=3Dy', this patch would still make > 'echo paddr > $debugfs/damon/target_ids' success. Is that what you are > intending? Yes, you are all right, I have not tested as above, sorry, i did not=20 fully consider the impact on the code after doing this, Please ignore this patch. > And, have you confirmed that will never make any issue, both in > kernel space and the user space? Could the behavioral change make the = user > space confused? > > Basically, reducing the dependency is a good idea. But, IMHO, the bene= fit > seems not so big. After all, efficiency was not a first goal of DAMON = debugfs > interface. That's why it is implemented on debugfs rather than its own > specialized/optimized file system. Better way for the efficiency of th= e user > space interface might be inventing such DAMON's own efficient file syst= em, like > tracepoints had its interface on debugfs but now uses its own file syst= em, > tracefs. > > So, unless you make me believe the benefit is big enough and all possib= le > corner cases are well handled and sufficiently tested, I'm sorry but I = would > not be convinced with this change. I think it makes sense to separate PADDR & VADDR, users can freely=20 choose dbfs + vaddr or dbfs + paddr; Doing this can make the code look like it doesn=E2=80=99t mix together=EF= =BC=8Ci will=20 continue to try to separate this part of the code later. > > > Thanks, > SJ > >> help >> This builds the debugfs interface for DAMON. The user space admi= ns >> can use the interface for arbitrary data access monitoring. >> --=20 >> 2.31.0 --=20 Best Regards! Xin Hao