[PATCH v2.6.36-rc7] init: don't call flush_scheduled_work() from do_initcalls()

[PATCH v2.6.36-rc7] init: don't call flush_scheduled_work() from do_initcalls()

[Tejun Heo]

It's unclear what flush_scheduled_work() in do_initcalls() tries to
achieve.  The call doesn't make much sense - there already are
multiple system workqueues which aren't affected by
flush_scheduled_wokr() and subsystems are free to create and use their
own.  Ordering requirements are and should be expressed explicitly.

Drop the call to prepare for deprecation and removal of
flush_scheduled_work().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
If no one objects, I'll route this through wq tree.

Thank you.

 init/main.c |    3 ---
 1 file changed, 3 deletions(-)

Index: work/init/main.c
===================================================================
--- work.orig/init/main.c
+++ work/init/main.c
@@ -778,9 +778,6 @@ static void __init do_initcalls(void)

 	for (fn = __early_initcall_end; fn < __initcall_end; fn++)
 		do_one_initcall(*fn);
-
-	/* Make sure there is no pending stuff from the initcall sequence */
-	flush_scheduled_work();
 }

 /*

Re: [PATCH v2.6.36-rc7] init: don't call flush_scheduled_work() from do_initcalls()

[Tejun Heo]

On 10/15/2010 05:13 PM, Tejun Heo wrote:
> It's unclear what flush_scheduled_work() in do_initcalls() tries to
> achieve.  The call doesn't make much sense - there already are
> multiple system workqueues which aren't affected by
> flush_scheduled_wokr() and subsystems are free to create and use their
> own.  Ordering requirements are and should be expressed explicitly.
> 
> Drop the call to prepare for deprecation and removal of
> flush_scheduled_work().
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
> If no one objects, I'll route this through wq tree.

Applying to wq#for-next.  Please scream if something is wrong.

Thanks.

-- 
tejun

Re: [PATCH v2.6.36-rc7] init: don't call flush_scheduled_work() from do_initcalls()

[Andrew Morton]

On Fri, 15 Oct 2010 17:13:24 +0200
Tejun Heo <tj@kernel.org> wrote:

> It's unclear what flush_scheduled_work() in do_initcalls() tries to
> achieve.  The call doesn't make much sense - there already are
> multiple system workqueues which aren't affected by
> flush_scheduled_wokr() and subsystems are free to create and use their
> own.  Ordering requirements are and should be expressed explicitly.
> 
> Drop the call to prepare for deprecation and removal of
> flush_scheduled_work().
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
> If no one objects, I'll route this through wq tree.
> 
> Thank you.
> 
>  init/main.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> Index: work/init/main.c
> ===================================================================
> --- work.orig/init/main.c
> +++ work/init/main.c
> @@ -778,9 +778,6 @@ static void __init do_initcalls(void)
> 
>  	for (fn = __early_initcall_end; fn < __initcall_end; fn++)
>  		do_one_initcall(*fn);
> -
> -	/* Make sure there is no pending stuff from the initcall sequence */
> -	flush_scheduled_work();
>  }
> 

hm, that predates the initial 2002 BK tree.

If some initcall function leaves a work scheduled and doesn't flush it
then free_initmem() can come along and pull the rug out from under its
feet.  Then you will own both pieces :)

If you really don't like people sending you angry emails then I suppose
you could add some warning here if a scheduled work is pending, and
that the scheduled work's callback existed in init.text memory.  Which
would be a bit of a pain to implement.

Oh well.  The oops traces will make it fairly clear what happened.

Re: [PATCH v2.6.36-rc7] init: don't call flush_scheduled_work() from do_initcalls()

[Tejun Heo]

Hello,

On 10/22/2010 02:12 AM, Andrew Morton wrote:
>> Index: work/init/main.c
>> ===================================================================
>> --- work.orig/init/main.c
>> +++ work/init/main.c
>> @@ -778,9 +778,6 @@ static void __init do_initcalls(void)
>>
>>  	for (fn = __early_initcall_end; fn < __initcall_end; fn++)
>>  		do_one_initcall(*fn);
>> -
>> -	/* Make sure there is no pending stuff from the initcall sequence */
>> -	flush_scheduled_work();
>>  }
>>
> 
> hm, that predates the initial 2002 BK tree.
> 
> If some initcall function leaves a work scheduled and doesn't flush it
> then free_initmem() can come along and pull the rug out from under its
> feet.  Then you will own both pieces :)

Ah, okay, so that's the rationale behind it.  That at least explains
why it made sense back then.  :-)

I don't think it makes much sense anymore tho for the following
reasons.

* As I wrote in the patch description, there are many other workqueues
  and, after auditing many workqueue users in kernel, my impression is
  that switching to different or dedicated workqueues is done quite
  casually and I've never seen any caller which notes explicit
  dependency on the auto flush behvaior.

* I don't think using workqueue that way is common and we already have
  a specialized mechanism for parallel probing - async - which has
  well defined flushing/ordering behavior during booting.

* System workqueue now can happily host long running works.  We can
  end up sitting there for undetermined amount of time.

> If you really don't like people sending you angry emails then I suppose
> you could add some warning here if a scheduled work is pending, and
> that the scheduled work's callback existed in init.text memory.  Which
> would be a bit of a pain to implement.
> 
> Oh well.  The oops traces will make it fairly clear what happened.

I haven't pushed the patch to Linus yet.  I'll remove it for now and
try to implement something which at least checks the text section of
pending and running works.

Thanks.

-- 
tejun

Re: [PATCH v2.6.36-rc7] init: don't call flush_scheduled_work() from do_initcalls()

[Andrew Morton]

