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

[Alex Ghiti <alex@ghiti.fr>]

On 1/9/19 7:23 PM, Mike Kravetz 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.
>
> 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.


I think there is more to do: free_gigantic_page is only defined with
ARCH_HAS_GIGANTIC_PAGE which in turn is only defined with
CMA || (MEMORY_ISOLATION && COMPACTION). Moreover, if
ARCH_HAS_GIGANTIC_PAGE is not defined, gigantic_page_supported
will return false and then function like update_and_free_page will
not reach the call to free_gigantic_page...etc. I will send you a patch
when I have fixed all of 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?

I'm not sure to understand: do you mean picking an already allocated
gigantic page or really allocating it with alloc_gigantic_page ? Or
something else ?


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


Thanks for that, I was unaware of that feature, I will take a look.


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

No problem, running the testsuite on x86 was quite simple anyway :)

Thanks Mike for your feedback,

Alex



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv