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

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

[zhangyiru]

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.

Signed-off-by: zhangyiru <zhangyiru3@huawei.com>
---
Changelog:
 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 | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index cdfb1ae78a3f..5ff3418759ed 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1467,13 +1467,12 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
 		*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",
+			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);
 		}
+		*ucounts = NULL;
+		return ERR_PTR(-EPERM);
 	}
 
 	file = ERR_PTR(-ENOSPC);
-- 
2.27.0

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

[Mike Kravetz]

On 10/9/21 2:43 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.
> 
> Signed-off-by: zhangyiru <zhangyiru3@huawei.com>

Thanks,
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

It will be interesting to see if anyone notices the change in behavior.
My 'guess' is that it will not be noticed until this is picked up by
distros, which may take quite some time.
-- 
Mike Kravetz

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

[Hugh Dickins]

On Sat, 9 Oct 2021, 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.
> 
> Signed-off-by: zhangyiru <zhangyiru3@huawei.com>
> ---
> Changelog:
>  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 | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index cdfb1ae78a3f..5ff3418759ed 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -1467,13 +1467,12 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
>  		*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",
> +			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);
>  		}
> +		*ucounts = NULL;
> +		return ERR_PTR(-EPERM);
>  	}
>  
>  	file = ERR_PTR(-ENOSPC);
> -- 
> 2.27.0

I have nothing against the obsoletion itself;
but this patch appears to be incorrect, and far from complete.

Incorrect (unless we actually want to *punish* those who still use
deprecated interfaces) because in these latest versions, it consumes
from the mlock ulimit (if use_shm_lock() succeeds), but never gives
back what it consumed.  I don't see why user_shm_lock() is called.

Incomplete, because further down (at the "out" label) there's
a pointless call to user_shm_unlock(), which should be removed.

Far from complete, because why is there even a ucounts argument to
hugetlb_file_setup() now?  That should be removed too, and ipc/shm.c
adjusted (but it still needs shp->mlock_ucounts for the shmem case).

(Sorry, I'm being totally unresponsive at present, needing to focus
on something else; but thought I'd better get these comments in before
mmotm's mmhugetlb-remove-mlock-ulimit-for-shm_hugetlb.patch goes to 5.16.)

Hugh

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

[zhangyiru]

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.

Signed-off-by: zhangyiru <zhangyiru3@huawei.com>
---
Changelog:
 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 | 11 +++--------
 ipc/shm.c            |  3 +--
 mm/memfd.c           |  4 +---
 mm/mmap.c            |  3 +--
 4 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index cdfb1ae78a3f..c7ef2ca16ba7 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,7 +1458,6 @@ 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);
@@ -1466,14 +1465,10 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
 	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",
 				current->comm, current->pid);
-			task_unlock(current);
-		} else {
-			*ucounts = NULL;
+			user_shm_unlock(size, *ucounts);
 			return ERR_PTR(-EPERM);
-		}
 	}
 
 	file = ERR_PTR(-ENOSPC);