On Fri, 22 Oct 2010 10:27:06 +0200 Tejun Heo <tj@kernel.org> wrote:

> > If you really don't like people sending you angry emails then I suppose
> > you could add some warning here if a scheduled work is pending, and
> > that the scheduled work's callback existed in init.text memory.  Which
> > would be a bit of a pain to implement.
> > 
> > Oh well.  The oops traces will make it fairly clear what happened.
> 
> I haven't pushed the patch to Linus yet.  I'll remove it for now and
> try to implement something which at least checks the text section of
> pending and running works.

mm..  I think we'd be OK to merge it.  Any such code is pretty badly
buggy and is probably also crashable with a well-timed rmmod.

It'll also be code which few people ever use, so any runtime checks
won't get us very good coverage.

Still, if it's not too hard to implement an "are there any scheduled
works which live in initmem" check then I guess that would be the
prudent approach.  A quite gross way of implementing that might be
something like


 init/main.c        |    3 +++
 kernel/workqueue.c |   11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff -puN init/main.c~a init/main.c
--- a/init/main.c~a
+++ a/init/main.c
@@ -125,6 +125,7 @@ static char *ramdisk_execute_command;
 unsigned int setup_max_cpus = NR_CPUS;
 EXPORT_SYMBOL(setup_max_cpus);
 
+int scheduled_work_hack;
 
 /*
  * Setup routine for controlling SMP activation
@@ -780,7 +781,9 @@ static void __init do_initcalls(void)
 		do_one_initcall(*fn);
 
 	/* Make sure there is no pending stuff from the initcall sequence */
+	scheduled_work_hack++;
 	flush_scheduled_work();
+	scheduled_work_hack++;
 }
 
 /*
diff -puN kernel/workqueue.c~a kernel/workqueue.c
--- a/kernel/workqueue.c~a
+++ a/kernel/workqueue.c
@@ -41,6 +41,7 @@
 #include <linux/debug_locks.h>
 #include <linux/lockdep.h>
 #include <linux/idr.h>
+#include <asm/sections.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/workqueue.h>
@@ -1819,7 +1820,15 @@ __acquires(&gcwq->lock)
 	lock_map_acquire(&cwq->wq->lockdep_map);
 	lock_map_acquire(&lockdep_map);
 	trace_workqueue_execute_start(work);
-	f(work);
+	{
+		extern int scheduled_work_hack;
+
+		if (scheduled_work_hack &&
+		    (f >= (void *)__init_begin && f < (void *)__init_end))
+			eek();
+		else
+			f(work);
+	}
 	/*
 	 * While we must be careful to not use "work" after this, the trace
 	 * point will only record its address.
_

Re: [PATCH v2.6.36-rc7] init: don't call flush_scheduled_work() from do_initcalls()

[Tejun Heo]

Hello, Andrew.

On 10/22/2010 08:09 PM, Andrew Morton wrote:
> mm..  I think we'd be OK to merge it.  Any such code is pretty badly
> buggy and is probably also crashable with a well-timed rmmod.
> 
> It'll also be code which few people ever use, so any runtime checks
> won't get us very good coverage.
> 
> Still, if it's not too hard to implement an "are there any scheduled
> works which live in initmem" check then I guess that would be the
> prudent approach.  A quite gross way of implementing that might be
> something like

I've been trying to implement proper check code but there is a
problem.  It's possible to check all pending works to see whether the
work struct itself or the work function is in initmem and warn about
them.

The problem is with currently running works.  As work_struct isn't
accessible once it starts executing, struct worker would need to cache
it for later reference.  Worker already remembers the work_struct
pointer itself and its cwq and adding one more field to remember the
currently running work function is easy.  However, it's only useful
during the unlikely buggy case during init.  Given that if anything is
still depending on initmem, it will blow up pretty reliably, I don't
think it's worthwhile to add additional tracking just for this.  So, I
think I'll just go ahead and drop the flush call and deal with the
unlikely fallouts if there's any.

Thank you.

-- 
tejun

Re: [PATCH v2.6.36-rc7] init: don't call flush_scheduled_work() from do_initcalls()

[Andrew Morton]

On Wed, 03 Nov 2010 11:59:21 +0100 Tejun Heo <tj@kernel.org> wrote:

> Hello, Andrew.
> 
> On 10/22/2010 08:09 PM, Andrew Morton wrote:
> > mm..  I think we'd be OK to merge it.  Any such code is pretty badly
> > buggy and is probably also crashable with a well-timed rmmod.
> > 
> > It'll also be code which few people ever use, so any runtime checks
> > won't get us very good coverage.
> > 
> > Still, if it's not too hard to implement an "are there any scheduled
> > works which live in initmem" check then I guess that would be the
> > prudent approach.  A quite gross way of implementing that might be
> > something like
> 
> I've been trying to implement proper check code but there is a
> problem.  It's possible to check all pending works to see whether the
> work struct itself or the work function is in initmem and warn about
> them.
> 
> The problem is with currently running works.  As work_struct isn't
> accessible once it starts executing, struct worker would need to cache
> it for later reference.  Worker already remembers the work_struct
> pointer itself and its cwq and adding one more field to remember the
> currently running work function is easy.  However, it's only useful
> during the unlikely buggy case during init.  Given that if anything is
> still depending on initmem, it will blow up pretty reliably, I don't
> think it's worthwhile to add additional tracking just for this.

yes, if it goes bang, we'll hear it.  And we'll be able to work out why
it went bang pretty easily.  Although I assume that a backtrace which
leads into a just-unloaded module won't be able to display that
module's symbols.

>  So, I
> think I'll just go ahead and drop the flush call and deal with the
> unlikely fallouts if there's any.

OK by me.