[PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[Tejun Heo]

lru_add_drain_all() uses schedule_on_each_cpu() which is synchronous.
There is no reason to call flush_scheduled_work() after
lru_add_drain_all().  Drop the spurious calls.

This is to prepare for the deprecation and removal of
flush_scheduled_work().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 mm/memory_hotplug.c |    2 --
 1 file changed, 2 deletions(-)

Index: work/mm/memory_hotplug.c
===================================================================
--- work.orig/mm/memory_hotplug.c
+++ work/mm/memory_hotplug.c
@@ -840,7 +840,6 @@ repeat:
 	ret = 0;
 	if (drain) {
 		lru_add_drain_all();
-		flush_scheduled_work();
 		cond_resched();
 		drain_all_pages();
 	}
@@ -862,7 +861,6 @@ repeat:
 	}
 	/* drain all zone's lru pagevec, this is asyncronous... */
 	lru_add_drain_all();
-	flush_scheduled_work();
 	yield();
 	/* drain pcp pages , this is synchrouns. */
 	drain_all_pages();

Re: [PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[KAMEZAWA Hiroyuki]

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

> lru_add_drain_all() uses schedule_on_each_cpu() which is synchronous.
> There is no reason to call flush_scheduled_work() after
> lru_add_drain_all().  Drop the spurious calls.
> 
> This is to prepare for the deprecation and removal of
> flush_scheduled_work().
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Re: [PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[Tejun Heo]

Hello,

On 10/18/2010 02:01 AM, KAMEZAWA Hiroyuki wrote:
> On Fri, 15 Oct 2010 17:25:15 +0200
> Tejun Heo <tj@kernel.org> wrote:
> 
>> lru_add_drain_all() uses schedule_on_each_cpu() which is synchronous.
>> There is no reason to call flush_scheduled_work() after
>> lru_add_drain_all().  Drop the spurious calls.
>>
>> This is to prepare for the deprecation and removal of
>> flush_scheduled_work().
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Through which tree should this be routed?  Shall I route this via the
wq tree?

Thank you.

-- 
tejun

Re: [PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[Minchan Kim]

On Sat, Oct 16, 2010 at 12:25 AM, Tejun Heo <tj@kernel.org> wrote:
> lru_add_drain_all() uses schedule_on_each_cpu() which is synchronous.
> There is no reason to call flush_scheduled_work() after
> lru_add_drain_all().  Drop the spurious calls.
>
> This is to prepare for the deprecation and removal of
> flush_scheduled_work().
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>


-- 
Kind regards,
Minchan Kim

Re: [PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[Mel Gorman]

On Fri, Oct 15, 2010 at 05:25:15PM +0200, Tejun Heo wrote:
> lru_add_drain_all() uses schedule_on_each_cpu() which is synchronous.
> There is no reason to call flush_scheduled_work() after
> lru_add_drain_all().  Drop the spurious calls.
> 

This looks correct.

Acked-by: Mel Gorman <mel@csn.ul.ie>

I suspect this call to flush_scheduled_work() existed because it is easy
to interpret the documentation of schedule_on_each_cpu to mean it is an
asynchronous call.  Maybe something like the following?

==== CUT HERE ====
workqueue: Clarify that schedule_on_each_cpu is synchronous

The documentation for schedule_on_each_cpu() states that it calls a function
on each online CPU from keventd. This can easily be interpreted as an
asyncronous call because the description does not mention that flush_work
is called. Clarify that it is synchronous.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
---
 kernel/workqueue.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f77afd9..07cba1e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2592,13 +2592,15 @@ int schedule_delayed_work_on(int cpu,
 EXPORT_SYMBOL(schedule_delayed_work_on);
 
 /**
- * schedule_on_each_cpu - call a function on each online CPU from keventd
+ * schedule_on_each_cpu - Call a function on each online CPU via keventd and wait for completion
  * @func: the function to call
  *
+ * schedule_on_each_cpu executes a given callback function on each CPU via keventd
+ * and blocks until each callback has completed. schedule_on_each_cpu() is very
+ * slow.
+ *
  * Returns zero on success.
  * Returns -ve errno on failure.
- *
- * schedule_on_each_cpu() is very slow.
  */
 int schedule_on_each_cpu(work_func_t func)
 {

Re: [PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[KAMEZAWA Hiroyuki]

On Mon, 18 Oct 2010 07:03:37 +0200
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> On 10/18/2010 02:01 AM, KAMEZAWA Hiroyuki wrote:
> > On Fri, 15 Oct 2010 17:25:15 +0200
> > Tejun Heo <tj@kernel.org> wrote:
> > 
> >> lru_add_drain_all() uses schedule_on_each_cpu() which is synchronous.
> >> There is no reason to call flush_scheduled_work() after
> >> lru_add_drain_all().  Drop the spurious calls.
> >>
> >> This is to prepare for the deprecation and removal of
> >> flush_scheduled_work().
> >>
> >> Signed-off-by: Tejun Heo <tj@kernel.org>
> > Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Through which tree should this be routed?  Shall I route this via the
> wq tree?
> 
please.

Thank,
-Kame

Re: [PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[KOSAKI Motohiro]

> On Fri, Oct 15, 2010 at 05:25:15PM +0200, Tejun Heo wrote:
> > lru_add_drain_all() uses schedule_on_each_cpu() which is synchronous.
> > There is no reason to call flush_scheduled_work() after
> > lru_add_drain_all().  Drop the spurious calls.
> > 
> 
> This looks correct.
> 
> Acked-by: Mel Gorman <mel@csn.ul.ie>
> 
> I suspect this call to flush_scheduled_work() existed because it is easy
> to interpret the documentation of schedule_on_each_cpu to mean it is an
> asynchronous call.  Maybe something like the following?
> 
> ==== CUT HERE ====
> workqueue: Clarify that schedule_on_each_cpu is synchronous
> 
> The documentation for schedule_on_each_cpu() states that it calls a function
> on each online CPU from keventd. This can easily be interpreted as an
> asyncronous call because the description does not mention that flush_work
> is called. Clarify that it is synchronous.
> 
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> ---
>  kernel/workqueue.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index f77afd9..07cba1e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2592,13 +2592,15 @@ int schedule_delayed_work_on(int cpu,
>  EXPORT_SYMBOL(schedule_delayed_work_on);
>  
>  /**
> - * schedule_on_each_cpu - call a function on each online CPU from keventd
> + * schedule_on_each_cpu - Call a function on each online CPU via keventd and wait for completion
>   * @func: the function to call
>   *
> + * schedule_on_each_cpu executes a given callback function on each CPU via keventd
> + * and blocks until each callback has completed. schedule_on_each_cpu() is very
> + * slow.
> + *
>   * Returns zero on success.
>   * Returns -ve errno on failure.
> - *
> - * schedule_on_each_cpu() is very slow.

Nice!
	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

[PATCH wq#for-next] workqueue: Clarify that schedule_on_each_cpu is synchronous

[Tejun Heo]

The documentation for schedule_on_each_cpu() states that it calls a
function on each online CPU from keventd.  This can easily be
interpreted as an asyncronous call because the description does not
mention that flush_work is called.  Clarify that it is synchronous.

tj: rephrased a bit

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
Commited somewhat rephrased version.  Thank you.

 kernel/workqueue.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2c6871c..eb5c197 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2676,13 +2676,15 @@ int schedule_delayed_work_on(int cpu,
 EXPORT_SYMBOL(schedule_delayed_work_on);

 /**
- * schedule_on_each_cpu - call a function on each online CPU from keventd
+ * schedule_on_each_cpu - execute a function synchronously on each online CPU
  * @func: the function to call
  *
- * Returns zero on success.
- * Returns -ve errno on failure.
- *
+ * schedule_on_each_cpu() executes @func on each online CPU using the
+ * system workqueue and blocks until all CPUs have completed.
  * schedule_on_each_cpu() is very slow.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
  */
 int schedule_on_each_cpu(work_func_t func)
 {
-- 
1.7.1

Re: [PATCH v2.6.36-rc7] memory_hotplug: drop spurious calls to flush_scheduled_work()

[Tejun Heo]

On 10/19/2010 01:58 AM, KAMEZAWA Hiroyuki wrote:
>> Through which tree should this be routed?  Shall I route this via the
>> wq tree?
>>
> please.

Applied to wq#for-next.  Thank you.

-- 
tejun

Re: [PATCH wq#for-next] workqueue: Clarify that schedule_on_each_cpu is synchronous

[KAMEZAWA Hiroyuki]

On Tue, 19 Oct 2010 11:18:46 +0200
Tejun Heo <tj@kernel.org> wrote:

> The documentation for schedule_on_each_cpu() states that it calls a
> function on each online CPU from keventd.  This can easily be
> interpreted as an asyncronous call because the description does not
> mention that flush_work is called.  Clarify that it is synchronous.
> 
> tj: rephrased a bit
> 
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Re: [PATCH wq#for-next] workqueue: Clarify that schedule_on_each_cpu is synchronous

[Wu Fengguang]

On Tue, Oct 19, 2010 at 05:18:46PM +0800, Tejun Heo wrote:
> The documentation for schedule_on_each_cpu() states that it calls a
> function on each online CPU from keventd.  This can easily be
> interpreted as an asyncronous call because the description does not
> mention that flush_work is called.  Clarify that it is synchronous.
> 
> tj: rephrased a bit
> 
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>

Nice comment!

Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>