Re: [PATCH 3/5] mm: Rename __thp_get_unmapped_area to mm_get_unmapped_area_aligned

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Peter Xu <peterx@redhat.com> [250613 09:41]:
> This function is pretty handy for any type of VMA to provide a size-aligned
> VMA address when mmap().  Rename the function and export it.
> 
> About the rename:
> 
>   - Dropping "THP" because it doesn't really have much to do with THP
>     internally.
> 
>   - The suffix "_aligned" imply it is a helper to generate aligned virtual
>     address based on what is specified (which can be not PMD_SIZE).

I am not okay with this.

You are renaming a function to drop thp and not moving it into generic
code.  Either it is a generic function that lives with the generic code,
or drop this change all together.

If this function is going to be generic, please make the return of 0
valid and not an error.  You are masking all errors to 0 currently.

vm_unmapped_area_info has an align_mask, and that's only used for
hugepages. It is wrong to have a generic function that does not use the
generic struct element that exists for this reason.  Is there a reason
that align_mask doesn't work, or why it's not used?

The return of mm_get_unmapped_area_vmflags() is not aligned with the
return of this function.  That is, the address returned from
mm_get_unmapped_area_vmflags() differs from __thp_get_unmapped_area()
based on MMF_TOPDOWN, and/or something related to off_sub?

Anyways, since it's different from mm_get_unmapped_area() in this
regard, we cannot rename it mm_get_unmapped_area_aligned() - it sounds
like a helper and is different, by a lot.

I also am not okay to export it for no reason.

Also, is it okay to export something as gpl or does the copyright holder
need to do that (I have no idea about this stuff, or maybe you work for
the copyright holder)?

The hint (addr) is also never checked for alignment in this function and
we are appending _aligned() to the name.  With this change we can now
get an unaligned _aligned() address.  This (probably) can happen with
MAP_FIXED today, but I don't think we imply it's going to be aligned
elsewhere.


Hate for the Pre-existing code in this function:

Dear lord:
off_sub = (off - ret) & (size - 1);
Using ret here is just (to be polite) unclear to save one assignment.  I
expect the one assignment would be optimised out by the compilers.

My hate for the unsub off_sub grows:
ret += off_sub;
return ret;

It is extremely frustrating that the self-documenting parts of this
function have documentation while poorly named variables are used in
puzzling calculations without any.

...

Thanks,
Liam