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

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

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.

>--
>Mike Kravetz