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

[Alex Ghiti <alex@ghiti.fr>]

On 1/9/19 10:15 PM, Palmer Dabbelt wrote:
> 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...
>

No problem Palmer :)

Thanks,


>> 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. :(
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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