Re: [PATCH] mm: introduce MAP_FIXED_HUGETLB_LEN to mmap()

["Li Xinhai" <lixinhai.lxh@gmail.com>]

On 2020-03-28 at 10:19 Li Xinhai wrote:
>On 2020-03-28 at 09:31 Mike Kravetz wrote:
>>On 3/27/20 12:12 PM, John Hubbard wrote:
>>> On 3/27/20 5:59 AM, Li Xinhai wrote:
>>>> The purpose of MAP_FIXED_HUGETLB_LEN is to check whether the parameter
>>>> length is valid or not according to the target file's huge page size.
>>>> When it is used, if length is not aligned to underlying huge page size,
>>>> mmap() is failed with errno set to EINVAL. When it is not used, the
>>>> current semantic is maintained, i.e., length is round up to underlying
>>>> huge page size.
>>>>
>>>> In current code, the vma related call, except mmap, are all consider
>>>> not correctly aligned length as invalid parameter, including mprotect,
>>>> munmap, mlock, etc., by checking through hugetlb_vm_op_split. So, user
>>>> will see failure, after successfully call mmap, although using same
>>>> length parameter to other mapping syscall.
>>>>
>>>> With MAP_FIXED_HUGETLB_LEN, user can choose to check if length is
>>>> correctly aligned at first place when call mmap, instead of failure after
>>>> mapping has been created.
>>>
>>> Hi Li,
>>>
>>> This is not worth creating a new MAP_ flag. If you look at the existing flags
>>> you will see that they are both limited and carefully chosen, so as to cover
>>> a reasonable chunk of functionality per flag. We don't just drop in a flag
>>> for tiny corner cases like this one.
>>>
>>> btw, remember that user API changes require man pages updates as well. And
>>> that the API has to be supported forever. And that if we use up valuable
>>> flag slots on trivia then we'll run out of flags quite soon, and won't be
>>> able to do broader, more important upgrades.
>>>
>>> Also, we need to include a user space API mailing list for things that
>>> affect that. Adding them now: Linux API <linux-api@vger.kernel.org>
>>> The man pages mailing list will also be needed if we go there.
>>>
>>> Let's take a closer look at your problem and see what it takes to solve it.
>>> If we need some sort of flag to mmap() or other routines, fine. But so far,
>>> I can see at least two solutions that are much easier:
>>
>>I too question the motivation for this patch.  Is it simply to eliminate some
>>of the hugetlb special behavior and make it behave more like the rest of mm?
>>
>>> Solution idea #2: just do the length check unconditionally here (without looking
>>> at a new flag), and return an error if it is not aligned. And same thing for the
>>> MAP_HUGETLB case below. And delete the "len = ALIGN(len, huge_page_size(hs));" in
>>> both cases.
>>>
>>> That would still require a man page update, and consensus that it won't Break
>>> The World, but it's possible (I really don't know) that this is a more common
>>> and desirable behavior.
>>>
>>> Let's see if anyone else weighs in about this.
>>
>>That certainly would be the easiest thing to do.  However, I'm guessing
>>the current behavior was added when hugetlb mmap support was added. 
>
>>There is no telling how many applications might break if we change the behavior.
>>I'm guessing this is the reason Li chose to only change the behavior if
>>a new flag was specified.
>Yes, I was considering this change would break something.
> 
It's better to have a new patch which don't introduce new flag, and I attached
statements from the kernel document.
https://lore.kernel.org/linux-api/1585451295-22302-1-git-send-email-lixinhai.lxh@gmail.com/

Let's have further check, thanks!

>>--
>>Mike Kravetz