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

Mike Kravetz
2019-01-09T19:23:42Z

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.

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. :(

--
Mike Kravetz