[RFC] Adding queue_delayed_work_on interface for workqueues

[RFC] Adding queue_delayed_work_on interface for workqueues

[Venkatesh Pallipadi]

This patch is a part of cpufreq patches for ondemand governor optimizations
and entire series is actually posted to cpufreq mailing list.
Subject "minor optimizations to ondemand governor"

The following patch however is a generic change to workqueue interface and 
I wanted to get comments on this on lkml.

Thanks,
Venki

Add queue_delayed_work_on() interface for workqueues.

Signed-off-by: Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

Index: linux-2.6.17/include/linux/workqueue.h
===================================================================
--- linux-2.6.17.orig/include/linux/workqueue.h
+++ linux-2.6.17/include/linux/workqueue.h
@@ -63,12 +63,13 @@ extern void destroy_workqueue(struct wor
 
 extern int FASTCALL(queue_work(struct workqueue_struct *wq, struct work_struct *work));
 extern int FASTCALL(queue_delayed_work(struct workqueue_struct *wq, struct work_struct *work, unsigned long delay));
+extern int FASTCALL(queue_delayed_work_on(int cpu, struct workqueue_struct *wq, struct work_struct *work, unsigned long delay));
 extern void FASTCALL(flush_workqueue(struct workqueue_struct *wq));
 
 extern int FASTCALL(schedule_work(struct work_struct *work));
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 
-extern int schedule_delayed_work_on(int cpu, struct work_struct *work, unsigned long delay);
+extern int FASTCALL(schedule_delayed_work_on(int cpu, struct work_struct *work, unsigned long delay));
 extern int schedule_on_each_cpu(void (*func)(void *info), void *info);
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
Index: linux-2.6.17/kernel/workqueue.c
===================================================================
--- linux-2.6.17.orig/kernel/workqueue.c
+++ linux-2.6.17/kernel/workqueue.c
@@ -148,6 +148,27 @@ int fastcall queue_delayed_work(struct w
 	return ret;
 }
 
+int fastcall queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
+			struct work_struct *work, unsigned long delay)
+{
+	int ret = 0;
+	struct timer_list *timer = &work->timer;
+
+	if (!test_and_set_bit(0, &work->pending)) {
+		BUG_ON(timer_pending(timer));
+		BUG_ON(!list_empty(&work->entry));
+
+		/* This stores wq for the moment, for the timer_fn */
+		work->wq_data = wq;
+		timer->expires = jiffies + delay;
+		timer->data = (unsigned long)work;
+		timer->function = delayed_work_timer_fn;
+		add_timer_on(timer, cpu);
+		ret = 1;
+	}
+	return ret;
+}
+
 static void run_workqueue(struct cpu_workqueue_struct *cwq)
 {
 	unsigned long flags;
@@ -408,24 +429,10 @@ int fastcall schedule_delayed_work(struc
 	return queue_delayed_work(keventd_wq, work, delay);
 }
 
-int schedule_delayed_work_on(int cpu,
+int fastcall schedule_delayed_work_on(int cpu,
 			struct work_struct *work, unsigned long delay)
 {
-	int ret = 0;
-	struct timer_list *timer = &work->timer;
-
-	if (!test_and_set_bit(0, &work->pending)) {
-		BUG_ON(timer_pending(timer));
-		BUG_ON(!list_empty(&work->entry));
-		/* This stores keventd_wq for the moment, for the timer_fn */
-		work->wq_data = keventd_wq;
-		timer->expires = jiffies + delay;
-		timer->data = (unsigned long)work;
-		timer->function = delayed_work_timer_fn;
-		add_timer_on(timer, cpu);
-		ret = 1;
-	}
-	return ret;
+	return queue_delayed_work_on(cpu, keventd_wq, work, delay);
 }
 
 int schedule_on_each_cpu(void (*func) (void *info), void *info)
@@ -608,6 +615,7 @@ void init_workqueues(void)
 EXPORT_SYMBOL_GPL(__create_workqueue);
 EXPORT_SYMBOL_GPL(queue_work);
 EXPORT_SYMBOL_GPL(queue_delayed_work);
+EXPORT_SYMBOL_GPL(queue_delayed_work_on);
 EXPORT_SYMBOL_GPL(flush_workqueue);
 EXPORT_SYMBOL_GPL(destroy_workqueue);
 

Re: [RFC] Adding queue_delayed_work_on interface for workqueues

[Andrew Morton]

Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> wrote:
>
> 
> This patch is a part of cpufreq patches for ondemand governor optimizations
> and entire series is actually posted to cpufreq mailing list.
> Subject "minor optimizations to ondemand governor"
> 
> The following patch however is a generic change to workqueue interface and 
> I wanted to get comments on this on lkml.
> 
> ...
>
> Add queue_delayed_work_on() interface for workqueues.

It looks sensible to me.

> +extern int FASTCALL(queue_delayed_work_on(int cpu, struct workqueue_struct *wq, struct work_struct *work, unsigned long delay));

Please wrap at 80-cols.

I wouldn't bother making this FASTCALL.  It's an ugly thing, and why this
particular function?  And this isn't fastpath stuff.

>  
> -extern int schedule_delayed_work_on(int cpu, struct work_struct *work, unsigned long delay);
> +extern int FASTCALL(schedule_delayed_work_on(int cpu, struct work_struct *work, unsigned long delay));

Ditto.

>  }
>  
> +int fastcall queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
> +			struct work_struct *work, unsigned long delay)
> +{
> +	int ret = 0;
> +	struct timer_list *timer = &work->timer;
> +
> +	if (!test_and_set_bit(0, &work->pending)) {
> +		BUG_ON(timer_pending(timer));
> +		BUG_ON(!list_empty(&work->entry));
> +
> +		/* This stores wq for the moment, for the timer_fn */
> +		work->wq_data = wq;
> +		timer->expires = jiffies + delay;
> +		timer->data = (unsigned long)work;
> +		timer->function = delayed_work_timer_fn;
> +		add_timer_on(timer, cpu);
> +		ret = 1;
> +	}
> +	return ret;
> +}

