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

[Hugh Dickins <hughd@google.com>]

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