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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DC341C433F5 for ; Thu, 21 Oct 2021 16:43:01 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A0E536138D for ; Thu, 21 Oct 2021 16:43:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A0E536138D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id D56FA6B006C; Thu, 21 Oct 2021 12:42:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D0606900002; Thu, 21 Oct 2021 12:42:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BA7C26B0073; Thu, 21 Oct 2021 12:42:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0024.hostedemail.com [216.40.44.24]) by kanga.kvack.org (Postfix) with ESMTP id A2F066B006C for ; Thu, 21 Oct 2021 12:42:59 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 4B56A2D222 for ; Thu, 21 Oct 2021 16:42:59 +0000 (UTC) X-FDA: 78721014078.30.771B43A Received: from out30-42.freemail.mail.aliyun.com (out30-42.freemail.mail.aliyun.com [115.124.30.42]) by imf13.hostedemail.com (Postfix) with ESMTP id C7244104373D for ; Thu, 21 Oct 2021 16:42:52 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R961e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04394;MF=xhao@linux.alibaba.com;NM=1;PH=DS;RN=5;SR=0;TI=SMTPD_---0UtABIP-_1634834571; Received: from B-X3VXMD6M-2058.local(mailfrom:xhao@linux.alibaba.com fp:SMTPD_---0UtABIP-_1634834571) by smtp.aliyun-inc.com(127.0.0.1); Fri, 22 Oct 2021 00:42:52 +0800 From: Xin Hao Reply-To: xhao@linux.alibaba.com Subject: Re: [PATCH] mm/damon/dbgfs: Optimize target_ids interface write operation To: SeongJae Park Cc: sjpark@amazon.de, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20211021094543.1846-1-sj@kernel.org> Message-ID: <3fecc78c-44ed-368a-d64f-6e809020bd77@linux.alibaba.com> Date: Fri, 22 Oct 2021 00:42:50 +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: <20211021094543.1846-1-sj@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed X-Stat-Signature: x9fbwyh9w8idf3g73uy33brz7pxztcp5 Authentication-Results: imf13.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=alibaba.com; spf=pass (imf13.hostedemail.com: domain of xhao@linux.alibaba.com designates 115.124.30.42 as permitted sender) smtp.mailfrom=xhao@linux.alibaba.com X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: C7244104373D X-HE-Tag: 1634834572-164435 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: On 2021/10/21 =E4=B8=8B=E5=8D=885:45, SeongJae Park wrote: > Hello Xin, > > On Thu, 21 Oct 2021 16:56:11 +0800 Xin Hao wro= te: > >> When writing some pids to target_ids interface, calling scanf() >> to get 'id' may be failed. If the value of '*nr_ids' is 0 at this time= , >> there is no need to return 'ids' here, we just need to release it and >> return NULL pointer to improve related code operation efficiency. > Thank you for the patch! But, I don't think this patch makes sense, be= cause > the case (*nr_ids =3D=3D 0) means not error but an ask to remove previo= usly set > target ids. For example, it works as below now: > > # echo 42 > target_ids > # cat target_ids > 42 > # echo > target_ids > # cat target_ids > > # > > But, with your patch, the behavior will be changed as below: > > # echo 42 > target_ids > # cat target_ids > 42 > # echo > target_ids > bash: echo: write error: Cannot allocate memory > # cat target_ids > 42 > # Hi Park: =C2=A0=C2=A0=C2=A0 I did not use the lastest code to test, so I ignored = that it calls=20 'damon_set_targets' to clear previously set target ids, Sorry. =C2=A0=C2=A0=C2=A0 I think if we want only want to clear the previously = set target=20 ids, this codes are all no need to execute, except call 'damon_set_targets(ctx, NULL, 0);' ---------------------------------------------------------- =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (id_is_pid) { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 for (i =3D 0; i < nr_targets; i++) { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 target= s[i] =3D (unsigned long)find_get_pid( =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 (int)targets[i]); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!t= argets[i]) { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dbgfs_put_pids(targets, i); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -EINVAL; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto free_targets_out; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 } =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mutex_lock(&ctx->kdamond_lock= ); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ctx->kdamond) { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 if (id_is_pid) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dbgfs_= put_pids(targets, nr_targets); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 ret =3D -EBUSY; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 goto unlock_out; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 . . . =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Configure the context for = the address space type */ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (id_is_pid) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 damon_va_set_primitives(ctx); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 damon_pa_set_primitives(ctx); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D damon_set_targets(ctx= , targets, nr_targets); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret) { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 if (id_is_pid) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dbgfs_= put_pids(targets, nr_targets); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 ret =3D count; > > Also, this patch makes the DAMON selftest fails as below: > > $ sudo make -C tools/testing/selftests/damon run_tests > make: Entering directory '/home/sjpark/linux/tools/testing/selftes= ts/damon' > TAP version 13 > 1..2 > # selftests: damon: debugfs_attrs.sh > # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument > # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument > # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument > # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument > # ./debugfs_attrs.sh: line 11: echo: write error: Invalid argument > # ./debugfs_attrs.sh: line 11: echo: write error: Cannot allocate = memory > # writing abc 2 3 to /sys/kernel/debug/damon/target_ids doesn't re= turn 0 > # expected because: the file allows wrong input > not ok 1 selftests: damon: debugfs_attrs.sh # exit=3D1 > > If I'm missing something, please let me know. I will post a new patch to fix the problems you encountered in the above=20 test, thank you so much! > > > Thanks, > SJ > >> Signed-off-by: Xin Hao >> --- >> mm/damon/dbgfs.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c >> index a02cf6bee8e8..2d77bf579ffb 100644 >> --- a/mm/damon/dbgfs.c >> +++ b/mm/damon/dbgfs.c >> @@ -308,21 +308,25 @@ static unsigned long *str_to_target_ids(const ch= ar *str, ssize_t len, >> unsigned long *ids; >> const int max_nr_ids =3D 32; >> unsigned long id; >> - int pos =3D 0, parsed, ret; >> + int pos =3D 0, parsed; >> =20 >> *nr_ids =3D 0; >> ids =3D kmalloc_array(max_nr_ids, sizeof(id), GFP_KERNEL); >> if (!ids) >> return NULL; >> while (*nr_ids < max_nr_ids && pos < len) { >> - ret =3D sscanf(&str[pos], "%lu%n", &id, &parsed); >> - pos +=3D parsed; >> - if (ret !=3D 1) >> + if (sscanf(&str[pos], "%lu%n", &id, &parsed) !=3D 1) >> break; >> + pos +=3D parsed; >> ids[*nr_ids] =3D id; >> *nr_ids +=3D 1; >> } >> =20 >> + if (!*nr_ids) { >> + kfree(ids); >> + return NULL; >> + } >> + >> return ids; >> } >> =20 >> --=20 >> 2.31.0 --=20 Best Regards! Xin Hao