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

[Andrew Jones]

On Fri, Dec 09, 2022 at 11:36:53PM +0800, Qinglin Pan wrote:
> 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?

To get a better estimate of the waste it should be possible to write a
quick kernel module which outputs sizeof on a few structs. But, from a
quick read of the structs, I think we're probably fine. Certainly it looks
like without CGROUP_HUGETLB and with a small MAX_NUMNODES it's no big
deal. With CGROUP_HUGETLB it's definitely more, but probably OK.
MAX_NUMNODES probably won't ever be huge, so, while there are five arrays
of MAX_NUMNODES elements in hstates, it's unlikely to matter much,
especially since the types of the array elements are small.

It'd be nice to get actual numbers, but based on that analysis I think I'm
OK with the comment in Kconfig.

Thanks,
drew

> 
> Please let me know if I ignore anything. Thanks a lot!
> 
> Regards,
> Qinglin.
> 
> > Thanks,
> > Conor.
>