On Mon, Aug 24, 2009 at 11:30, Hugh Dickins wrote:
> --- 2.6.31-rc7/ipc/shm.c 2009-06-25 05:18:09.000000000 +0100
> +++ linux/ipc/shm.c 2009-08-24 16:06:30.000000000 +0100
> @@ -174,7 +174,7 @@ static void shm_destroy(struct ipc_names
> shm_unlock(shp);
> if (!is_file_hugepages(shp->shm_file))
> shmem_lock(shp->shm_file, 0, shp->mlock_user);
> - else
> + else if (shp->mlock_user)
> user_shm_unlock(shp->shm_file->f_path.dentry->d_inode->i_size,
> shp->mlock_user);
> fput (shp->shm_file);
> @@ -369,8 +369,8 @@ static int newseg(struct ipc_namespace *
> /* hugetlb_file_setup applies strict accounting */
> if (shmflg & SHM_NORESERVE)
> acctflag = VM_NORESERVE;
> - file = hugetlb_file_setup(name, size, acctflag);
> - shp->mlock_user = current_user();
> + file = hugetlb_file_setup(name, size, acctflag,
> + &shp->mlock_user);
> } else {
> /*
> * Do not allow no accounting for OVERCOMMIT_NEVER, even
> @@ -410,6 +410,8 @@ static int newseg(struct ipc_namespace *
> return error;
>
> no_id:
> + if (shp->mlock_user) /* shmflg & SHM_HUGETLB case */
> + user_shm_unlock(size, shp->mlock_user);
> fput(file);
> no_file:
> security_shm_free(shp);
this breaks on no-mmu systems due to user_shm_unlock() being
mmu-specific. normally gcc is smart enough to do dead code culling so
it hasnt caused problems, but not here. hugetlb support is not
available on no-mmu systems, so the stubbed hugepage functions prevent
calls to user_shm_unlock() and such, but here gcc cant figure it out:
static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
{
...
shp->mlock_user = NULL;
...
if (shmflg & SHM_HUGETLB) {
/* hugetlb_file_setup applies strict accounting */
if (shmflg & SHM_NORESERVE)
acctflag = VM_NORESERVE;
file = hugetlb_file_setup(name, size, acctflag,
&shp->mlock_user);
...
id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
if (id < 0) {
error = id;
goto no_id;
}
...
no_id:
if (shp->mlock_user) /* shmflg & SHM_HUGETLB case */
user_shm_unlock(size, shp->mlock_user);
...
hugetlb_file_setup() expands to nothing and so mlock_user will never
come back from NULL, but gcc still emits a reference to
user_shm_unlock() in the error path. perhaps the best thing here is
to just add an #ifdef ?
no_id:
+#ifdef CONFIG_HUGETLB_PAGE
+ /* gcc isn't smart enough to see that mlock_user goes non-NULL
only by hugetlb */
if (shp->mlock_user) /* shmflg & SHM_HUGETLB case */
user_shm_unlock(size, shp->mlock_user);
+#endif
-mike
On Fri, 11 Sep 2009, Mike Frysinger wrote:
> On Mon, Aug 24, 2009 at 11:30, Hugh Dickins wrote:
> >
> > no_id:
> > + if (shp->mlock_user) /* shmflg & SHM_HUGETLB case */
> > + user_shm_unlock(size, shp->mlock_user);
> > fput(file);
> > no_file:
> > security_shm_free(shp);
>
> this breaks on no-mmu systems due to user_shm_unlock() being
> mmu-specific. normally gcc is smart enough to do dead code culling so
> it hasnt caused problems, but not here. hugetlb support is not
> available on no-mmu systems, so the stubbed hugepage functions prevent
> calls to user_shm_unlock() and such, but here gcc cant figure it out:
>
...
>
> hugetlb_file_setup() expands to nothing and so mlock_user will never
> come back from NULL, but gcc still emits a reference to
> user_shm_unlock() in the error path. perhaps the best thing here is
> to just add an #ifdef ?
> no_id:
> +#ifdef CONFIG_HUGETLB_PAGE
> + /* gcc isn't smart enough to see that mlock_user goes non-NULL
> only by hugetlb */
> if (shp->mlock_user) /* shmflg & SHM_HUGETLB case */
> user_shm_unlock(size, shp->mlock_user);
> +#endif
Many thanks for reporting that, Mike.
Sorry, I've messed up both 2.6.31 final and 2.6.30.6 stable.
My preference is to avoid the #ifdef and use precisely the same
optimization technique as is working for it elsewhere.
Patch follows immediately in separate mail.
Hugh
My 353d5c30c666580347515da609dd74a2b8e9b828 "mm: fix hugetlb bug due to
user_shm_unlock call" broke the CONFIG_SYSVIPC !CONFIG_MMU build of both
2.6.31 and 2.6.30.6: "undefined reference to `user_shm_unlock'".
gcc didn't understand my comment! so couldn't figure out to optimize
away user_shm_unlock() from the error path in the hugetlb-less case,
as it does elsewhere. Help it to do so, in a language it understands.
Reported-by: Mike Frysinger <[email protected]>
Signed-off-by: Hugh Dickins <[email protected]>
Cc: [email protected]
---
ipc/shm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- 2.6.31/ipc/shm.c 2009-09-09 23:13:59.000000000 +0100
+++ linux/ipc/shm.c 2009-09-12 11:27:00.000000000 +0100
@@ -410,7 +410,7 @@ static int newseg(struct ipc_namespace *
return error;
no_id:
- if (shp->mlock_user) /* shmflg & SHM_HUGETLB case */
+ if (is_file_hugepages(file) && shp->mlock_user)
user_shm_unlock(size, shp->mlock_user);
fput(file);
no_file:
On Sat, Sep 12, 2009 at 07:21, Hugh Dickins wrote:
> My 353d5c30c666580347515da609dd74a2b8e9b828 "mm: fix hugetlb bug due to
> user_shm_unlock call" broke the CONFIG_SYSVIPC !CONFIG_MMU build of both
> 2.6.31 and 2.6.30.6: "undefined reference to `user_shm_unlock'".
>
> gcc didn't understand my comment! so couldn't figure out to optimize
> away user_shm_unlock() from the error path in the hugetlb-less case,
> as it does elsewhere. Help it to do so, in a language it understands.
thanks, this works for me
> Cc: [email protected]
should go into 2.6.30.7 and 2.6.31.1
-mike