From: SeongJae Park <sj@kernel.org>
To: Yajun Deng <yajun.deng@linux.dev>
Cc: sj@kernel.org, akpm@linux-foundation.org, damon@lists.linux.dev,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mm/damon: introduce struct damos_access_pattern
Date: Thu, 8 Sep 2022 18:43:56 +0000 [thread overview]
Message-ID: <20220908184356.103133-1-sj@kernel.org> (raw)
In-Reply-To: <20220908032611.4072174-1-yajun.deng@linux.dev>
Hi Yajun,
On Thu, 8 Sep 2022 11:26:11 +0800 Yajun Deng <yajun.deng@linux.dev> wrote:
> The damon_new_scheme() has too many parameters, we can introduce struct
> damos_access_pattern to simplify it.
>
> In additon, we couldn't use bpf trace kprobe that more than 5 parameters.
>
> v2:
> - make the name of structure more sense.
Let's not put the change log here, but three lines after, after the marker line
('---')[1].
Also, providing the lore link to the previous version, e.g.,
https://lore.kernel.org/damon/20220907092626.3013765-1-yajun.deng@linux.dev/
for this case, in the change log would be a good practice.
[1] https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
>
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---
> include/linux/damon.h | 38 +++++++++++++++++++----------------
> mm/damon/core.c | 31 ++++++++++++++---------------
> mm/damon/dbgfs.c | 22 +++++++++++----------
> mm/damon/lru_sort.c | 46 ++++++++++++++++++++++++++-----------------
> mm/damon/reclaim.c | 23 +++++++++++++---------
> mm/damon/sysfs.c | 16 ++++++++++-----
> 6 files changed, 101 insertions(+), 75 deletions(-)
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 7b1f4a488230..7ddfb4551a13 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -216,13 +216,27 @@ struct damos_stat {
> };
>
> /**
> - * struct damos - Represents a Data Access Monitoring-based Operation Scheme.
> + * struct damos_access_pattern - Specify the access pattern of target regions.
> * @min_sz_region: Minimum size of target regions.
> * @max_sz_region: Maximum size of target regions.
> * @min_nr_accesses: Minimum ``->nr_accesses`` of target regions.
> * @max_nr_accesses: Maximum ``->nr_accesses`` of target regions.
> * @min_age_region: Minimum age of target regions.
> * @max_age_region: Maximum age of target regions.
> + */
> +
Unnecessary new line. Let's remove it.
> +struct damos_access_pattern {
> + unsigned long min_sz_region;
> + unsigned long max_sz_region;
> + unsigned int min_nr_accesses;
> + unsigned int max_nr_accesses;
> + unsigned int min_age_region;
> + unsigned int max_age_region;
> +};
> +
> +/**
> + * struct damos - Represents a Data Access Monitoring-based Operation Scheme.
> + * @pattern: Access pattern of target regions.
> * @action: &damo_action to be applied to the target regions.
> * @quota: Control the aggressiveness of this scheme.
> * @wmarks: Watermarks for automated (in)activation of this scheme.
> @@ -230,10 +244,8 @@ struct damos_stat {
> * @list: List head for siblings.
> *
> * For each aggregation interval, DAMON finds regions which fit in the
> - * condition (&min_sz_region, &max_sz_region, &min_nr_accesses,
> - * &max_nr_accesses, &min_age_region, &max_age_region) and applies &action to
> - * those. To avoid consuming too much CPU time or IO resources for the
> - * &action, "a is used.
> + * &pattern and applies &action to those. To avoid consuming too much
> + * CPU time or IO resources for the &action, "a is used.
> *
> * To do the work only when needed, schemes can be activated for specific
> * system situations using &wmarks. If all schemes that registered to the
> @@ -248,12 +260,7 @@ struct damos_stat {
> * &action is applied.
> */
> struct damos {
> - unsigned long min_sz_region;
> - unsigned long max_sz_region;
> - unsigned int min_nr_accesses;
> - unsigned int max_nr_accesses;
> - unsigned int min_age_region;
> - unsigned int max_age_region;
> + struct damos_access_pattern pattern;
> enum damos_action action;
> struct damos_quota quota;
> struct damos_watermarks wmarks;
> @@ -501,12 +508,9 @@ void damon_destroy_region(struct damon_region *r, struct damon_target *t);
> int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
> unsigned int nr_ranges);
>
> -struct damos *damon_new_scheme(
> - unsigned long min_sz_region, unsigned long max_sz_region,
> - unsigned int min_nr_accesses, unsigned int max_nr_accesses,
> - unsigned int min_age_region, unsigned int max_age_region,
> - enum damos_action action, struct damos_quota *quota,
> - struct damos_watermarks *wmarks);
> +struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
> + enum damos_action action, struct damos_quota *quota,
> + struct damos_watermarks *wmarks);
> void damon_add_scheme(struct damon_ctx *ctx, struct damos *s);
> void damon_destroy_scheme(struct damos *s);
>
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 9964b9d00768..57450a0160ab 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -230,24 +230,21 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
> return 0;
> }
>
> -struct damos *damon_new_scheme(
> - unsigned long min_sz_region, unsigned long max_sz_region,
> - unsigned int min_nr_accesses, unsigned int max_nr_accesses,
> - unsigned int min_age_region, unsigned int max_age_region,
> - enum damos_action action, struct damos_quota *quota,
> - struct damos_watermarks *wmarks)
> +struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
> + enum damos_action action, struct damos_quota *quota,
> + struct damos_watermarks *wmarks)
> {
> struct damos *scheme;
>
> scheme = kmalloc(sizeof(*scheme), GFP_KERNEL);
> if (!scheme)
> return NULL;
> - scheme->min_sz_region = min_sz_region;
> - scheme->max_sz_region = max_sz_region;
> - scheme->min_nr_accesses = min_nr_accesses;
> - scheme->max_nr_accesses = max_nr_accesses;
> - scheme->min_age_region = min_age_region;
> - scheme->max_age_region = max_age_region;
> + scheme->pattern.min_sz_region = pattern->min_sz_region;
> + scheme->pattern.max_sz_region = pattern->max_sz_region;
> + scheme->pattern.min_nr_accesses = pattern->min_nr_accesses;
> + scheme->pattern.max_nr_accesses = pattern->max_nr_accesses;
> + scheme->pattern.min_age_region = pattern->min_age_region;
> + scheme->pattern.max_age_region = pattern->max_age_region;
> scheme->action = action;
> scheme->stat = (struct damos_stat){};
> INIT_LIST_HEAD(&scheme->list);
> @@ -666,10 +663,12 @@ static bool __damos_valid_target(struct damon_region *r, struct damos *s)
> unsigned long sz;
>
> sz = r->ar.end - r->ar.start;
> - return s->min_sz_region <= sz && sz <= s->max_sz_region &&
> - s->min_nr_accesses <= r->nr_accesses &&
> - r->nr_accesses <= s->max_nr_accesses &&
> - s->min_age_region <= r->age && r->age <= s->max_age_region;
> + return s->pattern.min_sz_region <= sz &&
> + sz <= s->pattern.max_sz_region &&
> + s->pattern.min_nr_accesses <= r->nr_accesses &&
> + r->nr_accesses <= s->pattern.max_nr_accesses &&
> + s->pattern.min_age_region <= r->age &&
> + r->age <= s->pattern.max_age_region;
> }
>
> static bool damos_valid_target(struct damon_ctx *c, struct damon_target *t,
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index 652a94deafe3..5624e1805bb3 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -131,9 +131,9 @@ static ssize_t sprint_schemes(struct damon_ctx *c, char *buf, ssize_t len)
> damon_for_each_scheme(s, c) {
> rc = scnprintf(&buf[written], len - written,
> "%lu %lu %u %u %u %u %d %lu %lu %lu %u %u %u %d %lu %lu %lu %lu %lu %lu %lu %lu %lu\n",
> - s->min_sz_region, s->max_sz_region,
> - s->min_nr_accesses, s->max_nr_accesses,
> - s->min_age_region, s->max_age_region,
> + s->pattern.min_sz_region, s->pattern.max_sz_region,
> + s->pattern.min_nr_accesses, s->pattern.max_nr_accesses,
> + s->pattern.min_age_region, s->pattern.max_age_region,
Sorry, I prefer 80 columns rule:
https://docs.kernel.org/process/coding-style.html#breaking-long-lines-and-strings
> damos_action_to_dbgfs_scheme_action(s->action),
> s->quota.ms, s->quota.sz,
> s->quota.reset_interval,
> @@ -221,8 +221,6 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
> struct damos *scheme, **schemes;
> const int max_nr_schemes = 256;
> int pos = 0, parsed, ret;
> - unsigned long min_sz, max_sz;
> - unsigned int min_nr_a, max_nr_a, min_age, max_age;
> unsigned int action_input;
> enum damos_action action;
>
> @@ -233,13 +231,16 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
>
> *nr_schemes = 0;
> while (pos < len && *nr_schemes < max_nr_schemes) {
> + struct damos_access_pattern pattern = {};
> struct damos_quota quota = {};
> struct damos_watermarks wmarks;
>
> ret = sscanf(&str[pos],
> "%lu %lu %u %u %u %u %u %lu %lu %lu %u %u %u %u %lu %lu %lu %lu%n",
> - &min_sz, &max_sz, &min_nr_a, &max_nr_a,
> - &min_age, &max_age, &action_input, "a.ms,
> + &pattern.min_sz_region, &pattern.max_sz_region,
> + &pattern.min_nr_accesses, &pattern.max_nr_accesses,
> + &pattern.min_age_region, &pattern.max_age_region,
80 columns, again.
> + &action_input, "a.ms,
> "a.sz, "a.reset_interval,
> "a.weight_sz, "a.weight_nr_accesses,
> "a.weight_age, &wmarks.metric,
> @@ -251,7 +252,9 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
> if ((int)action < 0)
> goto fail;
>
> - if (min_sz > max_sz || min_nr_a > max_nr_a || min_age > max_age)
> + if (pattern.min_sz_region > pattern.max_sz_region ||
> + pattern.min_nr_accesses > pattern.max_nr_accesses ||
> + pattern.min_age_region > pattern.max_age_region)
> goto fail;
>
> if (wmarks.high < wmarks.mid || wmarks.high < wmarks.low ||
> @@ -259,8 +262,7 @@ static struct damos **str_to_schemes(const char *str, ssize_t len,
> goto fail;
>
> pos += parsed;
> - scheme = damon_new_scheme(min_sz, max_sz, min_nr_a, max_nr_a,
> - min_age, max_age, action, "a, &wmarks);
> + scheme = damon_new_scheme(&pattern, action, "a, &wmarks);
> if (!scheme)
> goto fail;
>
> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 9de6f00a71c5..59f6115e59de 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -293,6 +293,17 @@ static bool get_monitoring_region(unsigned long *start, unsigned long *end)
> /* Create a DAMON-based operation scheme for hot memory regions */
> static struct damos *damon_lru_sort_new_hot_scheme(unsigned int hot_thres)
> {
> + struct damos_access_pattern pattern = {
> + /* Find regions having PAGE_SIZE or larger size */
> + .min_sz_region = PAGE_SIZE,
> + .max_sz_region = ULONG_MAX,
> + /* and accessed for more than the threshold */
> + .min_nr_accesses = hot_thres,
> + .max_nr_accesses = UINT_MAX,
> + /* no matter its age*/
Let's add one space before closing the comment.
> + .min_age_region = 0,
> + .max_age_region = UINT_MAX,
> + };
> struct damos_watermarks wmarks = {
> .metric = DAMOS_WMARK_FREE_MEM_RATE,
> .interval = wmarks_interval,
> @@ -313,26 +324,31 @@ static struct damos *damon_lru_sort_new_hot_scheme(unsigned int hot_thres)
> .weight_nr_accesses = 1,
> .weight_age = 0,
> };
> - struct damos *scheme = damon_new_scheme(
> - /* Find regions having PAGE_SIZE or larger size */
> - PAGE_SIZE, ULONG_MAX,
> - /* and accessed for more than the threshold */
> - hot_thres, UINT_MAX,
> - /* no matter its age */
> - 0, UINT_MAX,
> +
> + return damon_new_scheme(
> + &pattern,
> /* prioritize those on LRU lists, as soon as found */
> DAMOS_LRU_PRIO,
> /* under the quota. */
> "a,
> /* (De)activate this according to the watermarks. */
> &wmarks);
> -
> - return scheme;
> }
>
> /* Create a DAMON-based operation scheme for cold memory regions */
> static struct damos *damon_lru_sort_new_cold_scheme(unsigned int cold_thres)
> {
> + struct damos_access_pattern pattern = {
> + /* Find regions having PAGE_SIZE or larger size */
> + .min_sz_region = PAGE_SIZE,
> + .max_sz_region = ULONG_MAX,
> + /* and not accessed at all */
> + .min_nr_accesses = 0,
> + .max_nr_accesses = 0,
> + /* for min_age or more micro-seconds*/
One space before closing the comment.
> + .min_age_region = cold_thres,
> + .max_age_region = UINT_MAX,
> + };
> struct damos_watermarks wmarks = {
> .metric = DAMOS_WMARK_FREE_MEM_RATE,
> .interval = wmarks_interval,
> @@ -354,21 +370,15 @@ static struct damos *damon_lru_sort_new_cold_scheme(unsigned int cold_thres)
> .weight_nr_accesses = 0,
> .weight_age = 1,
> };
> - struct damos *scheme = damon_new_scheme(
> - /* Find regions having PAGE_SIZE or larger size */
> - PAGE_SIZE, ULONG_MAX,
> - /* and not accessed at all */
> - 0, 0,
> - /* for cold_thres or more micro-seconds, and */
> - cold_thres, UINT_MAX,
> +
> + return damon_new_scheme(
> + &pattern,
> /* mark those as not accessed, as soon as found */
> DAMOS_LRU_DEPRIO,
> /* under the quota. */
> "a,
> /* (De)activate this according to the watermarks. */
> &wmarks);
> -
> - return scheme;
> }
>
> static int damon_lru_sort_apply_parameters(void)
> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index a7faf51b4bd4..3d78dcca1701 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c
> @@ -264,6 +264,17 @@ static bool get_monitoring_region(unsigned long *start, unsigned long *end)
>
> static struct damos *damon_reclaim_new_scheme(void)
> {
> + struct damos_access_pattern pattern = {
> + /* Find regions having PAGE_SIZE or larger size */
> + .min_sz_region = PAGE_SIZE,
> + .max_sz_region = ULONG_MAX,
> + /* and not accessed at all */
> + .min_nr_accesses = 0,
> + .max_nr_accesses = 0,
> + /* for min_age or more micro-seconds*/
One space before closing the comment.
> + .min_age_region = min_age / aggr_interval,
> + .max_age_region = UINT_MAX,
> + };
> struct damos_watermarks wmarks = {
> .metric = DAMOS_WMARK_FREE_MEM_RATE,
> .interval = wmarks_interval,
> @@ -284,21 +295,15 @@ static struct damos *damon_reclaim_new_scheme(void)
> .weight_nr_accesses = 0,
> .weight_age = 1
> };
> - struct damos *scheme = damon_new_scheme(
> - /* Find regions having PAGE_SIZE or larger size */
> - PAGE_SIZE, ULONG_MAX,
> - /* and not accessed at all */
> - 0, 0,
> - /* for min_age or more micro-seconds, and */
> - min_age / aggr_interval, UINT_MAX,
> +
> + return damon_new_scheme(
> + &pattern,
> /* page out those, as soon as found */
> DAMOS_PAGEOUT,
> /* under the quota. */
> "a,
> /* (De)activate this according to the watermarks. */
> &wmarks);
> -
> - return scheme;
> }
>
> static int damon_reclaim_apply_parameters(void)
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index fe6c6870cf86..012b5e9476bc 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -2259,11 +2259,20 @@ static int damon_sysfs_set_targets(struct damon_ctx *ctx,
> static struct damos *damon_sysfs_mk_scheme(
> struct damon_sysfs_scheme *sysfs_scheme)
> {
> - struct damon_sysfs_access_pattern *pattern =
> + struct damon_sysfs_access_pattern *access_pattern =
> sysfs_scheme->access_pattern;
> struct damon_sysfs_quotas *sysfs_quotas = sysfs_scheme->quotas;
> struct damon_sysfs_weights *sysfs_weights = sysfs_quotas->weights;
> struct damon_sysfs_watermarks *sysfs_wmarks = sysfs_scheme->watermarks;
> +
> + struct damos_access_pattern pattern = {
> + .min_sz_region = access_pattern->sz->min,
> + .max_sz_region = access_pattern->sz->max,
> + .min_nr_accesses = access_pattern->nr_accesses->min,
> + .max_nr_accesses = access_pattern->nr_accesses->max,
> + .min_age_region = access_pattern->age->min,
> + .max_age_region = access_pattern->age->max,
> + };
> struct damos_quota quota = {
> .ms = sysfs_quotas->ms,
> .sz = sysfs_quotas->sz,
> @@ -2280,10 +2289,7 @@ static struct damos *damon_sysfs_mk_scheme(
> .low = sysfs_wmarks->low,
> };
>
> - return damon_new_scheme(pattern->sz->min, pattern->sz->max,
> - pattern->nr_accesses->min, pattern->nr_accesses->max,
> - pattern->age->min, pattern->age->max,
> - sysfs_scheme->action, "a, &wmarks);
> + return damon_new_scheme(&pattern, sysfs_scheme->action, "a, &wmarks);
80 columns?
> }
>
> static int damon_sysfs_set_schemes(struct damon_ctx *ctx,
> --
> 2.25.1
Thanks,
SJ
next prev parent reply other threads:[~2022-09-08 18:43 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-08 3:26 [PATCH v2] mm/damon: introduce struct damos_access_pattern Yajun Deng
2022-09-08 18:43 ` SeongJae Park [this message]
2022-09-08 19:17 ` SeongJae Park
2022-09-09 1:53 ` Yajun Deng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220908184356.103133-1-sj@kernel.org \
--to=sj@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=damon@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=yajun.deng@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.