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

Andrew Jones
2022-12-09T07:42:52Z

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
if cgroups are enabled. I don't see any easy solution either. Maybe a
warning in the Kconfig text would be sufficient?

Thanks,
drew