linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: SeongJae Park <sj@kernel.org>
To: Yueyang Pan <pyyjason@gmail.com>
Cc: SeongJae Park <sj@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Usama Arif <usamaarif642@gmail.com>,
	damon@lists.linux.dev, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 0/2] mm/damon: Add damos_stat support for vaddr
Date: Wed, 30 Jul 2025 13:02:39 -0700	[thread overview]
Message-ID: <20250730200239.60984-1-sj@kernel.org> (raw)
In-Reply-To: <cover.1753895066.git.pyyjason@gmail.com>

On Wed, 30 Jul 2025 10:19:54 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:

> Previously damos_stat only supoort paddr. This patch set adds support 
> for damos_stat for vaddr. Also all different types of filters are 
> supported. 
> 
> Functionality Test
> ==================
> I wrote a small test program which allocates 10GB of DRAM, use 
> madvise(MADV_HUGEPAGE) to convert the base pages to 2MB huge pages
> Then my program does the following things in order:
> 1. Write sequentially to the whole 10GB region
> 2. Read the first 5GB region sequentially for 10 times
> 3. Sleep 5s
> 4. Read the second 5GB region sequentially for 10 times
> 
> With a proper damon setting, we are expected to see df-passed to be 10GB
> and hot region move around with the read
> 
> $ # Start damon record
> $sudo ./damo/damo start "./my_test/test" --monitoring_intervals 100ms\
> 1s 2s

Above command is not really doing record, so the first line would better to be
updated, say, "Start DAMON" ?

Also, it is easier to read if you give prefix spaces for quoting commands as
well, to make it consistent with their outputs.  Fo for example, above could be
something like below.

    $ # Start DAMON
    $ sudo ./damo/damo start ...

> 
> $ # damon report
> $sudo ./damo/damo report access --snapshot_damos_filter allow \
> hugepage_size 2MiB 2MiB

Again, I'd prefer having prefix spaces for above commands consistent to the
following output.

Also, maybe the first comment could be more detailed, say, "Show
DAMON-generated access pattern snapshot" ?

>     heatmap:
>     # min/max temperatures: -600,000,000, 100,001,000, column size: 137.352 MiB
>     intervals: sample 100 ms aggr 1 s (max access hz 10)
>     # damos filters (df): reject none hugepage_size [2.000 MiB, 2.000 MiB]
>     df-pass:
>     # min/max temperatures: -400,000,000, 100,001,000, column size: 128.031 MiB
>     0   addr 85.373 TiB   size 745.555 MiB access 0 hz   age 6 s           df-passed 0 B
>     1   addr 127.608 TiB  size 877.664 MiB access 3.000 hz age 0 ns          df-passed 878.000 MiB
>     2   addr 127.609 TiB  size 219.418 MiB access 2.000 hz age 0 ns          df-passed 220.000 MiB
>     3   addr 127.609 TiB  size 316.613 MiB access 1.000 hz age 1 s           df-passed 316.000 MiB
[...]
>     memory bw estimate: 6.524 GiB per second  df-passed: 6.527 GiB per second
>     total size: 10.731 GiB  df-passed 10.000 GiB
>     record DAMON intervals: sample 100 ms, aggr 1 s
> 
> 
> $ # damon report again
> $sudo ./damo/damo report access --snapshot_damos_filter allow \
> hugepage_size 2MiB 2MiB

Ditto.

>     heatmap:
>     # min/max temperatures: -1,100,000,000, 2,000, column size: 137.352 MiB
>     intervals: sample 100 ms aggr 1 s (max access hz 10)
>     # damos filters (df): reject none hugepage_size [2.000 MiB, 2.000 MiB]
>     df-pass:
>     # min/max temperatures: -900,000,000, 2,000, column size: 128.031 MiB
>     0   addr 85.373 TiB   size 745.555 MiB access 0 hz   age 11 s          df-passed 0 B
>     1   addr 127.608 TiB  size 579.715 MiB access 2.000 hz age 0 ns          df-passed 580.000 MiB
>     2   addr 127.608 TiB  size 144.930 MiB access 2.000 hz age 0 ns          df-passed 146.000 MiB
>     3   addr 127.608 TiB  size 452.453 MiB access 2.000 hz age 0 ns          df-passed 452.000 MiB
[...]
>     memory bw estimate: 5.297 GiB per second  df-passed: 5.297 GiB per second
>     total size: 10.731 GiB  df-passed 10.000 GiB
>     record DAMON intervals: sample 100 ms, aggr 1 s
> 
> As you can see the total df-passed region is 10GiB and the hot region
> moves as the seq read keeps going
> 
> Revision History
> ================
> Changes from v1 [1]:
> - Follow David's advise to combine *pmd_entry() and *pte_entry() into
>   one. Also remove manually setting walk->action
> - Use vma_normal_page and vma_normal_page_pmd instead of damon_get_folio
>   to remove redundant folio_get and folio_put
> - Follow SJ's advise to only move damon_pa_scheme_has_filter to
>   ops-common
> - Change the command used in cover-letter for more natural illustration
> 
> [1] https://lore.kernel.org/all/cover.1753794408.git.pyyjason@gmail.com/
> 
> Yueyang Pan (2):
>   mm/damon: Move has filter to ops-common
>   mm/damon: Add damos_stat support for vaddr

I also left a few trivial comments to the patches.  So I find no real concerns
but asking for a few trivial cleanups.  Could you please make the changes with
next version, after waiting a day for giving others to chime in for things I
may missed?


Thanks,
SJ

[...]


      parent reply	other threads:[~2025-07-30 20:02 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-30 17:19 [PATCH v2 0/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
2025-07-30 17:19 ` [PATCH v2 1/2] mm/damon: Move has filter to ops-common Yueyang Pan
2025-07-30 17:45   ` SeongJae Park
2025-07-31 22:04     ` YUEYANG PAN
2025-07-30 17:19 ` [PATCH v2 2/2] mm/damon: Add damos_stat support for vaddr Yueyang Pan
2025-07-30 17:56   ` SeongJae Park
2025-07-31 22:03     ` YUEYANG PAN
2025-07-30 20:02 ` SeongJae Park [this message]

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=20250730200239.60984-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=pyyjason@gmail.com \
    --cc=usamaarif642@gmail.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).