Re: [RFC PATCH] mm/damon: add full LPAE support for memory monitoring above 4GB

[zuoze]

在 2025/4/11 6:25, SeongJae Park 写道:
> On Thu, 10 Apr 2025 14:28:23 +0800 zuoze <zuoze1@xxxxxxxxxx> wrote:
>
> > 在 2025/4/10 1:36, SeongJae Park 写道:
> > > On Wed, 9 Apr 2025 15:01:39 +0800 zuoze <zuoze1@xxxxxxxxxx> wrote:
> > >
> > > > 在 2025/4/9 10:50, SeongJae Park 写道:
> > > > > Hi Ze,
> > > > >
> > > > > On Tue, 8 Apr 2025 15:55:53 +0800 Ze Zuo <zuoze1@xxxxxxxxxx> wrote:
> > > > >
> > > > > > Previously, DAMON's physical address space monitoring only supported
> > > > > > memory ranges below 4GB on LPAE-enabled systems. This was due to
> > > > > > the use of 'unsigned long' in 'struct damon_addr_range', which is
> > > > > > 32-bit on ARM32 even with LPAE enabled.
> [...]
> > > I think another approach for this issue is adding a DAMON parameter, say,
> > > address_unit.  It will represent the factor value that need to be multiplied to
> > > DAMON-address to get real address on the given address space.  For example, if
> > > address_unit is 10 and the user sets DAMON monitoring target address range as
> > > 200-300, it means user wants DAMON to monitor address range 2000-3000 of the
> > > given address space.  The default value of the parameter would be 1, so that
> > > old users show no change.  32bit ARM with LPAE users would need to explicitly
> > > set the parameter but I believe that shouldn't be a big issue?
> >
> > Regarding the address_unit approach, I have some concerns after checking
> > the code:
> >
> > 1. Scaling Factor Limitations - While the scaling factor resolves the
> > damon_addr_range storage issue, actual physical addresses (PAs) would
> > still require unsigned long long temporaries in many cases.
>
> The current behavior, which is using 'unsigned long' as the type of the real
> address on DAMON operations set for physical address space (paddr), was just a
> wrong approach.  'paddr' operations set should use proper type for physical
> address, namely phys_addr_t.

Agreed. Using phys_addr_t for paddr is the right approach—unsigned long
was incorrect for physical addresses.

> > Different
> > system with varying iomem regions may require different scaling
> > factors, making deployment harder than a fixed maximum range.
>
> I was thinking the user space could set the proper scaling factor.  Would it be
> challenging?

Since memory ranges vary across systems, different scaling factors would
be needed. This could increase maintenance complexity.


> > 2. Significant Code Impact & Overhead - Implementing this would require
> > significant changes with every PA traversal needing rescaling, which
> > introduces computational overhead.
>
> Right, no small amount of code change will be required.  But those will be
> mostly isolated in operations set layer.
>
> For the computational overhead, I don't expect it woudl be significant, given
> region-based controlled and minimum overhead design.
>
> But, obviously doing some prototyping and testing first woudl give us a better
> picture >

Agreed. Some prototype testing would be helpful to evaluate overhead,
especially in extreme cases with large numbers of regions.

> > That said, there remains a necessity
> > to use unsigned long long to store certain variables in structures, such
> > as sampling_addr in the damon_region structure and sz_tried in the
> > damos_stat structure.
>
> I want the core layer to continue using its own concpetual address type
> (unsigned long).  sampling_addr and sz_tried are core layer's concepts, so
> should continu using 'unsigned long', while operations set should convert those
> appropriately.
>  >>
> > If I'm misunderstanding any points, please correct me, and feel free to
> > add any additional concerns.
> >
> > As an alternative, we could adopt a pattern similar to other subsystems
> > (e.g., memblock, CMA, resource), which define their own address types.
>
> The example cases directly deal with the specific address space, so their
> approaches make sense to me.  Also DAMON's operations set layer implmentation
> should also learn from them.

Glad you found them helpful!

> > For example:
> >
> >    #ifdef CONFIG_PHYS_ADDR_T_64BIT
> > 	typedef unsigned long long damon_addr_t;
> >    #else
> > 	typedef unsigned long damon_addr_t;
> >    #endif
>
> But in case of DAMON's core layer, it should deal with arbitrary address
> spaces, so I feel that might not necessarily be the only approach that we
> should use.

You're right, this method is not the only option.

> > This approach would avoid scaling complexity while maintaining
> > consistency with existing mm code.
> >
> > What do you think? SJ, I'm happy to help test any approach or discuss
> > further.
>
> So I still don't anticipate big problems with my proposed approach.  But only
> prototyping and testing would let us know more truth.  If you don't mind, I
> will quickly write and share a prototype of my idea so that you could test.

Sounds good! Please share the prototype when ready - happy to test and
help improve it.

> Thanks,
> SJ
>
> [...]