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

On 2020-03-28 at 10:19 Li Xinhai wrote:

On 2020-03-28 at 09:31 Mike Kravetz wrote:

quoted

On 3/27/20 12:12 PM, John Hubbard wrote:

quoted

On 3/27/20 5:59 AM, Li Xinhai wrote:

quoted

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 [off-list ref]
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?

quoted

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. 

quoted

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/ (local)

Let's have further check, thanks!

quoted

--
Mike Kravetz