[PATCH] genirq: don't use flush_scheduled_work() in IRQ affinity notifiers

[PATCH] genirq: don't use flush_scheduled_work() in IRQ affinity notifiers

[Tejun Heo]

cd7eab44e9 (genirq: Add IRQ affinity notifiers) added use of
flush_scheduled_work() which is being deprecated.  Add a dedicated
workqueue and flush it instead of flushing system-wide workqueue.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
I'm planning on marking flush_scheduled_work() deprecated in
linux-next soonish.  It would be great if this patch (or something
else which removes flush_scheduled_work() call somehow) can be
included in linux-next.

Thanks.

 include/linux/interrupt.h |    6 +-----
 kernel/irq/manage.c       |   17 ++++++++++++++++-
 2 files changed, 17 insertions(+), 6 deletions(-)

Index: work/include/linux/interrupt.h
===================================================================
--- work.orig/include/linux/interrupt.h
+++ work/include/linux/interrupt.h
@@ -256,11 +256,7 @@ struct irq_affinity_notify {
 
 extern int
 irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
-
-static inline void irq_run_affinity_notifiers(void)
-{
-	flush_scheduled_work();
-}
+extern void irq_run_affinity_notifiers(void);
 
 #else /* CONFIG_SMP */
 
Index: work/kernel/irq/manage.c
===================================================================
--- work.orig/kernel/irq/manage.c
+++ work/kernel/irq/manage.c
@@ -74,6 +74,16 @@ EXPORT_SYMBOL(synchronize_irq);
 
 #ifdef CONFIG_SMP
 cpumask_var_t irq_default_affinity;
+static struct workqueue_struct *irq_affinity_notify_wq;
+
+static int __init irq_affinity_init(void)
+{
+	irq_affinity_notify_wq = alloc_workqueue("irq_affinity_notify", 0, 0);
+	if (!irq_affinity_notify_wq)
+		return -ENOMEM;
+	return 0;
+}
+subsys_initcall(irq_affinity_init);
 
 /**
  *	irq_can_set_affinity - Check if the affinity of a given irq can be set
@@ -164,7 +174,7 @@ int __irq_set_affinity_locked(struct irq
 
 	if (desc->affinity_notify) {
 		kref_get(&desc->affinity_notify->kref);
-		schedule_work(&desc->affinity_notify->work);
+		queue_work(irq_affinity_notify_wq, &desc->affinity_notify->work);
 	}
 	irqd_set(data, IRQD_AFFINITY_SET);
 
@@ -273,6 +283,11 @@ irq_set_affinity_notifier(unsigned int i
 }
 EXPORT_SYMBOL_GPL(irq_set_affinity_notifier);
 
+void irq_run_affinity_notifiers(void)
+{
+	flush_workqueue(irq_affinity_notify_wq);
+}
+
 #ifndef CONFIG_AUTO_IRQ_AFFINITY
 /*
  * Generic version of the affinity autoselector.

Re: [PATCH] genirq: don't use flush_scheduled_work() in IRQ affinity notifiers

[Tejun Heo]

On Wed, Jun 15, 2011 at 04:29:17PM +0200, Tejun Heo wrote:
> cd7eab44e9 (genirq: Add IRQ affinity notifiers) added use of
> flush_scheduled_work() which is being deprecated.  Add a dedicated
> workqueue and flush it instead of flushing system-wide workqueue.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
> ---
> I'm planning on marking flush_scheduled_work() deprecated in
> linux-next soonish.  It would be great if this patch (or something
> else which removes flush_scheduled_work() call somehow) can be
> included in linux-next.

Ooh, right, only compile & boot tested.  Don't have anything which
makes use of affinity notifier.

Thanks.

-- 
tejun

Re: [PATCH] genirq: don't use flush_scheduled_work() in IRQ affinity notifiers

[Thomas Gleixner]

On Wed, 15 Jun 2011, Tejun Heo wrote:

> On Wed, Jun 15, 2011 at 04:29:17PM +0200, Tejun Heo wrote:
> > cd7eab44e9 (genirq: Add IRQ affinity notifiers) added use of
> > flush_scheduled_work() which is being deprecated.  Add a dedicated
> > workqueue and flush it instead of flushing system-wide workqueue.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ben Hutchings <bhutchings@solarflare.com>
> > ---
> > I'm planning on marking flush_scheduled_work() deprecated in
> > linux-next soonish.  It would be great if this patch (or something
> > else which removes flush_scheduled_work() call somehow) can be
> > included in linux-next.
> 
> Ooh, right, only compile & boot tested.  Don't have anything which
> makes use of affinity notifier.

Ben?

Re: [PATCH] genirq: don't use flush_scheduled_work() in IRQ affinity notifiers

[Ben Hutchings]

On Wed, 2011-06-15 at 16:29 +0200, Tejun Heo wrote:
> cd7eab44e9 (genirq: Add IRQ affinity notifiers) added use of
> flush_scheduled_work() which is being deprecated.  Add a dedicated
> workqueue and flush it instead of flushing system-wide workqueue.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
> ---
> I'm planning on marking flush_scheduled_work() deprecated in
> linux-next soonish.  It would be great if this patch (or something
> else which removes flush_scheduled_work() call somehow) can be
> included in linux-next.
[...]
> --- work.orig/kernel/irq/manage.c
> +++ work/kernel/irq/manage.c
> @@ -74,6 +74,16 @@ EXPORT_SYMBOL(synchronize_irq);
>  
>  #ifdef CONFIG_SMP
>  cpumask_var_t irq_default_affinity;
> +static struct workqueue_struct *irq_affinity_notify_wq;
> +
> +static int __init irq_affinity_init(void)
> +{
> +	irq_affinity_notify_wq = alloc_workqueue("irq_affinity_notify", 0, 0);
> +	if (!irq_affinity_notify_wq)
> +		return -ENOMEM;
> +	return 0;
> +}
> +subsys_initcall(irq_affinity_init);
[...]

This facility is enabled on all configurations with NET && SMP &&
GENERIC_HARDIRQS, but at the moment is only useful for some net drivers
(currently only one).  So I don't think it should be creating a task at
boot time.  Does alloc_workqueue() still create any tasks immediately?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] genirq: don't use flush_scheduled_work() in IRQ affinity notifiers

[Tejun Heo]

Hello,

On Fri, Jun 17, 2011 at 12:28:06AM +0100, Ben Hutchings wrote:
> This facility is enabled on all configurations with NET && SMP &&
> GENERIC_HARDIRQS, but at the moment is only useful for some net drivers
> (currently only one).  So I don't think it should be creating a task at
> boot time.  Does alloc_workqueue() still create any tasks immediately?

Nope, no need to worry about it.  The only added overhead is the
memory occupied by workqueue itself (which includes small percpu area
but one systemwide one isn't gonna hurt anyone and we've been
decreasing the number of workqueues significantly).  It just serves as
a flush domain.

-- 
tejun

Re: [PATCH] genirq: don't use flush_scheduled_work() in IRQ affinity notifiers

[Ben Hutchings]

On Fri, 2011-06-17 at 11:45 +0200, Tejun Heo wrote:
> Hello,
> 
> On Fri, Jun 17, 2011 at 12:28:06AM +0100, Ben Hutchings wrote:
> > This facility is enabled on all configurations with NET && SMP &&
> > GENERIC_HARDIRQS, but at the moment is only useful for some net drivers
> > (currently only one).  So I don't think it should be creating a task at
> > boot time.  Does alloc_workqueue() still create any tasks immediately?
> 
> Nope, no need to worry about it.  The only added overhead is the
> memory occupied by workqueue itself (which includes small percpu area
> but one systemwide one isn't gonna hurt anyone and we've been
> decreasing the number of workqueues significantly).  It just serves as
> a flush domain.

Sorry I didn't reply to this earlier.

Given what you've said, I see no problem with this change.  So you can
add:

Acked-by: Ben Hutchings <bhutchings@solarflare.com>

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] genirq: don't use flush_scheduled_work() in IRQ affinity notifiers

[Tejun Heo]

On Thu, Jul 14, 2011 at 05:49:20PM +0100, Ben Hutchings wrote:
> On Fri, 2011-06-17 at 11:45 +0200, Tejun Heo wrote:
> > Hello,
> > 
> > On Fri, Jun 17, 2011 at 12:28:06AM +0100, Ben Hutchings wrote:
> > > This facility is enabled on all configurations with NET && SMP &&
> > > GENERIC_HARDIRQS, but at the moment is only useful for some net drivers
> > > (currently only one).  So I don't think it should be creating a task at
> > > boot time.  Does alloc_workqueue() still create any tasks immediately?
> > 
> > Nope, no need to worry about it.  The only added overhead is the
> > memory occupied by workqueue itself (which includes small percpu area
> > but one systemwide one isn't gonna hurt anyone and we've been
> > decreasing the number of workqueues significantly).  It just serves as
> > a flush domain.
> 
> Sorry I didn't reply to this earlier.
> 
> Given what you've said, I see no problem with this change.  So you can
> add:
> 
> Acked-by: Ben Hutchings <bhutchings@solarflare.com>

Thomas, I suppose this should be routed through tip:irq/core?  Or
shall I push it through wq tree?

Thank you.

-- 
tejun