diff --git a/ipc/shm.c b/ipc/shm.c
index 748933e376ca..64f34aa074d1 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -650,8 +650,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 		if (shmflg & SHM_NORESERVE)
 			acctflag = VM_NORESERVE;
 		file = hugetlb_file_setup(name, hugesize, acctflag,
-				  &shp->mlock_ucounts, HUGETLB_SHMFS_INODE,
-				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK);
+				, HUGETLB_SHMFS_INODE, (shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK);
 	} else {
 		/*
 		 * Do not allow no accounting for OVERCOMMIT_NEVER, even
diff --git a/mm/memfd.c b/mm/memfd.c
index 081dd33e6a61..9f80f162791a 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -297,9 +297,7 @@ SYSCALL_DEFINE2(memfd_create,
 	}
 
 	if (flags & MFD_HUGETLB) {
-		struct ucounts *ucounts = NULL;
-
-		file = hugetlb_file_setup(name, 0, VM_NORESERVE, &ucounts,
+		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
 					HUGETLB_ANONHUGE_INODE,
 					(flags >> MFD_HUGE_SHIFT) &
 					MFD_HUGE_MASK);
diff --git a/mm/mmap.c b/mm/mmap.c
index ca54d36d203a..da5b229e05b4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1608,7 +1608,6 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 			goto out_fput;
 		}
 	} else if (flags & MAP_HUGETLB) {
-		struct ucounts *ucounts = NULL;
 		struct hstate *hs;
 
 		hs = hstate_sizelog((flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
@@ -1624,7 +1623,7 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 		 */
 		file = hugetlb_file_setup(HUGETLB_ANON_FILE, len,
 				VM_NORESERVE,
-				&ucounts, HUGETLB_ANONHUGE_INODE,
+				HUGETLB_ANONHUGE_INODE,
 				(flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
 		if (IS_ERR(file))
 			return PTR_ERR(file);
-- 
2.27.0

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

[zhangyiru]

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.

Signed-off-by: zhangyiru <zhangyiru3@huawei.com>
---
Changelog:
 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    | 11 ++++-------
 include/linux/hugetlb.h |  3 +--
 ipc/shm.c               |  8 +-------
 mm/memfd.c              |  4 +---
 mm/mmap.c               |  3 +--
 5 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index cdfb1ae78a3f..363eec008390 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1446,19 +1446,19 @@ 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;
 	int hstate_idx;
 	struct file *file;
+	struct ucounts **ucounts;
 
 	hstate_idx = get_hstate_idx(page_size_log);
 	if (hstate_idx < 0)
 		return ERR_PTR(-ENODEV);
 
-	*ucounts = NULL;
 	mnt = hugetlbfs_vfsmount[hstate_idx];
 	if (!mnt)
 		return ERR_PTR(-ENOENT);
@@ -1466,12 +1466,9 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
 	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",
 				current->comm, current->pid);
-			task_unlock(current);
-		} else {
-			*ucounts = NULL;
+			user_shm_unlock(size, *ucounts);
 			return ERR_PTR(-EPERM);
 		}
 	}
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)
 {
diff --git a/ipc/shm.c b/ipc/shm.c
index 748933e376ca..a55d1755c8cc 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -287,9 +287,6 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
 	shm_unlock(shp);
 	if (!is_file_hugepages(shm_file))
 		shmem_lock(shm_file, 0, shp->mlock_ucounts);
-	else if (shp->mlock_ucounts)
-		user_shm_unlock(i_size_read(file_inode(shm_file)),
-				shp->mlock_ucounts);
 	fput(shm_file);
 	ipc_update_pid(&shp->shm_cprid, NULL);
 	ipc_update_pid(&shp->shm_lprid, NULL);
@@ -650,8 +647,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 		if (shmflg & SHM_NORESERVE)
 			acctflag = VM_NORESERVE;
 		file = hugetlb_file_setup(name, hugesize, acctflag,
-				  &shp->mlock_ucounts, HUGETLB_SHMFS_INODE,
-				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK);
+				HUGETLB_SHMFS_INODE, (shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK);
 	} else {
 		/*
 		 * Do not allow no accounting for OVERCOMMIT_NEVER, even
@@ -698,8 +694,6 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 no_id:
 	ipc_update_pid(&shp->shm_cprid, NULL);
 	ipc_update_pid(&shp->shm_lprid, NULL);
-	if (is_file_hugepages(file) && shp->mlock_ucounts)
-		user_shm_unlock(size, shp->mlock_ucounts);
 	fput(file);
 	ipc_rcu_putref(&shp->shm_perm, shm_rcu_free);
 	return error;
diff --git a/mm/memfd.c b/mm/memfd.c
index 081dd33e6a61..9f80f162791a 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -297,9 +297,7 @@ SYSCALL_DEFINE2(memfd_create,
 	}
 
 	if (flags & MFD_HUGETLB) {
-		struct ucounts *ucounts = NULL;
-
-		file = hugetlb_file_setup(name, 0, VM_NORESERVE, &ucounts,
+		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
 					HUGETLB_ANONHUGE_INODE,
 					(flags >> MFD_HUGE_SHIFT) &
 					MFD_HUGE_MASK);
diff --git a/mm/mmap.c b/mm/mmap.c
index ca54d36d203a..da5b229e05b4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1608,7 +1608,6 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 			goto out_fput;
 		}
 	} else if (flags & MAP_HUGETLB) {
-		struct ucounts *ucounts = NULL;
 		struct hstate *hs;
 
 		hs = hstate_sizelog((flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
@@ -1624,7 +1623,7 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 		 */
 		file = hugetlb_file_setup(HUGETLB_ANON_FILE, len,
 				VM_NORESERVE,
-				&ucounts, HUGETLB_ANONHUGE_INODE,
+				HUGETLB_ANONHUGE_INODE,
 				(flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
 		if (IS_ERR(file))
 			return PTR_ERR(file);
-- 
2.27.0

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

[Mike Kravetz]

On 11/1/21 2:09 PM, 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.
> 
> Signed-off-by: zhangyiru <zhangyiru3@huawei.com>
> ---
> Changelog:
>  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    | 11 ++++-------
>  include/linux/hugetlb.h |  3 +--
>  ipc/shm.c               |  8 +-------
>  mm/memfd.c              |  4 +---
>  mm/mmap.c               |  3 +--
>  5 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index cdfb1ae78a3f..363eec008390 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -1446,19 +1446,19 @@ 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;
>  	int hstate_idx;
>  	struct file *file;
> +	struct ucounts **ucounts;

How about just moving this to the block where it is used?  And no need
for **ucounts, just struct *ucounts should do.

>  
>  	hstate_idx = get_hstate_idx(page_size_log);
>  	if (hstate_idx < 0)
>  		return ERR_PTR(-ENODEV);
>  
> -	*ucounts = NULL;
>  	mnt = hugetlbfs_vfsmount[hstate_idx];
>  	if (!mnt)
>  		return ERR_PTR(-ENOENT);
> @@ -1466,12 +1466,9 @@ struct file *hugetlb_file_setup(const char *name, size_t size,
>  	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",

Please do change this to "... is obsolete\n".  I am not sure why there
was a v4 with the change back to deprecated?

>  				current->comm, current->pid);
> -			task_unlock(current);
> -		} else {
> -			*ucounts = NULL;
> +			user_shm_unlock(size, *ucounts);
>  			return ERR_PTR(-EPERM);

I believe this error return should be outside this 'if user_shm_lock()'
block.  After the following '}'.

>  		}

		Place return ERR_PTR(-EPERM); here.

>  	}

Also, need to remove the user_shm_unlock after out: in this function.

-- 
Mike Kravetz

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

[zhangyiru]

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

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)
 {
diff --git a/ipc/shm.c b/ipc/shm.c
index 748933e376ca..a55d1755c8cc 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -287,9 +287,6 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
 	shm_unlock(shp);
 	if (!is_file_hugepages(shm_file))
 		shmem_lock(shm_file, 0, shp->mlock_ucounts);
-	else if (shp->mlock_ucounts)
-		user_shm_unlock(i_size_read(file_inode(shm_file)),
-				shp->mlock_ucounts);
 	fput(shm_file);
 	ipc_update_pid(&shp->shm_cprid, NULL);
 	ipc_update_pid(&shp->shm_lprid, NULL);
@@ -650,8 +647,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 		if (shmflg & SHM_NORESERVE)
 			acctflag = VM_NORESERVE;
 		file = hugetlb_file_setup(name, hugesize, acctflag,
-				  &shp->mlock_ucounts, HUGETLB_SHMFS_INODE,
-				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK);
+				HUGETLB_SHMFS_INODE, (shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK);
 	} else {
 		/*
 		 * Do not allow no accounting for OVERCOMMIT_NEVER, even
@@ -698,8 +694,6 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 no_id:
 	ipc_update_pid(&shp->shm_cprid, NULL);
 	ipc_update_pid(&shp->shm_lprid, NULL);
-	if (is_file_hugepages(file) && shp->mlock_ucounts)
-		user_shm_unlock(size, shp->mlock_ucounts);
 	fput(file);
 	ipc_rcu_putref(&shp->shm_perm, shm_rcu_free);
 	return error;
diff --git a/mm/memfd.c b/mm/memfd.c
index 081dd33e6a61..9f80f162791a 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -297,9 +297,7 @@ SYSCALL_DEFINE2(memfd_create,
 	}
 
 	if (flags & MFD_HUGETLB) {
-		struct ucounts *ucounts = NULL;
-
-		file = hugetlb_file_setup(name, 0, VM_NORESERVE, &ucounts,
+		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
 					HUGETLB_ANONHUGE_INODE,
 					(flags >> MFD_HUGE_SHIFT) &
 					MFD_HUGE_MASK);
diff --git a/mm/mmap.c b/mm/mmap.c
index ca54d36d203a..da5b229e05b4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1608,7 +1608,6 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 			goto out_fput;
 		}
 	} else if (flags & MAP_HUGETLB) {
-		struct ucounts *ucounts = NULL;
 		struct hstate *hs;
 
 		hs = hstate_sizelog((flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
@@ -1624,7 +1623,7 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 		 */
 		file = hugetlb_file_setup(HUGETLB_ANON_FILE, len,
 				VM_NORESERVE,
-				&ucounts, HUGETLB_ANONHUGE_INODE,
+				HUGETLB_ANONHUGE_INODE,
 				(flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
 		if (IS_ERR(file))
 			return PTR_ERR(file);
-- 
2.27.0

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

[Mike Kravetz]

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

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

[zhangyiru]

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:
 v8: modify !CONFIG_HUGETLBFS version of hugetlb_file_setup()
 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 |  6 ++----
 ipc/shm.c               |  8 +-------
 mm/memfd.c              |  4 +---
 mm/mmap.c               |  3 +--
 5 files changed, 13 insertions(+), 31 deletions(-)

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..74eb4f7f4eab 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)
 {
@@ -488,8 +487,7 @@ static inline struct hstate *hstate_inode(struct inode *i)
 #define is_file_hugepages(file)			false
 static inline 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)
+		int creat_flags, int page_size_log)
 {
 	return ERR_PTR(-ENOSYS);
 }
diff --git a/ipc/shm.c b/ipc/shm.c
index 748933e376ca..a55d1755c8cc 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -287,9 +287,6 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
 	shm_unlock(shp);
 	if (!is_file_hugepages(shm_file))
 		shmem_lock(shm_file, 0, shp->mlock_ucounts);
-	else if (shp->mlock_ucounts)
-		user_shm_unlock(i_size_read(file_inode(shm_file)),
-				shp->mlock_ucounts);
 	fput(shm_file);
 	ipc_update_pid(&shp->shm_cprid, NULL);
 	ipc_update_pid(&shp->shm_lprid, NULL);
@@ -650,8 +647,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 		if (shmflg & SHM_NORESERVE)
 			acctflag = VM_NORESERVE;
 		file = hugetlb_file_setup(name, hugesize, acctflag,
-				  &shp->mlock_ucounts, HUGETLB_SHMFS_INODE,
-				(shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK);
+				HUGETLB_SHMFS_INODE, (shmflg >> SHM_HUGE_SHIFT) & SHM_HUGE_MASK);
 	} else {
 		/*
 		 * Do not allow no accounting for OVERCOMMIT_NEVER, even
@@ -698,8 +694,6 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 no_id:
 	ipc_update_pid(&shp->shm_cprid, NULL);
 	ipc_update_pid(&shp->shm_lprid, NULL);
-	if (is_file_hugepages(file) && shp->mlock_ucounts)
-		user_shm_unlock(size, shp->mlock_ucounts);
 	fput(file);
 	ipc_rcu_putref(&shp->shm_perm, shm_rcu_free);
 	return error;
diff --git a/mm/memfd.c b/mm/memfd.c
index 081dd33e6a61..9f80f162791a 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -297,9 +297,7 @@ SYSCALL_DEFINE2(memfd_create,
 	}
 
 	if (flags & MFD_HUGETLB) {
-		struct ucounts *ucounts = NULL;
-
-		file = hugetlb_file_setup(name, 0, VM_NORESERVE, &ucounts,
+		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
 					HUGETLB_ANONHUGE_INODE,
 					(flags >> MFD_HUGE_SHIFT) &
 					MFD_HUGE_MASK);
diff --git a/mm/mmap.c b/mm/mmap.c
index ca54d36d203a..da5b229e05b4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1608,7 +1608,6 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 			goto out_fput;
 		}
 	} else if (flags & MAP_HUGETLB) {
-		struct ucounts *ucounts = NULL;
 		struct hstate *hs;
 
 		hs = hstate_sizelog((flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
@@ -1624,7 +1623,7 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
 		 */
 		file = hugetlb_file_setup(HUGETLB_ANON_FILE, len,
 				VM_NORESERVE,
-				&ucounts, HUGETLB_ANONHUGE_INODE,
+				HUGETLB_ANONHUGE_INODE,
 				(flags >> MAP_HUGE_SHIFT) & MAP_HUGE_MASK);
 		if (IS_ERR(file))
 			return PTR_ERR(file);
-- 
2.27.0

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

[Mike Kravetz]

On 11/3/21 3:58 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:
>  v8: modify !CONFIG_HUGETLBFS version of hugetlb_file_setup()
>  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 |  6 ++----
>  ipc/shm.c               |  8 +-------
>  mm/memfd.c              |  4 +---
>  mm/mmap.c               |  3 +--
>  5 files changed, 13 insertions(+), 31 deletions(-)

Thanks,

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

Andrew, the version you previously added to your tree (v4 I believe) has
an issue as reported by Hugh and should be replaced with this version.
Sorry!
-- 
Mike Kravetz

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

[Mike Kravetz]

On 11/1/21 12:01 AM, Hugh Dickins wrote:
> On Sat, 9 Oct 2021, zhangyiru wrote:
>>  fs/hugetlbfs/inode.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> I have nothing against the obsoletion itself;
> but this patch appears to be incorrect, and far from complete.
> 
> Incorrect (unless we actually want to *punish* those who still use
> deprecated interfaces) because in these latest versions, it consumes
> from the mlock ulimit (if use_shm_lock() succeeds), but never gives
> back what it consumed.  I don't see why user_shm_lock() is called.

Thanks Hugh!

I was so focused on how how this might impact people still using the
deprecated feature, I missed this.

Since I have seen the 'this is deprecated' message as recently as this
year, I suspect this will impact someone.  The way it will impact them
is that their application will no longer work due to shmget() failing.
This is why it would be good idea to at least log a message saying someone
tried to use the obsolete feature.

The most convenient way to  determine if the call would have succeeded due
to the obsolete feature, is by using the existing user_shm_lock/unlock calls.
I suppose we could create a separate routine to just check the limit so that
a message can be printed.  Or, possibly open code.  But, I would advocate
for removing the 'this is obsolete' message in the next release.  When
we remove the message, we can eliminate the calls to user_shm_lock/unlock.

-- 
Mike Kravetz