* [PATCH 1/2] devtmpfs: fix placement of complete() call
@ 2021-03-12 10:30 Rasmus Villemoes
2021-03-12 10:30 ` [PATCH 2/2] devtmpfs: actually reclaim some init memory Rasmus Villemoes
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2021-03-12 10:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Christoph Hellwig, Rasmus Villemoes, linux-kernel
Calling complete() from within the __init function is wrong -
theoretically, the init process could proceed all the way to freeing
the init mem before the devtmpfsd thread gets to execute the return
instruction in devtmpfs_setup().
In practice, it seems to be harmless as gcc inlines devtmpfs_setup()
into devtmpfsd(). So the calls of the __init functions init_chdir()
etc. actually happen from devtmpfs_setup(), but the __ref on that one
silences modpost (it's all right, because those calls happen before
the complete()). But it does make the __init annotation of the setup
function moot, which we'll fix in a subsequent patch.
Fixes: bcbacc4909f1 ("devtmpfs: refactor devtmpfsd()")
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
drivers/base/devtmpfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 653c8c6ac7a7..aedeb2dc1a18 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -419,7 +419,6 @@ static int __init devtmpfs_setup(void *p)
init_chroot(".");
out:
*(int *)p = err;
- complete(&setup_done);
return err;
}
@@ -432,6 +431,7 @@ static int __ref devtmpfsd(void *p)
{
int err = devtmpfs_setup(p);
+ complete(&setup_done);
if (err)
return err;
devtmpfs_work_loop();
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] devtmpfs: actually reclaim some init memory
2021-03-12 10:30 [PATCH 1/2] devtmpfs: fix placement of complete() call Rasmus Villemoes
@ 2021-03-12 10:30 ` Rasmus Villemoes
2021-03-12 16:27 ` Christoph Hellwig
2021-03-12 16:26 ` [PATCH 1/2] devtmpfs: fix placement of complete() call Christoph Hellwig
2021-03-18 12:44 ` Rasmus Villemoes
2 siblings, 1 reply; 6+ messages in thread
From: Rasmus Villemoes @ 2021-03-12 10:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Christoph Hellwig, Rasmus Villemoes, linux-kernel
Currently gcc seems to inline devtmpfs_setup() into devtmpfsd(), so
its memory footprint isn't reclaimed as intended. Mark it noinline to
make sure it gets put in .init.text.
While here, setup_done can also be put in .init.data: After complete()
releases the internal spinlock, the completion object is never touched
again by that thread, and the waiting thread doesn't proceed until it
observes ->done while holding that spinlock.
This is now the same pattern as for kthreadd_done in init/main.c:
complete() is done in a __ref function, while the corresponding
wait_for_completion() is in an __init function.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
drivers/base/devtmpfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index aedeb2dc1a18..8be352ab4ddb 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -371,7 +371,7 @@ int __init devtmpfs_mount(void)
return err;
}
-static DECLARE_COMPLETION(setup_done);
+static __initdata DECLARE_COMPLETION(setup_done);
static int handle(const char *name, umode_t mode, kuid_t uid, kgid_t gid,
struct device *dev)
@@ -405,7 +405,7 @@ static void __noreturn devtmpfs_work_loop(void)
}
}
-static int __init devtmpfs_setup(void *p)
+static noinline int __init devtmpfs_setup(void *p)
{
int err;
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] devtmpfs: fix placement of complete() call
2021-03-12 10:30 [PATCH 1/2] devtmpfs: fix placement of complete() call Rasmus Villemoes
2021-03-12 10:30 ` [PATCH 2/2] devtmpfs: actually reclaim some init memory Rasmus Villemoes
@ 2021-03-12 16:26 ` Christoph Hellwig
2021-03-18 12:44 ` Rasmus Villemoes
2 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2021-03-12 16:26 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Christoph Hellwig, linux-kernel
Looks fine:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] devtmpfs: actually reclaim some init memory
2021-03-12 10:30 ` [PATCH 2/2] devtmpfs: actually reclaim some init memory Rasmus Villemoes
@ 2021-03-12 16:27 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2021-03-12 16:27 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Christoph Hellwig, linux-kernel
On Fri, Mar 12, 2021 at 11:30:27AM +0100, Rasmus Villemoes wrote:
> Currently gcc seems to inline devtmpfs_setup() into devtmpfsd(), so
> its memory footprint isn't reclaimed as intended. Mark it noinline to
> make sure it gets put in .init.text.
>
> While here, setup_done can also be put in .init.data: After complete()
> releases the internal spinlock, the completion object is never touched
> again by that thread, and the waiting thread doesn't proceed until it
> observes ->done while holding that spinlock.
>
> This is now the same pattern as for kthreadd_done in init/main.c:
> complete() is done in a __ref function, while the corresponding
> wait_for_completion() is in an __init function.
I'm not sure if this matters in any way, but it does look fine to me:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] devtmpfs: fix placement of complete() call
2021-03-12 10:30 [PATCH 1/2] devtmpfs: fix placement of complete() call Rasmus Villemoes
2021-03-12 10:30 ` [PATCH 2/2] devtmpfs: actually reclaim some init memory Rasmus Villemoes
2021-03-12 16:26 ` [PATCH 1/2] devtmpfs: fix placement of complete() call Christoph Hellwig
@ 2021-03-18 12:44 ` Rasmus Villemoes
2021-03-18 12:53 ` Greg Kroah-Hartman
2 siblings, 1 reply; 6+ messages in thread
From: Rasmus Villemoes @ 2021-03-18 12:44 UTC (permalink / raw)
To: Rasmus Villemoes, Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Christoph Hellwig, linux-kernel
On 12/03/2021 11.30, Rasmus Villemoes wrote:
> Calling complete() from within the __init function is wrong -
> theoretically, the init process could proceed all the way to freeing
> the init mem before the devtmpfsd thread gets to execute the return
> instruction in devtmpfs_setup().
Greg, ping? Also for the other one.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] devtmpfs: fix placement of complete() call
2021-03-18 12:44 ` Rasmus Villemoes
@ 2021-03-18 12:53 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2021-03-18 12:53 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Rafael J. Wysocki, Christoph Hellwig, linux-kernel
On Thu, Mar 18, 2021 at 01:44:04PM +0100, Rasmus Villemoes wrote:
> On 12/03/2021 11.30, Rasmus Villemoes wrote:
> > Calling complete() from within the __init function is wrong -
> > theoretically, the init process could proceed all the way to freeing
> > the init mem before the devtmpfsd thread gets to execute the return
> > instruction in devtmpfs_setup().
>
> Greg, ping? Also for the other one.
I'll pick this up for my tree, give me a chance to catch up, there's no
rush with this one :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-18 12:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 10:30 [PATCH 1/2] devtmpfs: fix placement of complete() call Rasmus Villemoes
2021-03-12 10:30 ` [PATCH 2/2] devtmpfs: actually reclaim some init memory Rasmus Villemoes
2021-03-12 16:27 ` Christoph Hellwig
2021-03-12 16:26 ` [PATCH 1/2] devtmpfs: fix placement of complete() call Christoph Hellwig
2021-03-18 12:44 ` Rasmus Villemoes
2021-03-18 12:53 ` Greg Kroah-Hartman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.