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

[zuoze <zuoze1@huawei.com>]

在 2025/4/11 6:25, SeongJae Park 写道:
> On Thu, 10 Apr 2025 14:28:23 +0800 zuoze <zuoze1@huawei.com> wrote:
> 
>>
>>
>> 在 2025/4/10 1:36, SeongJae Park 写道:
>>> On Wed, 9 Apr 2025 15:01:39 +0800 zuoze <zuoze1@huawei.com> wrote:
>>>
>>>>
>>>>
>>>> 在 2025/4/9 10:50, SeongJae Park 写道:
>>>>> Hi Ze,
>>>>>
>>>>> On Tue, 8 Apr 2025 15:55:53 +0800 Ze Zuo <zuoze1@huawei.com> 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
> 
> [...]
>