[PATCH v9 1/3] riscv: mm: modify pte format for Svnapot

[Qinglin Pan]

Hi all,

On 2022/12/9 22:33, Conor Dooley wrote:
> 
> 
> On 9 December 2022 08:42:29 GMT+01:00, Andrew Jones <ajones at ventanamicro.com> wrote:
>> On Fri, Dec 09, 2022 at 01:34:42AM +0800, Qinglin Pan wrote:
>> ...
>>>>>> +#ifdef CONFIG_RISCV_ISA_SVNAPOT
>>>>>> +#define HUGE_MAX_HSTATE        (2 + (NAPOT_ORDER_MAX -
>>>>>> NAPOT_CONT_ORDER_BASE))
>>>>>> +#else
>>>>>> +#define HUGE_MAX_HSTATE        2
>>>>>> +#endif
>>>>>
>>>>> This is coming from a place of *complete* ignorance:
>>>>> If CONFIG_RISCV_ISA_SVNAPOT is enabled in the kernel but there is no
>>>>> support for it on a platform is it okay to change the value of
>>>>> HUGE_MAX_HSTATE? Apologies if I've missed something obvious.
>>>>
>>>> :(
>>>> You are right. If CONFIG_RISCV_ISA_SVNAPOT is enabled, HUGE_MAX_HSTATE
>>>> will always be 3 whether has_svnapot() is true or false. I will try to
>>>> fix this.
>>>
>>> I am really expecting that HUGE_MAX_HSTATE can change according to the
>>> platform situation when CONFIG_RISCV_ISA_SVNAPOT is enabled. But it
>>> seems impossible to achive this with a minor patch. Because this value
>>> is used as some static allocated arrays' length (for example, hstates in
>>> mm/hugetlb.c:52) in arch-independent code :(
>>>
>>> This immuture HUGE_MAX_HSTATE value will cause no functional error but
>>> it really makes some objects used only partially when
>>> CONFIG_RISCV_ISA_SVNAPOT is enabled but the platform has no svnapot
>>> support :( I am not sure whether this patch can be accepted with this
>>> feature? Or any other helpful idea to fix this problem?
>>>
>>> Please let me know if I ignore any information about it.
>>
>> I agree that the only problem appears to a waste of memory, particularly
> 
> Caveat about ignorance still applies, how much of a waste of memory are we talking?
> 
>> if cgroups are enabled. I don't see any easy solution either. Maybe a
>> warning in the Kconfig text would be sufficient?
> 
> I think that entirely depends on how wasteful it is.
> If it's minor, sure?
> 

I am also not sure how minor this waste will be. But I believe it will
be acceptable. Because before this patchset, we also use only 1/2 of
HUGE_MAX_HSTATE value (which is 2) when CONFIG_64BIT is enabled and
CONFIG_CONTIG_ALLOC is disabled. As HUGE_MAX_HSTATE is a 'max' value,
it is normal to use it up only sometimes but not always.

I prefer to do as Andrew said. To add an notification about this waste
in KConfig text, and still use the immuture HUGE_MAX_HSTATE value.

@Andrew @Conor what do you think of this explanation and this solution?

Please let me know if I ignore anything. Thanks a lot!

Regards,
Qinglin.

> Thanks,
> Conor.