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 9BCEACD1292 for ; Mon, 8 Apr 2024 17:52:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 21A626B0082; Mon, 8 Apr 2024 13:52:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1CB3D6B0087; Mon, 8 Apr 2024 13:52:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0B98C6B008A; Mon, 8 Apr 2024 13:52:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id E23AB6B0082 for ; Mon, 8 Apr 2024 13:52:40 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id AB246A00A7 for ; Mon, 8 Apr 2024 17:52:40 +0000 (UTC) X-FDA: 81987109680.26.3FFCE94 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf08.hostedemail.com (Postfix) with ESMTP id 55CB716001A for ; Mon, 8 Apr 2024 17:52:36 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=dETVU4z5; spf=pass (imf08.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=1712598758; 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=VA3WcqXEmFJgqI9n09S3GwkiUE14Z8sZtwntCeAhbvE=; b=8l/r2lQl1Z0Vu4mmSAvsdRBlJF0CL8eDUvzvckCZq3L3L52UeX/DdeDABsmOHffS7Jl/Go AIjS8kpw+wuCDXv6Rf7XHvSPq+gUg7YeheKXauqNIuCbwdJ3dFBRjo3jNeMBvzeIZNKsUB LSClu5GeeLYFB3SruhGdUHZ3sodcxtg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712598758; a=rsa-sha256; cv=none; b=E/nfCwh6J0G66Q8cgkOKYOr9nxZVeTfpN0DPboUS8/2uuN0Y0G0LiGp2nRaqW6BimqtLr8 7Dl4GMBD0M9QlzpZMns/K4+1JZ2NvL3K8uKVRHpUFekMHAFGuvhtePKMfki82I2/twzscu hPw38LAVB3E2Fs2ZUaRqEFG0KPL6Fx0= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=dETVU4z5; spf=pass (imf08.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 65790CE122F; Mon, 8 Apr 2024 17:52:32 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0A4BDC433F1; Mon, 8 Apr 2024 17:52:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1712598751; bh=lqMYSwhK0qnhw09eJ1+Ailh7I3TSY0ZybkqE6/ufJ1c=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=dETVU4z5saCmnOtTCAvVfwcmZVyqiTbnvhnbyUHMbHRPFAUROww0oKuWoO6NtGdHS 5f2+06+UOqSYXziqPLu06ZUoMs6i8czPHx+n8SKi1degY9Q7ekGgozVIJs3ohkrJ6u eVb082wPFbA236byh5LRflA46GI8PnahThrnYv9U7l2cgsiVcnAy1LJgzLDiuXHsXB /y1q/lwED31nG8qIWpqvtS3jtO4jwWtmHMTk7RdZ3ieR/Pz/vgfiImOrVtvDHFhP6e j2ZLcku9F1MTc7OtG9myVWttU/0DWkGZAQQle8+TV5q+4FHVQnvvFG0XsVG4MRycMn 0K8vJS4x5hJxg== From: SeongJae Park To: Honggyu Kim Cc: SeongJae Park , damon@lists.linux.dev, linux-mm@kvack.org, akpm@linux-foundation.org, apopple@nvidia.com, baolin.wang@linux.alibaba.com, dave.jiang@intel.com, hyeongtak.ji@sk.com, kernel_team@skhynix.com, linmiaohe@huawei.com, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com, mhiramat@kernel.org, rakie.kim@sk.com, rostedt@goodmis.org, surenb@google.com, yangx.jy@fujitsu.com, ying.huang@intel.com, ziy@nvidia.com, 42.hyeyoo@gmail.com, art.jeongseob@gmail.com Subject: Re: [RFC PATCH v3 5/7] mm/damon/paddr: introduce DAMOS_MIGRATE_COLD action for demotion Date: Mon, 8 Apr 2024 10:52:28 -0700 Message-Id: <20240408175228.91414-1-sj@kernel.org> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240408120648.2947-1-honggyu.kim@sk.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 55CB716001A X-Rspam-User: X-Stat-Signature: rfou4wzam6n831aw4kgun56z6yan65pj X-Rspamd-Server: rspam03 X-HE-Tag: 1712598756-745038 X-HE-Meta: U2FsdGVkX1+SmzLp8EF8B4AaaFB1GOe/kjmwpKXqQX2tCCCjyfr2WFpojCf/pK7mgckaY/zvGOfksKLb/gZtnJR9FiizhkAjLzBv7GB7iwrYWl6oAJsNpLWQhtxo8gHlOGZLu9vpTEPMeEm/P/jIIC4RjXRdHn0UOUjJFfF/pdlNUtwf7XMkac/hOxXqtZ0XLV9ZgYldBjsI9eQoIyjpZDiPhAxoedEcsZdc2+bx+JuUkRKCWddvFlD6j65+ky18ngRyoN1Kzoy8xhPxzxbfqSN2OWUG0agm68zs8euPBIMX1mHgnbvfaG8KwjIdRH6yR2SRKuM8/EJris9Z+vZdNJ+eIT/J2zwrx7y03ZdknZKIL/JNdR/hUvuDw1HqHXAC5B3h3hq84v1zc4iTOvNY4qbPkJJRNNNhnqGGi2/uyN2ExJSIzVkeMemKeXJVaBwBhhlvH0CZEpHS6nPfxUxTW6gCXv9kPo2eT3pxtG+jnQQATsQiZdKDKxdR6uLvgftRr0tjz5HJcagoW4sQglsmF2GdfwtXaxkxBq24yFuJO6rVdwlRvaFVBa7gjGIA/92lfb48/gbnq93/8/EBoUQESLGzTGfcIxjRqTtmoo5beBulGx2ZljPFRjP/njwgFotQFWc5f4JLqhJh6PzxjyzssJ+7LKIOGpEZ5b/cvZSzgG4vOTlWTAy6wUPDJza4mYjzJODm8mRfdtki8K5i6T2GkrVIQl2i7ZNZgXRTz/25+QQlIQvQ27/N0MoYb0FZGd7AUNphFLPioIMTzorYL3DLumnamvPCDQPv8Ht0qIphhndtRv+IuZI7tCQponxE4ONO6CIh7m/zEsCymCbSZquqXOOXjdelTfWJa1FZ90+mYUNK4JImiq8gFmJbbs23O6vfjVlGnrILAsVCnj/fJP/NKzdQZyOOmGZTI5rohmkwwmIIB3qhCJ4VO9y69yHLcIIXP1Yc6MQvBzWWw098Leg ngU7xpki 57dyh6bH8E784iCaoj6alEOOYgQ2GrqePEdM2w3oM36Lqj7XhZycXTBkD4stjaxKND3wtYSx3Jgujkm2U1c5V1DZIc4fzTD3XCoats2kZ+0ag/lWWsncfUSMgrkgUMeCdjugojjbqq1D1bET9PlxwK7ML0e58Ehxd/Npqg2gW7TLoZ2Iu8Mt7HxWloLs6NjZzONOnASqI1Ma9miw0sszc4EKLQec9P5dRsRJZDkedsgt5nyB0ZuO8S+xMHrP6wdYzoqW2j03SYUvLJxoyrr9TLcZq7Z+oW+oqrv0ZzV/MSUFSmslZrt/Smrr2qIi+dbZ7fTdeWCOKaxKwApSMa6pWH0zFyT703yMjII6kPDT8qpa7WBGWmSG+LxBUXULeBqSwtnHiSb5QsdA/aGufq+MvX80/z4RNRZuetEfQ3C9KhFsRMvojRGklTpsPIVxjFoVqH7xQ 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 Mon, 8 Apr 2024 21:06:44 +0900 Honggyu Kim wrote: > On Fri, 5 Apr 2024 12:24:30 -0700 SeongJae Park wrote: > > On Fri, 5 Apr 2024 15:08:54 +0900 Honggyu Kim wrote: [...] > > > Here is one of the example usage of this 'migrate_cold' action. > > > > > > $ cd /sys/kernel/mm/damon/admin/kdamonds/ > > > $ cat contexts//schemes//action > > > migrate_cold > > > $ echo 2 > contexts//schemes//target_nid > > > $ echo commit > state > > > $ numactl -p 0 ./hot_cold 500M 600M & > > > $ numastat -c -p hot_cold > > > > > > Per-node process memory usage (in MBs) > > > PID Node 0 Node 1 Node 2 Total > > > -------------- ------ ------ ------ ----- > > > 701 (hot_cold) 501 0 601 1101 > > > > > > Since there are some common routines with pageout, many functions have > > > similar logics between pageout and migrate cold. > > > > > > damon_pa_migrate_folio_list() is a minimized version of > > > shrink_folio_list(), but it's minified only for demotion. > > > > MIGRATE_COLD is not only for demotion, right? I think the last two words are > > better to be removed for reducing unnecessary confuses. > > You mean the last two sentences? I will remove them if you feel it's > confusing. Yes. My real intended suggestion was 's/only for demotion/only for migration/', but entirely removing the sentences is also ok for me. > > > > > > > Signed-off-by: Honggyu Kim > > > Signed-off-by: Hyeongtak Ji > > > --- > > > include/linux/damon.h | 2 + > > > mm/damon/paddr.c | 146 ++++++++++++++++++++++++++++++++++++++- > > > mm/damon/sysfs-schemes.c | 4 ++ > > > 3 files changed, 151 insertions(+), 1 deletion(-) [...] > > > --- a/mm/damon/paddr.c > > > +++ b/mm/damon/paddr.c [...] > > > +{ > > > + unsigned int nr_succeeded; > > > + nodemask_t allowed_mask = NODE_MASK_NONE; > > > + > > > > I personally prefer not having empty lines in the middle of variable > > declarations/definitions. Could we remove this empty line? > > I can remove it, but I would like to have more discussion about this > issue. The current implementation allows only a single migration > target with "target_nid", but users might want to provide fall back > migration target nids. > > For example, if more than two CXL nodes exist in the system, users might > want to migrate cold pages to any CXL nodes. In such cases, we might > have to make "target_nid" accept comma separated node IDs. nodemask can > be better but we should provide a way to change the scanning order. > > I would like to hear how you think about this. Good point. I think we could later extend the sysfs file to receive the comma-separated numbers, or even mask. For simplicity, adding sysfs files dedicated for the different format of inputs could also be an option (e.g., target_nids_list, target_nids_mask). But starting from this single node as is now looks ok to me. [...] > > > + /* 'folio_list' is always empty here */ > > > + > > > + /* Migrate folios selected for migration */ > > > + nr_migrated += migrate_folio_list(&migrate_folios, pgdat, target_nid); > > > + /* Folios that could not be migrated are still in @migrate_folios */ > > > + if (!list_empty(&migrate_folios)) { > > > + /* Folios which weren't migrated go back on @folio_list */ > > > + list_splice_init(&migrate_folios, folio_list); > > > + } > > > > Let's not use braces for single statement > > (https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces). > > Hmm.. I know the convention but left it as is because of the comment. > If I remove the braces, it would have a weird alignment for the two > lines for comment and statement lines. I don't really hate such alignment. But if you don't like it, how about moving the comment out of the if statement? Having one comment for one-line if statement looks not bad to me. > > > > + > > > + try_to_unmap_flush(); > > > + > > > + list_splice(&ret_folios, folio_list); > > > > Can't we move remaining folios in migrate_folios to ret_folios at once? > > I will see if it's possible. Thank you. Not a strict request, though. [...] > > > + nid = folio_nid(lru_to_folio(folio_list)); > > > + do { > > > + struct folio *folio = lru_to_folio(folio_list); > > > + > > > + if (nid == folio_nid(folio)) { > > > + folio_clear_active(folio); > > > > I think this was necessary for demotion, but now this should be removed since > > this function is no more for demotion but for migrating random pages, right? > > Yeah, it can be removed because we do migration instead of demotion, > but I need to make sure if it doesn't change the performance evaluation > results. Yes, please ensure the test results are valid :) Thanks, SJ [...]