Re: [RFC] Adding queue_delayed_work_on interface for workqueues

[Andrew Morton <akpm@osdl.org>]

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.