Message-ID: <CAMvvPS65Vi-1U2vgezx649J80nfYeZ5YeA=+1NBuhMkbyOvc8A@mail.gmail.com>
Date: Mon, 23 Jun 2025 09:57:33 -0500
From: Bijan Tabatabai <bijan311@...il.com>
To: Joshua Hahn <joshua.hahnjy@...il.com>
Cc: damon@...ts.linux.dev, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	sj@...nel.org, akpm@...ux-foundation.org, david@...hat.com, ziy@...dia.com, 
	matthew.brost@...el.com, rakie.kim@...com, byungchul@...com, 
	gourry@...rry.net, ying.huang@...ux.alibaba.com, apopple@...dia.com, 
	bijantabatab@...ron.com, venkataravis@...ron.com, emirakhur@...ron.com, 
	ajayjoshi@...ron.com, vtavarespetr@...ron.com
Subject: Re: [RFC PATCH v2 0/2] mm/damon/paddr: Allow interleaving in
 migrate_{hot,cold} actions

Hi Joshua,

On Mon, Jun 23, 2025 at 8:45 AM Joshua Hahn <joshua.hahnjy@...il.com> wrote:
>
> On Fri, 20 Jun 2025 13:04:56 -0500 Bijan Tabatabai <bijan311@...il.com> wrote:
>
> Hi Bijan,
>
> I hope you are doing well! Sorry for the late response.

No need to be sorry. I have no expectation patches sent on a Friday
afternoon will be looked at over the weekend.

[...]
> > Performance Test
> > ================
> > Below is a simple example showing that interleaving application data using
> > these patches can improve application performance.
> > To do this, we run a bandwidth intensive embedding reduction application
> > [5]. This workload is useful for this test because it reports the time it
> > takes each iteration to run and reuses its buffers between allocation,
> > allowing us to clearly see the benefits of the migration.
> >
> > We evaluate this a 128 core/256 thread AMD CPU, with 72 GB/s of local DDR
> > bandwidth and 26 GB/s of CXL memory.
> >
> > Before we start the workload, the system bandwidth utilization is low, so
> > we start with interleave weights biased as much as possible to the local
> > node. When the workload begins, it saturates the local bandwidth, making
> > the page placement suboptimal. To alleviate this, we modify the interleave
> > weights, triggering DAMON to migrate the workload's data.
> >
> >   $ cd /sys/kernel/mm/damon/admin/kdamonds/0/
> >   $ sudo cat ./contexts/0/schemes/0/action
> >   migrate_hot
> >   $ sudo cat ./contexts/0/schemes/0/target_nid
> >   0-1
> >   $ echo 255 | sudo tee /sys/kernel/mm/mempolicy/weighted_interleave/node0
> >   $ echo 1 | sudo tee /sys/kernel/mm/mempolicy/weighted_interleave/node1
> >   $ <path>/eval_baseline -d amazon_All -c 255 -r 100
> >   <clip startup output>
> >   Eval Phase 3: Running Baseline...
> >
> >   REPEAT # 0 Baseline Total time : 9043.24 ms
> >   REPEAT # 1 Baseline Total time : 7307.71 ms
> >   REPEAT # 2 Baseline Total time : 7301.4 ms
> >   REPEAT # 3 Baseline Total time : 7312.44 ms
> >   REPEAT # 4 Baseline Total time : 7282.43 ms
> >   # Interleave weights changed to 3:1
> >   REPEAT # 5 Baseline Total time : 6754.78 ms
> >   REPEAT # 6 Baseline Total time : 5322.38 ms
> >   REPEAT # 7 Baseline Total time : 5359.89 ms
> >   REPEAT # 8 Baseline Total time : 5346.14 ms
> >   REPEAT # 9 Baseline Total time : 5321.98 ms
> >
> > Updating the interleave weights, and having DAMON migrate the workload
> > data according to the weights resulted in an approximately 25% speedup.
>
> Thank you for sharing these very impressive results! So if I can understand
> correctly, this workload allocates once (mostly), and each iteration just
> re-uses the same allocation, meaning the effects of the weighted interleave
> change are isolated mostly to the migration portion.

That's correct.

> Based on that understanding, I'm wondering if a longer benchmark would help
> demonstrate the effects of this patch a bit better. That is, IIRC short-lived
> workloads should see most of its benefits come from correct allocation,
> while longer-lived workloads should see most of its benefits come from
> correct migration policies. I don't have a good idea of what the threshold
> is for characterizing short vs. long workloads, but I think this could be
> another prospective test you can use to demonstrate the gains of your patch.

You might be right. I'll try to think of something for the next
revision, but no promises.

[...]
> > Questions for Reviewers
> > =======================
> > 1. Are you happy with the changes to the DAMON sysfs interface?
> > 2. Setting an interleave weight to 0 is currently not allowed. This makes
> >    sense when the weights are only used for allocation. Does it make sense
> >    to allow 0 weights now?
>
> If the goal of 0 weights is to prevent migration to that node, I think that
> we should try to re-use existing mechanisms. There was actually quite a bit
> of discussion on whether 0 weights should be allowed (the entire converstaion
> was split across multiple versions, but I think this is the first instance [1]).

Thanks, I'll look over this.

> How about using nodemasks instead? I think that they serve a more explicit
> purpose of preventing certain nodes from being used. Please let me know if
> I'm missing something as to why we cannot use nodemasks here : -)

I think since we're moving towards DAMON having its own weights, this
would only apply to mempolicy. Changing an application's mempolic
nodemask would be nice, but based off Gregory's previous comments,
having something outside the application change that application's
nodemask might be a bit difficult [1]. Also, I think it would be
easier to change one weight rather than every affected application's
mempolicy.

> [...snip...]
>
> One last thing that I wanted to note -- given that these weights now serve
> a dual purpose of setting allocation & migration weights, does it make sense
> to update the weighted interleave documentation with this information as well?
> Or, since it really only affects DAMON users, should we be ok with leaving it
> out?
>
> My preference is that we include it in weighted interleave documentation
> (Documentation/ABI/testing/sysfs-kernel-mm-mempolikcy-weighted-interleave)
> so that anyone who edits weighted interleave code in the future will at least
> be aware that the changes they make will have effects in other subsystems.

I think if we continued to use the mempolicy weights, it would make
sense to document that. However, it seems like we are moving towards
using DAMON specific weights.

Thanks for your feedback,
Bijan

[1] https://lore.kernel.org/damon/aFBXuTtwhAV7BHeY@gourry-fedora-PF4VCD3F/

Powered by blists - more mailing lists