Re: [PATCH 0/3] Hugetlbfs support for riscv

Palmer Dabbelt
2019-01-09T22:15:28Z

On Wed, 09 Jan 2019 11:23:22 PST (-0800), mike.kravetz@oracle.com wrote:
> On 12/9/18 10:21 PM, Alexandre Ghiti wrote:
>> This series introduces hugetlbfs support for both riscv 32/64. Riscv32
>> is architecturally limited to huge pages of size 4MB whereas riscv64 has
>> 2MB/1G huge pages support. Transparent huge page support is not
>> implemented here, I will submit another series later.
>
> Thanks for doing this.
>
> Since patches only touch riscv specific code, I did not look too closely
> and do not feel qualified to offer an opinion as to whether they are correct
> for the architecture.o

Sorry about that. These appear to have missed by inbox somehow. I think they
did manage to get caught by patchwork

https://patchwork.kernel.org/cover/10720635/

I'll take a look...

> With that said, the patches do look reasonable with a few comments below.
>
>> CMA or (MEMORY_ISOLATION && COMPACTION) must be enabled so that boot
>> reserved gigantic pages can be freed: indeed, one can reduce the number
>> of huge pages by calling free_gigantic_pages which in turn calls
>> free_contig_range, defined only with those configs defined.
>> However I don't see any strong dependency between free_contig_range
>> and those configs, maybe we could allow hugetlbfs users to free boot
>> reserved hugepages without those configs activated, I will propose
>> something if Mike Kravetz agrees.
>
> Yes, that should be modified. I think it would be a simple matter of moving
> free_contig_range out of that ifdef block. If you would like, I can submit
> a patch for that.
>
> Somewhat related, I do not think your patches enable dynamic allocation of
> gigantic pages. Not sure if that was an oversight or conscious decision.
> I am not sure this is a highly used feature, but if you can do it on riscv
> then why not?
>
> Another 'missing feature' in your patches is PMD sharing. This feature if
> only of value for BIG shared hugetlbfs mappings. DB folks like it. As
> mentioned above, I do not know much about riscv so I do not know if this
> might be of use to potential users.
>
>> - libhugetlbfs testsuite on riscv64/1G:
>> - brk_near_huge triggers an assert in malloc.c, does not on x86.
>> - mmap-gettest, mmap-cow: testsuite passes the number of default free
>> pages as parameters and then fails for 1G which is not the default.
>> Otherwise succeeds when given the right number of pages.
>> - map_high_truncate_2 fails on x86 too: 0x60000000 is not 1G aligned
>> and fails at line 694 of fs/hugetlbfs/inode.c.
>> - heapshrink on 1G fails on x86 too, not investigated.
>> - counters.sh on 1G fails on x86 too: alloc_surplus_huge_page returns
>> NULL in case of gigantic pages.
>> - icache-hygiene succeeds after patch #3 of this series which lowers
>> the base address of mmap.
>> - fallocate_stress.sh on 1G never ends, on x86 too, not investigated.
>
> In general, libhugetlbfs tests seem to have issues with anything besides
> default huge page size. You encountered that and noted that tests break
> for 1G pages on x86 as well. Cleaning all that up has been on my todo list
> for years, but there always seems to be something of higher priority. :(