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 2AB57EE49A4 for ; Mon, 11 Sep 2023 02:34:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 733016B0159; Sun, 10 Sep 2023 22:34:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6E33B6B015A; Sun, 10 Sep 2023 22:34:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5AB056B015B; Sun, 10 Sep 2023 22:34:28 -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 49F9B6B0159 for ; Sun, 10 Sep 2023 22:34:28 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 14F74C0536 for ; Mon, 11 Sep 2023 02:34:28 +0000 (UTC) X-FDA: 81222747816.11.B06CEE3 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by imf23.hostedemail.com (Postfix) with ESMTP id 1E1DC140008 for ; Mon, 11 Sep 2023 02:34:25 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=X2yNn9sq; spf=pass (imf23.hostedemail.com: domain of sj@kernel.org designates 145.40.68.75 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=1694399666; 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=5vshBcKS6PPq2s6IYjyjRsf+j7cPX5XEd3dtQDFzQv0=; b=uYDJYt++lO+biMIeeL9UARSQFW0raYzOO1s92/f3MHrGGMSnTzZeE4Nyz1D2xpRMv1roxJ 7p/id02iYr4dvf1gE3nqfi2GQ14yy0qLslpUtN4G2crWrThXFdznfu7cisUvPM7QCZu4EY hFbinXqR+X42UzsTq+fQ+Fqm2bASzHg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694399666; a=rsa-sha256; cv=none; b=bMZpM+lLD2pKhp861m+RGbKxEkhrR8vgOOcQBefovb4nyXA9YqjVW97y/tkq+btu4cyRSm PPfSHl8WWLrL2hKoRz+0bcy+O3VIklFb3YeO8XAG/2kgwXrD4rne7/9Eppq73aV3kjpXsG vUS4FIfZ/bVkpuqeq389qw4IY01iGVM= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=X2yNn9sq; spf=pass (imf23.hostedemail.com: domain of sj@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 71421B80D29; Mon, 11 Sep 2023 02:34:24 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B1BC2C433C8; Mon, 11 Sep 2023 02:34:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1694399663; bh=k/G7cODyLQl2Sxk/QoPyb4F689V1kXiXFE6EkBvZ2yY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=X2yNn9sqM4AXQModc75Aqk5wahhVKcu+LDjW4w8bYfKiOzAMG+6Jc8OrvJ+Y8QATy 3LVdBg3Ns//aNEEdY/I8JmOklu+YLS15g0zIE5D/gojIV/Lz5A9lL/1v1ToWRZ4xp2 8bhKn0jatOSavwyAC65UTEI9gVJRScYV5cQBsoFX6kkLIwQdMPyVa8kqTcsTJQqVAu YRWHqmYFjX+mTFc/xNFjbLIEb0TQmKLzwmPap0aNwdMxGrExx/ScXyWv0jjSFsD1kz jML8JHHWKpaE3+jPZunzjQkH8ND58vlhdV7fxGEIakDgx6TZiJj5heZv/pldEyut5T 9wJPCZpJMncqw== From: SeongJae Park To: SeongJae Park Cc: damon@lists.linux.dev, Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [RFC v2] mm/damon/core: use number of passed access sampling as a timer Date: Mon, 11 Sep 2023 02:34:21 +0000 Message-Id: <20230911023421.2040-1-sj@kernel.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230905035210.127868-1-sj@kernel.org> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 1E1DC140008 X-Rspam-User: X-Stat-Signature: nge1fkonaxtr7yjjn1pjdz3f7hg4yytt X-Rspamd-Server: rspam03 X-HE-Tag: 1694399665-903822 X-HE-Meta: U2FsdGVkX1/qgobu9OBA3LUq9Nhv4JW94jrHuMd/Q4SPe964G+VCo72NjrJJdY59FYaO7xamA5lqRljLb2Vc4umrWyftqXksR1LgIXcUCFaAK8/aCA9JUcW86ZX7rXszzwXLtEJLgX4MdWUCNm2Q47jwV6g+n7C2sGaEmv03YX/f5/OWSdlbT207v/7qm54hz7iO2xXkY0mNmgAFZJUmiFKdRJyJG54gvb4tTx4LXOR3TUAtitCurkIgwh4hrnZ8sySxk9PZnwwgCKdwKnmjEdGPIEoKfUrizGNyUJoMyOUFwHHw2SB1ewhtKMKm6+2DldjzM6adBs+gZs2hDr0ll5SqOHOpotzV7Hs03DsJDfWeKjvP+3l3NKJ3B3YPBKhfnCBUmOCsgg9rSLmsDKdlz+lXM0FtihJnhgqIGodV2k8ff0gSPs8AwAJqecJEbhI4GV6h/pBmtW6b7Gd0FZqDeu+zxZfIXX4vwcEKQiuZZhuiy3XRMy2lmlgck/P2LxgSkO7zaDMf+k+I87bvzZX3vWawMHGV3kVSqIUTsnzCdrfBswBHQbuP2UEQWTGQj3cJTlEnhGj8BWIfoLHXbYZUORlGFFWfHD+Xa8ltqQQe+6+yhoE/0en28AzQPVAfrWOq4qOxy/A68M6f0kosgUkTy/qSw9Wm4E5EmB2HRkYOFsuSeHkjsUSoqKw+FSLR5T/guuc0nPaertiZqEdRtfQ6DE8goU8TQ55KntQRjs9k4EIDVMlNqDKW3C9zyE7rrOrMmmXPh3tsmK/NU7qnRlDUybOVxH+AdQHKxOtnmHtmk508FFN4MRL4Fx7QPCYxl+ceMYv+Ky7E7FWHgeHm4yZEC8uw5rWuRM4G7TkXYpV9CHkaHAj2dGk0aHiZ3Vmqp8d07F/RPuQuDmjYS86glcSuK1UaBYUDaCMpRPUWkgSb/BODZr6C927yJlKKBGE/fJvRrgexYhz6XUb+WzUlgkb vhUWne8H NOiMmQXxoSmifwBszLedd2jEQL3kjWDBwQRtn0p6XgZYCtuDT+NE8PR9vP2sVZYEXWP6dasoy4W2MlGOJfVcjZlax78DXaGoL+DvYjEyOvPGviYfRK8rIpgABmYyX/EFz2gNn8/hcF5JrAfcmvi6BjvSx88oAKsB4VpJGaZHNuwR64csHyL38y/m5LzRFqfI0UGYT58jeZ5Y7zO/bgOlSzjOlBi/qqi2ApO6brGUACXEUwfhoxL7OpUdHflfgj/0x8HDsO0EDMvCUQTGAV/g2NUPPy9fQo27WZ8BIqj+PrnJ5wweMouo42NzUY+J+4+pQNYJS2bThsjGDI1YpeZAD+IZUCg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, 5 Sep 2023 03:52:10 +0000 SeongJae Park wrote: > Changes from v1 > (https://lore.kernel.org/damon/20230827003727.49369-1-sj@kernel.org/) > - Initalize next_*_sis at the beginning of kdamond_fn() > - Remove unnecessary remaining intervals compensations in > damon_set_attrs() > > DAMON sleeps for sampling interval after each sampling, and check if the > aggregation interval and the ops update interval have passed using > ktime_get_coarse_ts64() and baseline timestamps for the intervals. That > design is for making the operations occur at deterministic timing > regardless of the time that spend for each work. However, it turned out > it is not that useful, and incur not-that-intuitive results. > > After all, timer functions, and especially sleep functions that DAMON > uses to wait for specific timing, are not necessarily strictly accurate. > It is legal design, so no problem. However, depending on such > inaccuracies, the nr_accesses can be larger than aggregation interval > divided by sampling interval. For example, with the default setting (5 > ms sampling interval and 100 ms aggregation interval) we frequently show > regions having nr_accesses larger than 20. Also, if the execution of a > DAMOS scheme takes a long time, next aggregation could happen before > enough number of samples are collected. This is not what usual users > would intuitively expect. > > Since access check sampling is the smallest unit work of DAMON, using > the number of passed sampling intervals as the DAMON-internal timer can > easily avoid these problems. That is, convert aggregation and ops > update intervals to numbers of sampling intervals that need to be passed > before those operations be executed, count the number of passed sampling > intervals, and invoke the operations as soon as the specific amount of > sampling intervals passed. Make the change. > > Note that this could make a behavioral change to settings that using > intervals that not aligned by the sampling interval. For example, if > the sampling interval is 5 ms and the aggregation interval is 12 ms, > DAMON effectively used 15 ms as its aggregation interval with the old > design, hence it checks whether the aggregation interval after sleeping > the sampling interval. This change will make DAMON to effectively use > 10 ms as aggregation interval, since it uses 'aggregation interval / > sampling interval * sampling interval' as the effective aggregation > interval, and we don't use floating point types. Usual users would have > used aligned intervals, so this behavioral change is not expected to > make any meaningful impact, so just make this change. > > Signed-off-by: SeongJae Park > --- > include/linux/damon.h | 14 ++++++- > mm/damon/core.c | 87 +++++++++++++++++++------------------------ > 2 files changed, 51 insertions(+), 50 deletions(-) > [...] > diff --git a/mm/damon/core.c b/mm/damon/core.c > index b895f70acb2d..d64ddaa472ca 100644 > --- a/mm/damon/core.c > +++ b/mm/damon/core.c [...] > @@ -581,6 +583,8 @@ static void damon_update_monitoring_results(struct damon_ctx *ctx, > */ > int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs) > { > + unsigned long sample_interval; > + > if (attrs->min_nr_regions < 3) > return -EINVAL; > if (attrs->min_nr_regions > attrs->max_nr_regions) > @@ -588,6 +592,12 @@ int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs) > if (attrs->sample_interval > attrs->aggr_interval) > return -EINVAL; > > + sample_interval = attrs->sample_interval ? attrs->sample_interval : 1; > + ctx->next_aggregation_sis = ctx->passed_sample_intervals + > + attrs->aggr_interval / sample_interval; > + ctx->next_ops_update_sis = ctx->passed_sample_intervals + > + attrs->ops_update_interval / sample_interval; > + So, damon_set_attrs() can be called from after_wamrks_check() or after_aggregation() callbacks while kdamond is running. And, it updates the next_{aggregation,ops_update}_sis. > damon_update_monitoring_results(ctx, attrs); > ctx->attrs = *attrs; > return 0; > @@ -761,38 +771,6 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs) > return err; > } > [...] > @@ -1432,6 +1409,8 @@ static int kdamond_fn(void *data) > > pr_debug("kdamond (%d) starts\n", current->pid); > > + kdamond_init_intervals_sis(ctx); > + > if (ctx->ops.init) > ctx->ops.init(ctx); > if (ctx->callback.before_start && ctx->callback.before_start(ctx)) > @@ -1440,6 +1419,8 @@ static int kdamond_fn(void *data) > sz_limit = damon_region_sz_limit(ctx); > > while (!kdamond_need_stop(ctx)) { > + unsigned long sample_interval; > + > if (kdamond_wait_activation(ctx)) The after_wmarks_check() callback is called from kdamond_wait_activation(). Hence, the user might call damon_set_attrs() there, and next_{aggr_interval,ops_update}_sis can be changed here. > break; > > @@ -1450,11 +1431,17 @@ static int kdamond_fn(void *data) > break; > > kdamond_usleep(ctx->attrs.sample_interval); > + ctx->passed_sample_intervals++; > > if (ctx->ops.check_accesses) > max_nr_accesses = ctx->ops.check_accesses(ctx); > > - if (kdamond_aggregate_interval_passed(ctx)) { > + sample_interval = ctx->attrs.sample_interval ? > + ctx->attrs.sample_interval : 1; > + if (ctx->passed_sample_intervals == > + ctx->next_aggregation_sis) { This branch reads next_aggregation_sis, which could be changed from above after_wmarks_check() call. This branch also executes after_aggregation() callback, which again can call damon_set_attrs() and therefore update next_ops_update_sis. > + ctx->next_aggregation_sis += > + ctx->attrs.aggr_interval / sample_interval; > kdamond_merge_regions(ctx, > max_nr_accesses / 10, > sz_limit); > @@ -1469,7 +1456,11 @@ static int kdamond_fn(void *data) > ctx->ops.reset_aggregated(ctx); > } > > - if (kdamond_need_update_operations(ctx)) { > + if (ctx->passed_sample_intervals == > + ctx->next_ops_update_sis) { This branch reads next_ops_update_sis, which could be changed from above after_aggregation() or after_wmarks_check() callbacks. As a result, we can unexpectedly skip aggregation or ops update executions. This could be problem for later changes including pseudo-moving sum based nr_accesses[1] and DAMOS apply intervals[2]. Let's read next_{aggregation,ops_update}_sis at the beginning of the loop and use those. [1] https://lore.kernel.org/damon/20230909033711.55794-9-sj@kernel.org/ [2] https://lore.kernel.org/damon/20230910034048.59191-1-sj@kernel.org/ > + ctx->next_ops_update_sis += > + ctx->attrs.ops_update_interval / > + sample_interval; > if (ctx->ops.update) > ctx->ops.update(ctx); > sz_limit = damon_region_sz_limit(ctx); > -- > 2.25.1 > >