Re: [PATCH v7] mm,hugetlb: remove mlock ulimit for SHM_HUGETLB

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

On 11/2/21 8:40 AM, zhangyiru wrote:
> commit 21a3c273f88c9cbbaf7e ("mm, hugetlb: add thread name and pid to
> SHM_HUGETLB mlock rlimit warning") marked this as deprecated in 2012,
> but it is not deleted yet.
> 
> Mike says he still see that message in log files on occasion,
> so maybe we should preserve this warning.
> 
> Also remove hugetlbfs related user_shm_unlock in ipc/shm.c and remove
> the user_shm_unlock after out.
> 
> Signed-off-by: zhangyiru <zhangyiru3@huawei.com>
> ---
> Changelog:
>  v7: modify ucounts definition and remove the user_shm_unlock after out
>  v6: fix errors in v5 and remove hugetlbfs related user_shm_unlock in
> ipc/shm.c
>  v5: delete ucounts variable, and function calls that use ucounts in
> additon
>  v4: modify context information of obsolete
>  v3: modify warning message to obsolete
>  v2: preserve warning message
>  v1: remove mlock ulimit for SHM_HUGETLB
> ---
>  fs/hugetlbfs/inode.c    | 23 ++++++++---------------
>  include/linux/hugetlb.h |  3 +--
>  ipc/shm.c               |  8 +-------
>  mm/memfd.c              |  4 +---
>  mm/mmap.c               |  3 +--
>  5 files changed, 12 insertions(+), 29 deletions(-)

Thanks for continuing to make updates.

I think the use of user_shm_lock/unlock as done below to check for
potential users of the obsolete feature is the simplest thing to do.
If Hugh or someone else has another opinion, please speak up.

> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index cdfb1ae78a3f..49d2e686be74 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -1446,8 +1446,8 @@ static int get_hstate_idx(int page_size_log)
>   * otherwise hugetlb_reserve_pages reserves one less hugepages than intended.
>   */
>  struct file *hugetlb_file_setup(const char *name, size_t size,
> -				vm_flags_t acctflag, struct ucounts **ucounts,
> -				int creat_flags, int page_size_log)
> +				vm_flags_t acctflag, int creat_flags,
> +				int page_size_log)
>  {
>  	struct inode *inode;
>  	struct vfsmount *mnt;
> @@ -1458,22 +1458,19 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
>  	if (hstate_idx < 0)
>  		return ERR_PTR(-ENODEV);
>  
> -	*ucounts = NULL;
>  	mnt = hugetlbfs_vfsmount[hstate_idx];
>  	if (!mnt)
>  		return ERR_PTR(-ENOENT);
>  
>  	if (creat_flags == HUGETLB_SHMFS_INODE && !can_do_hugetlb_shm()) {
> -		*ucounts = current_ucounts();
> -		if (user_shm_lock(size, *ucounts)) {
> -			task_lock(current);
> -			pr_warn_once("%s (%d): Using mlock ulimits for SHM_HUGETLB is deprecated\n",
> +		struct ucounts *ucounts = current_ucounts();
> +
> +		if (user_shm_lock(size, ucounts)) {
> +			pr_warn_once("%s (%d): Using mlock ulimits for SHM_HUGETLB is obsolete\n",
>  				current->comm, current->pid);
> -			task_unlock(current);
> -		} else {
> -			*ucounts = NULL;
> -			return ERR_PTR(-EPERM);
> +			user_shm_unlock(size, ucounts);
>  		}
> +		return ERR_PTR(-EPERM);
>  	}
>  
>  	file = ERR_PTR(-ENOSPC);
> @@ -1498,10 +1495,6 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
>  
>  	iput(inode);
>  out:
> -	if (*ucounts) {
> -		user_shm_unlock(size, *ucounts);
> -		*ucounts = NULL;
> -	}
>  	return file;
>  }
>  
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index f7ca1a3870ea..e8e033b74b7e 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -468,8 +468,7 @@ static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
>  extern const struct file_operations hugetlbfs_file_operations;
>  extern const struct vm_operations_struct hugetlb_vm_ops;
>  struct file *hugetlb_file_setup(const char *name, size_t size, vm_flags_t acct,
> -				struct ucounts **ucounts, int creat_flags,
> -				int page_size_log);
> +				int creat_flags, int page_size_log);
>  
>  static inline bool is_file_hugepages(struct file *file)
>  {

You forgot to make the same change to the !CONFIG_HUGETLBFS version of
hugetlb_file_setup() within include/linux/hugetlb.h.

The rest of the changes look good to me.
-- 
Mike Kravetz