Re: [PATCH] resource: Remove dependency on SPARSEMEM from GET_FREE_REGION

[Huang, Ying]

Hi, Dan,

Dan Williams <dan.j.williams@xxxxxxxxx> writes:

> David Hildenbrand wrote:
>> On 15.10.24 10:03, Huang, Ying wrote:
>> > Hi, David,
>> > 
>> > David Hildenbrand <david@xxxxxxxxxx> writes:
>> > 
>> >> On 15.10.24 07:15, Huang Ying wrote:
>> >>> We want to use the functions configured via GET_FREE_REGION in
>> >>> resource kunit tests.  However, GET_FREE_REGION depends on SPARSEMEM.
>> >>> This makes resource kunit tests cannot be built on some architectures
>> >>> lacking SPARSEMEM.  In fact, these functions doesn't depend on
>> >>> SPARSEMEM now.  So, remove dependency on SPARSEMEM from
>> >>> GET_FREE_REGION.
>> >>> Link:
>> >>> https://lore.kernel.org/lkml/20240922225041.603186-1-linux@xxxxxxxxxxxx/
>> >>> Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
>> >>> Tested-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>> >>> Cc: Nathan Chancellor <nathan@xxxxxxxxxx>
>> >>> Cc: Arnd Bergmann <arnd@xxxxxxxx>
>> >>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> >>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>> >>> Cc: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
>> >>> ---
>> >>>    mm/Kconfig | 1 -
>> >>>    1 file changed, 1 deletion(-)
>> >>> diff --git a/mm/Kconfig b/mm/Kconfig
>> >>> index 4c9f5ea13271..33fa51d608dc 100644
>> >>> --- a/mm/Kconfig
>> >>> +++ b/mm/Kconfig
>> >>> @@ -1085,7 +1085,6 @@ config HMM_MIRROR
>> >>>    	depends on MMU
>> >>>      config GET_FREE_REGION
>> >>> -	depends on SPARSEMEM
>> >>>    	bool
>> >>>      config DEVICE_PRIVATE
>> >>
>> >> Added by
>> >>
>> >> commit 14b80582c43e4f550acfd93c2b2cadbe36ea0874
>> >> Author: Dan Williams <dan.j.williams@xxxxxxxxx>
>> >> Date:   Fri May 20 13:41:24 2022 -0700
>> >>
>> >>      resource: Introduce alloc_free_mem_region()
>> >>
>> >> @Dan, any insight why that dependency was added?
>> > 
>> > Dan has explain it some what in the following email,
>> > 
>> > https://lore.kernel.org/lkml/66f5abd431dce_964f2294b9@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch/
>> > 
>> > This is reachable from the "Link:" tag in the patch.
>> 
>> That should be part of the patch description then :)
>
> That Link: does not really describe the history though...

Sorry.  I made a mistake here.

> The description I would add is:
>
> ---
>
> When get_free_mem_region() was introduced the only consumers were those
> looking to pass the address range to memremap_pages(). That address
> range needed to be mindful of the maximum addressable platform physical
> address which at the time only SPARSMEM defined via MAX_PHYSMEM_BITS.
>
> Given that memremap_pages() also depended on SPARSEMEM via ZONE_DEVICE,
> it was easier to just depend on that definition than invent a general
> MAX_PHYSMEM_BITS concept outside of SPARSEMEM.
>
> Turns out that decision was buggy and did not account for KASAN
> consumption of physical address space. That problem was resolved
> recently with commit ea72ce5da228 ("x86/kaslr: Expose and use the end of
> the physical memory address space"), and GET_FREE_REGION dropped its
> MAX_PHYSMEM_BITS dependency.
>
> Then commit 99185c10d5d9 ("resource, kunit: add test case for
> region_intersects()"), went ahead and fixed up the only remaining
> dependency on SPARSEMEM which was usage of the PA_SECTION_SHIFT macro
> for setting the default alignment. A PAGE_SIZE fallback is fine in the
> SPARSEMEM=n case.
>
> With those build dependencies gone GET_FREE_REGION no longer depends on
> SPARSEMEM.

This looks great!  Will use this in the patch description in the next
version.

--
Best Regards,
Huang, Ying