Feel free to add some kernel-doc for this function ;)

> @@ -608,6 +615,7 @@ void init_workqueues(void)
>  EXPORT_SYMBOL_GPL(__create_workqueue);
>  EXPORT_SYMBOL_GPL(queue_work);
>  EXPORT_SYMBOL_GPL(queue_delayed_work);
> +EXPORT_SYMBOL_GPL(queue_delayed_work_on);
>  EXPORT_SYMBOL_GPL(flush_workqueue);
>  EXPORT_SYMBOL_GPL(destroy_workqueue);

Opinions vary a bit, but I think we mostly prefer to put the
EXPORT_SYMBOL()s at the site of the thing which is being exported:

foo()
{
}
EXPORT_SYMBOL(foo);

because it keeps all the info in the same place.  (We don't declare
functions to be static or to return char* or to be __init at a different
place in the source file..).

Then again, it's legit to follow existing local style too.  Someone will
come along later and fix it all in a single hit.  Whatever.

Re: [RFC] Adding queue_delayed_work_on interface for workqueues

[Dave Jones]

On Wed, Jun 28, 2006 at 02:32:42PM -0700, Andrew Morton wrote:

 > > This patch is a part of cpufreq patches for ondemand governor optimizations
 > > and entire series is actually posted to cpufreq mailing list.
 > > Subject "minor optimizations to ondemand governor"
 > > 
 > > The following patch however is a generic change to workqueue interface and 
 > > I wanted to get comments on this on lkml.
 > > 
 > > ...
 > >
 > > Add queue_delayed_work_on() interface for workqueues.
 > 
 > It looks sensible to me.

*nod*, same here.

As (for now) cpufreq is the only user of this, any objection to
me marshalling this through the cpufreq tree (+ the cleanups you
suggested of course) ?

		Dave

-- 
http://www.codemonkey.org.uk

Re: [RFC] Adding queue_delayed_work_on interface for workqueues

[Andrew Morton]

Dave Jones <davej@redhat.com> wrote:
>
> On Wed, Jun 28, 2006 at 02:32:42PM -0700, Andrew Morton wrote:
> 
>  > > This patch is a part of cpufreq patches for ondemand governor optimizations
>  > > and entire series is actually posted to cpufreq mailing list.
>  > > Subject "minor optimizations to ondemand governor"
>  > > 
>  > > The following patch however is a generic change to workqueue interface and 
>  > > I wanted to get comments on this on lkml.
>  > > 
>  > > ...
>  > >
>  > > Add queue_delayed_work_on() interface for workqueues.
>  > 
>  > It looks sensible to me.
> 
> *nod*, same here.
> 
> As (for now) cpufreq is the only user of this, any objection to
> me marshalling this through the cpufreq tree (+ the cleanups you
> suggested of course) ?
> 

Is OK by me.

It's good to point out the presence of out-of-scope things like this in the
eventual Linus pull request.

Re: [RFC] Adding queue_delayed_work_on interface for workqueues

[Dave Jones]

On Wed, Jun 28, 2006 at 02:32:42PM -0700, Andrew Morton wrote:

 > > +extern int FASTCALL(queue_delayed_work_on(int cpu, struct workqueue_struct *wq, struct work_struct *work, unsigned long delay));
 > 
 > Please wrap at 80-cols.

fixed up

 > I wouldn't bother making this FASTCALL.  It's an ugly thing, and why this
 > particular function?  And this isn't fastpath stuff.

fixed up

 > > -extern int schedule_delayed_work_on(int cpu, struct work_struct *work, unsigned long delay);
 > > +extern int FASTCALL(schedule_delayed_work_on(int cpu, struct work_struct *work, unsigned long delay));
 > 
 > Ditto.

ditto :)

 > >  }
 > >  
 > > +int fastcall queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
 > > +			struct work_struct *work, unsigned long delay)
 > > +{
 > > +}
 > 
 > Feel free to add some kernel-doc for this function ;)

Venki, I'm lazy and it's past my bedtime, send a follow-up diff please ?

 > > @@ -608,6 +615,7 @@ void init_workqueues(void)
 > >  EXPORT_SYMBOL_GPL(__create_workqueue);
 > >  EXPORT_SYMBOL_GPL(queue_work);
 > >  EXPORT_SYMBOL_GPL(queue_delayed_work);
 > > +EXPORT_SYMBOL_GPL(queue_delayed_work_on);
 > >  EXPORT_SYMBOL_GPL(flush_workqueue);
 > >  EXPORT_SYMBOL_GPL(destroy_workqueue);
 > 
 > Opinions vary a bit, but I think we mostly prefer to put the
 > EXPORT_SYMBOL()s at the site of the thing which is being exported:
 >.. 
 > Then again, it's legit to follow existing local style too.  Someone will
 > come along later and fix it all in a single hit.  Whatever.

That someone was me. I did it as a follow-on for Venki's series.

All pushed out to cpufreq.git

		Dave

-- 
http://www.codemonkey.org.uk