workqueue_set_max_active(wq, 0)?

workqueue_set_max_active(wq, 0)?

[Johannes Berg]

Hi,

I know workqueue_set_max_active(wq, 0) isn't allowed, but I was just
playing with making mac80211 completely synchronous in the configuration
path. (The reason is that right now, we have a single-threaded workqueue
and configuration entry points, so it's *almost* single-threaded /
synchronous. This means that the non-synchronous nature rarely gets
tested.)

So I thought I could put a work struct onto the workqueue and then when
it executes it signals the configuration function & waits for that to
finish, etc., a bit like the rendezvous mechanism used to flush works.

But then I thought maybe this should be more generic, like
"workqueue_pause()" / "workqueue_resume()".

And then I found workqueue_set_max_active() but passing 0 isn't allowed.

Before I dive in more deeply I figured I'd ask if you think what a good
way of doing this would be (and whether I'm completely insane) :-)

johannes

Re: workqueue_set_max_active(wq, 0)?

[Tejun Heo]

Hello, Johannes. :)

On Fri, Dec 09, 2011 at 10:54:42AM +0100, Johannes Berg wrote:
> And then I found workqueue_set_max_active() but passing 0 isn't allowed.
> 
> Before I dive in more deeply I figured I'd ask if you think what a good
> way of doing this would be (and whether I'm completely insane) :-)

Hmmm... yeah, actually, that's what wq uses internally to implement
freezable workqueues.  It sets max_active to 0 temporarily and waits
for all in-flight works to finish.  On thaw, the original value is
restored.

Updating workqueue_set_max_active() to return the old value would be a
nice API update which goes together, I think.

Thanks.

-- 
tejun

Re: workqueue_set_max_active(wq, 0)?

[Johannes Berg]

Hi Tejun,

> > Before I dive in more deeply I figured I'd ask if you think what a good
> > way of doing this would be (and whether I'm completely insane) :-)
> 
> Hmmm... yeah, actually, that's what wq uses internally to implement
> freezable workqueues.  It sets max_active to 0 temporarily and waits
> for all in-flight works to finish.  On thaw, the original value is
> restored.

Right, it's a little more complicated though because it has to wait etc.

> Updating workqueue_set_max_active() to return the old value would be a
> nice API update which goes together, I think.

Yeah, that would probably be useful, in particular if one tries to
freeze a paused workqueue...

johannes

Re: workqueue_set_max_active(wq, 0)?

[Johannes Berg]

Hi Tejun,


> > Before I dive in more deeply I figured I'd ask if you think what a good
> > way of doing this would be (and whether I'm completely insane) :-)
> 
> Hmmm... yeah, actually, that's what wq uses internally to implement
> freezable workqueues.  It sets max_active to 0 temporarily and waits
> for all in-flight works to finish.  On thaw, the original value is
> restored.
> 
> Updating workqueue_set_max_active() to return the old value would be a
> nice API update which goes together, I think.

So I looked at set_max_active() but I think it can also be called in
atomic contexts but I would want this to wait I think ... Does the below
look like a reasonable first cut (never mind that it's not really
exported in header files yet, I haven't even tested it yet but the
interaction with other code is interesting)

johannes


---
 kernel/workqueue.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

--- a/kernel/workqueue.c	2011-12-10 17:32:26.000000000 +0100
+++ b/kernel/workqueue.c	2011-12-15 16:36:06.000000000 +0100
@@ -51,6 +51,7 @@ enum {
 	GCWQ_DISASSOCIATED	= 1 << 2,	/* cpu can't serve workers */
 	GCWQ_FREEZING		= 1 << 3,	/* freeze in progress */
 	GCWQ_HIGHPRI_PENDING	= 1 << 4,	/* highpri works on queue */
+	GCWQ_PAUSING		= 1 << 5,
 
 	/* worker flags */
 	WORKER_STARTED		= 1 << 0,	/* started */
@@ -246,6 +247,7 @@ struct workqueue_struct {
 #ifdef CONFIG_LOCKDEP
 	struct lockdep_map	lockdep_map;
 #endif
+	wait_queue_head_t	waitq;
 };
 
 struct workqueue_struct *system_wq __read_mostly;
@@ -1759,6 +1761,8 @@ static void cwq_dec_nr_in_flight(struct
 			if (cwq->nr_active < cwq->max_active)
 				cwq_activate_first_delayed(cwq);
 		}
+		if (cwq->nr_active == 0 && cwq->max_active == 0)
+			wake_up(&cwq->wq->waitq);
 	}
 
 	/* is flush in progress and are we at the flushing tip? */
@@ -2990,6 +2994,7 @@ struct workqueue_struct *__alloc_workque
 	atomic_set(&wq->nr_cwqs_to_flush, 0);
 	INIT_LIST_HEAD(&wq->flusher_queue);
 	INIT_LIST_HEAD(&wq->flusher_overflow);
+	init_waitqueue_head(&wq->waitq);
 
 	wq->name = name;
 	lockdep_init_map(&wq->lockdep_map, lock_name, key, 0);
@@ -3124,7 +3129,7 @@ void workqueue_set_max_active(struct wor
 		spin_lock_irq(&gcwq->lock);
 
 		if (!(wq->flags & WQ_FREEZABLE) ||
-		    !(gcwq->flags & GCWQ_FREEZING))
+		    !(gcwq->flags & (GCWQ_FREEZING | GCWQ_PAUSING)))
 			get_cwq(gcwq->cpu, wq)->max_active = max_active;
 
 		spin_unlock_irq(&gcwq->lock);
@@ -3377,7 +3382,7 @@ static int __cpuinit trustee_thread(void
 	 * completion while frozen.
 	 */
 	while (gcwq->nr_workers != gcwq->nr_idle ||
-	       gcwq->flags & GCWQ_FREEZING ||
+	       gcwq->flags & (GCWQ_FREEZING | GCWQ_PAUSING) ||
 	       gcwq->trustee_state == TRUSTEE_IN_CHARGE) {
 		int nr_works = 0;
 
@@ -3767,6 +3772,65 @@ out_unlock:
 }
 #endif /* CONFIG_FREEZER */
 
+static unsigned int count_active(struct workqueue_struct *wq)
+{
+	unsigned int cpu, active = 0;
+
+	for_each_cwq_cpu(cpu, wq) {
+		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+		struct global_cwq *gcwq = cwq->gcwq;
+
+		spin_lock_irq(&gcwq->lock);
+		active += cwq->nr_active;
+		spin_unlock_irq(&gcwq->lock);
+	}
+
+	return active;
+}
+
+void pause_workqueue(struct workqueue_struct *wq)
+{
+	unsigned int cpu;
+
+	for_each_cwq_cpu(cpu, wq) {
+		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+		struct global_cwq *gcwq = cwq->gcwq;
+
+		spin_lock_irq(&gcwq->lock);
+
+		WARN_ON(gcwq->flags & GCWQ_PAUSING);
+		gcwq->flags |= GCWQ_PAUSING;
+
+		cwq->max_active = 0;
+
+		spin_unlock_irq(&gcwq->lock);
+	}
+
+	wait_event(wq->waitq, count_active(wq) == 0);
+}
+EXPORT_SYMBOL(pause_workqueue);
+
+void resume_workqueue(struct workqueue_struct *wq)
+{
+	unsigned int cpu;
+
+	for_each_cwq_cpu(cpu, wq) {
+		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+		struct global_cwq *gcwq = cwq->gcwq;
+
+		spin_lock_irq(&gcwq->lock);
+
+		WARN_ON(!(gcwq->flags & GCWQ_PAUSING));
+		gcwq->flags &= ~GCWQ_PAUSING;
+
+		cwq->max_active = wq->saved_max_active;
+
+		wake_up_worker(gcwq);
+		spin_unlock_irq(&gcwq->lock);
+	}
+}
+EXPORT_SYMBOL(resume_workqueue);
+
 static int __init init_workqueues(void)
 {
 	unsigned int cpu;

Re: workqueue_set_max_active(wq, 0)?

[Tejun Heo]

Hello, Johannes.

On Thu, Dec 15, 2011 at 04:38:12PM +0100, Johannes Berg wrote:
> --- a/kernel/workqueue.c	2011-12-10 17:32:26.000000000 +0100
> +++ b/kernel/workqueue.c	2011-12-15 16:36:06.000000000 +0100
> @@ -51,6 +51,7 @@ enum {
>  	GCWQ_DISASSOCIATED	= 1 << 2,	/* cpu can't serve workers */
>  	GCWQ_FREEZING		= 1 << 3,	/* freeze in progress */
>  	GCWQ_HIGHPRI_PENDING	= 1 << 4,	/* highpri works on queue */
> +	GCWQ_PAUSING		= 1 << 5,

Hmmm... confused.  Pausing is per-wq, why is this flag on gcwq?
Shouldn't it be on workqueue_struct?

> +void pause_workqueue(struct workqueue_struct *wq)
> +{
> +	unsigned int cpu;
> +
> +	for_each_cwq_cpu(cpu, wq) {
> +		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
> +		struct global_cwq *gcwq = cwq->gcwq;
> +
> +		spin_lock_irq(&gcwq->lock);
> +
> +		WARN_ON(gcwq->flags & GCWQ_PAUSING);
> +		gcwq->flags |= GCWQ_PAUSING;
> +
> +		cwq->max_active = 0;
> +
> +		spin_unlock_irq(&gcwq->lock);
> +	}
> +
> +	wait_event(wq->waitq, count_active(wq) == 0);
> +}
> +EXPORT_SYMBOL(pause_workqueue);

What if there are multiple callers of this function on the same wq?
Maybe something like wq->pause_depth and also use it from freeze path?

> +void resume_workqueue(struct workqueue_struct *wq)
> +{
> +	unsigned int cpu;
> +
> +	for_each_cwq_cpu(cpu, wq) {
> +		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
> +		struct global_cwq *gcwq = cwq->gcwq;
> +
> +		spin_lock_irq(&gcwq->lock);
> +
> +		WARN_ON(!(gcwq->flags & GCWQ_PAUSING));
> +		gcwq->flags &= ~GCWQ_PAUSING;
> +
> +		cwq->max_active = wq->saved_max_active;
> +
> +		wake_up_worker(gcwq);
> +		spin_unlock_irq(&gcwq->lock);
> +	}
> +}
> +EXPORT_SYMBOL(resume_workqueue);

I don't think wake_up_worker() would be sufficient.
cwq_activate_first_delayed() needs to be called to kick the delayed
work items.

I think it would be great if this can be abstracted out so that both
the freezer and explicit pausing use the same facility.  They aren't
that different after all.

Thanks.

-- 
tejun

Re: workqueue_set_max_active(wq, 0)?

[Johannes Berg]

Hi,

> > @@ -51,6 +51,7 @@ enum {
> >  	GCWQ_DISASSOCIATED	= 1 << 2,	/* cpu can't serve workers */
> >  	GCWQ_FREEZING		= 1 << 3,	/* freeze in progress */
> >  	GCWQ_HIGHPRI_PENDING	= 1 << 4,	/* highpri works on queue */
> > +	GCWQ_PAUSING		= 1 << 5,
> 
> Hmmm... confused.  Pausing is per-wq, why is this flag on gcwq?
> Shouldn't it be on workqueue_struct?

Hm, no idea :-)
I just copied FREEZING really without knowing what I was doing. I'm not
very familiar with this code (yet).

> > +void pause_workqueue(struct workqueue_struct *wq)
> > +{
> > +	unsigned int cpu;
> > +
> > +	for_each_cwq_cpu(cpu, wq) {
> > +		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
> > +		struct global_cwq *gcwq = cwq->gcwq;
> > +
> > +		spin_lock_irq(&gcwq->lock);
> > +
> > +		WARN_ON(gcwq->flags & GCWQ_PAUSING);
> > +		gcwq->flags |= GCWQ_PAUSING;
> > +
> > +		cwq->max_active = 0;
> > +
> > +		spin_unlock_irq(&gcwq->lock);
> > +	}
> > +
> > +	wait_event(wq->waitq, count_active(wq) == 0);
> > +}
> > +EXPORT_SYMBOL(pause_workqueue);
> 
> What if there are multiple callers of this function on the same wq?
> Maybe something like wq->pause_depth and also use it from freeze path?

Hm, good point. We can't abstract out all of it -- the freezer API
doesn't want to wait for it to finish -- but probably a bit of it.

How do you iterate workqueues? We'd have to do that for the freezer
part, unless we want to work on CWQs again.

Actually I'm not really sure I understand the differences between WQ,
CWQ and GCWQ...

> > +void resume_workqueue(struct workqueue_struct *wq)
> > +{
> > +	unsigned int cpu;
> > +
> > +	for_each_cwq_cpu(cpu, wq) {
> > +		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
> > +		struct global_cwq *gcwq = cwq->gcwq;
> > +
> > +		spin_lock_irq(&gcwq->lock);
> > +
> > +		WARN_ON(!(gcwq->flags & GCWQ_PAUSING));
> > +		gcwq->flags &= ~GCWQ_PAUSING;
> > +
> > +		cwq->max_active = wq->saved_max_active;
> > +
> > +		wake_up_worker(gcwq);
> > +		spin_unlock_irq(&gcwq->lock);
> > +	}
> > +}
> > +EXPORT_SYMBOL(resume_workqueue);
> 
> I don't think wake_up_worker() would be sufficient.
> cwq_activate_first_delayed() needs to be called to kick the delayed
> work items.

Hm, ok.

> I think it would be great if this can be abstracted out so that both
> the freezer and explicit pausing use the same facility.  They aren't
> that different after all.

I'll take a look tomorrow. If you want to beat me to it ... ;-)

johannes

Re: workqueue_set_max_active(wq, 0)?

[Tejun Heo]

Hello,

On Thu, Dec 15, 2011 at 07:43:40PM +0100, Johannes Berg wrote:
> Hm, good point. We can't abstract out all of it -- the freezer API
> doesn't want to wait for it to finish -- but probably a bit of it.
> 
> How do you iterate workqueues? We'd have to do that for the freezer
> part, unless we want to work on CWQs again.

By Locking workqueue_lock and walking workqueues list.  Hmmm...

> Actually I'm not really sure I understand the differences between WQ,
> CWQ and GCWQ...

WQ is workqueue - the part visible to users.

CWQ is cpu workqueue.  Each wq has its own set of cpu workqueues for
all CPUs (there are exceptions but this should be a good enough
explanation).  A WQ is always a set of cwq's.  WQ chooses which CWQ to
use on queue but most of actual processing happens on CWQs.

GCWQ stands for global cpu workqueue - there's one for each CPU.  This
is per-cpu global worker pool used by all workqueues.  Every CWQ on a
CPU shares the GCWQ on that CPU.

The reason why FREEZING currently is on GCWQ is because freezing is a
system wide operation.  If we're gonna implement pause, I think it
should probably be in cwq.

> > I think it would be great if this can be abstracted out so that both
> > the freezer and explicit pausing use the same facility.  They aren't
> > that different after all.
> 
> I'll take a look tomorrow. If you want to beat me to it ... ;-)

Heh heh, [un]fortunately, I'm pretty occupied at the moment.  :P

Thank you.

-- 
tejun

Re: workqueue_set_max_active(wq, 0)?

[Johannes Berg]

On Thu, 2011-12-15 at 11:12 -0800, Tejun Heo wrote:

> > Hm, good point. We can't abstract out all of it -- the freezer API
> > doesn't want to wait for it to finish -- but probably a bit of it.
> > 
> > How do you iterate workqueues? We'd have to do that for the freezer
> > part, unless we want to work on CWQs again.
> 
> By Locking workqueue_lock and walking workqueues list.  Hmmm...

Ah. So fundamentally, the freeze code does:

 * set each gcwq frozen
 * set max_active=0 for each CWQ in each WQ

but it interleaves the two loops. I guess this would have to be
untangled if we want to share it so it sets all gcwq frozen and then
iterates the workqueues and their CWQs. Locking seems a bit hairy
though, why does the current code keep the GCWQ lock over CWQ changes? I
guess that's so nothing can work on the CWQ?

> > Actually I'm not really sure I understand the differences between WQ,
> > CWQ and GCWQ...
> 
[snip explanation]

thanks.

> The reason why FREEZING currently is on GCWQ is because freezing is a
> system wide operation.  If we're gonna implement pause, I think it
> should probably be in cwq.

Ok, makes sense too.

I think I'm going to do something simpler first though, the locking
scares me a bit. I'll do something for my single-threaded max-active=1
workqueue first directly in mac80211 to try out the idea ...

johannes

Re: workqueue_set_max_active(wq, 0)?

[Tejun Heo]

Hello,

On Thu, Dec 15, 2011 at 08:26:01PM +0100, Johannes Berg wrote:
> Ah. So fundamentally, the freeze code does:
> 
>  * set each gcwq frozen
>  * set max_active=0 for each CWQ in each WQ

Yeap and then iterate over them waiting for all nr_actives to drop to
zero.

> but it interleaves the two loops. I guess this would have to be
> untangled if we want to share it so it sets all gcwq frozen and then
> iterates the workqueues and their CWQs. Locking seems a bit hairy
> though, why does the current code keep the GCWQ lock over CWQ changes? I
> guess that's so nothing can work on the CWQ?

All CWQ's are protected by the corresponding GCWQ lock, so all CWQs on
the same CPU are protected by single gcwq->lock on that CPU.  It's
actually rather simple.  The reason the loop there is interleaved is
to avoid releasing and grabbing different gcwq->lock's for each
iteration.  I don't think that would really matter either way.

Thanks.

-- 
tejun