| Message ID | 20211009094355.127705-1-zhangyiru3@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] mm,hugetlb: remove mlock ulimit for SHM_HUGETLB | expand |
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.
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
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.
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);
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(-)