Re: [RFC PATCH 3/3] mm: shm: Use new hugetlb size encoding definitions

[Mike Kravetz <mike.kravetz@oracle.com>]

On 07/27/2017 12:50 AM, Michal Hocko wrote:
> On Wed 26-07-17 10:39:30, Mike Kravetz wrote:
>> On 07/26/2017 03:07 AM, Michal Hocko wrote:
>>> On Wed 26-07-17 11:53:38, Michal Hocko wrote:
>>>> On Mon 17-07-17 15:28:01, Mike Kravetz wrote:
>>>>> Use the common definitions from hugetlb_encode.h header file for
>>>>> encoding hugetlb size definitions in shmget system call flags.  In
>>>>> addition, move these definitions to the from the internal to user
>>>>> (uapi) header file.
>>>>
>>>> s@to the from@from@
>>>>
>>>>>
>>>>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>>>>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>>>>
>>>> with s@HUGETLB_FLAG_ENCODE__16GB@HUGETLB_FLAG_ENCODE_16GB@
>>>>
>>>> Acked-by: Michal Hocko <mhocko@suse.com>
>>>
>>> Btw. man page mentions only 2MB and 1GB, we should document others and
>>> note that each arch might support only subset of them
>>
>> Thanks for looking at these Michal.
>> BTW, those definitions below are wrong.  They should be SHM_HUGE_*. :(
> 
> Ups, and I completely missed that.
> 
>> In the overview of this RFC, I mentioned still needing to address the
>> comment from Aneesh about splitting SHM_HUGE_* definitions into arch
>> specific header files.  This is how it is done for mmap.  If an arch
>> supports multiple huge page sizes, the 'asm/mman.h' contains definitions
>> for those sizes.  There will be a bit of churn (such as header file
>> renaming) to do this for shm as well.  So, I keep going back and forth
>> asking myself 'is it worth it'?
> 
> Why cannot we use a generic header? Btw. I think it would be better for
> MMAP definitions as well.

I assume you are asking about a uapi asm-generic header file?  Currently
mmap has two such files:  mman.h and mman-common.h.  In order to get the
definitions in such files, arch specific header files must #include the
asm-generic headers.  There are arch specific mmap headers today that do
not include either of the asm-generic headers.  And, they have their own
definitions for MAP_HUGE_SHIFT.  So, it seems we can not use one of the
existing mmap asm-generic header files.  Rather, we would need to create
a new one and have that included by all arch specific files.

However, ALL the MAP_HUGE_* definitions in all the arch specific and
asm-generic header files are the same.  It would be possible to just put
all those MAP_HUGE_* definitions in the primary uapi header file
(include/uapi/linux/mman.h).  If there was ever a need for arch specific
values in the future, we could split them out at that time.

>> Some things to consider.
>>
>> - We should be consistent between mmap and shm.  Also remember, that I
>>   will propose adding the same type of encoding to memfd_create.  So,
>>   three system calls will use the encoding.  They should be consistent.
> 
> agreed
> 
>> - Adding the arch specific definitions seems the 'most correct', as a
>>   user can not use a definition not supported by the arch.  However,
>>   even if an arch supports a huge page size it does not mean that the
>>   running kernel supports that size.  Therefore, the folllowing is in
>>   the man page.
>>   "The  range  of  huge page sizes that are supported by the system
>>    can be discovered by listing  the  subdirectories  in
>>    /sys/kernel/mm/hugepages."
> 
> Doesn't the respective call return -EINVAL on the unsupported hugepage
> size?

Yes, it does.

>> - Another alternative is to make all known huge page sizes available
>>   to all users.  This is 'easier' as the definitions can likely reside
>>   in a common header file.  The user will  need to determine what
>>   huge page sizes are supported by the running kernel as mentioned in
>>   the man page.
> 
> yes I think this makes more sense.

Ok, thanks.

The only remaining question is what kind of common header to use:
1) An asm-generic header file in case there may be arch specific differences
   in the future.
2) Use the primary uapi header file in include/uapi/linux/mman|shm.h.

-- 
Mike Kravetz