* workqueue_set_max_active(wq, 0)?
@ 2011-12-09 9:54 Johannes Berg
2011-12-09 16:57 ` Tejun Heo
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2011-12-09 9:54 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: workqueue_set_max_active(wq, 0)?
2011-12-09 9:54 workqueue_set_max_active(wq, 0)? Johannes Berg
@ 2011-12-09 16:57 ` Tejun Heo
2011-12-09 16:59 ` Johannes Berg
2011-12-15 15:38 ` Johannes Berg
0 siblings, 2 replies; 9+ messages in thread
From: Tejun Heo @ 2011-12-09 16:57 UTC (permalink / raw)
To: Johannes Berg; +Cc: LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: workqueue_set_max_active(wq, 0)?
2011-12-09 16:57 ` Tejun Heo
@ 2011-12-09 16:59 ` Johannes Berg
2011-12-15 15:38 ` Johannes Berg
1 sibling, 0 replies; 9+ messages in thread
From: Johannes Berg @ 2011-12-09 16:59 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: workqueue_set_max_active(wq, 0)?
2011-12-09 16:57 ` Tejun Heo
2011-12-09 16:59 ` Johannes Berg
@ 2011-12-15 15:38 ` Johannes Berg
2011-12-15 18:35 ` Tejun Heo
1 sibling, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2011-12-15 15:38 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML
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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: workqueue_set_max_active(wq, 0)?
2011-12-15 15:38 ` Johannes Berg
@ 2011-12-15 18:35 ` Tejun Heo
2011-12-15 18:43 ` Johannes Berg
0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2011-12-15 18:35 UTC (permalink / raw)
To: Johannes Berg; +Cc: LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: workqueue_set_max_active(wq, 0)?
2011-12-15 18:35 ` Tejun Heo
@ 2011-12-15 18:43 ` Johannes Berg
2011-12-15 19:12 ` Tejun Heo
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2011-12-15 18:43 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: workqueue_set_max_active(wq, 0)?
2011-12-15 18:43 ` Johannes Berg
@ 2011-12-15 19:12 ` Tejun Heo
2011-12-15 19:26 ` Johannes Berg
0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2011-12-15 19:12 UTC (permalink / raw)
To: Johannes Berg; +Cc: LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: workqueue_set_max_active(wq, 0)?
2011-12-15 19:12 ` Tejun Heo
@ 2011-12-15 19:26 ` Johannes Berg
2011-12-15 19:31 ` Tejun Heo
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2011-12-15 19:26 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: workqueue_set_max_active(wq, 0)?
2011-12-15 19:26 ` Johannes Berg
@ 2011-12-15 19:31 ` Tejun Heo
0 siblings, 0 replies; 9+ messages in thread
From: Tejun Heo @ 2011-12-15 19:31 UTC (permalink / raw)
To: Johannes Berg; +Cc: LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-12-15 19:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-09 9:54 workqueue_set_max_active(wq, 0)? Johannes Berg
2011-12-09 16:57 ` Tejun Heo
2011-12-09 16:59 ` Johannes Berg
2011-12-15 15:38 ` Johannes Berg
2011-12-15 18:35 ` Tejun Heo
2011-12-15 18:43 ` Johannes Berg
2011-12-15 19:12 ` Tejun Heo
2011-12-15 19:26 ` Johannes Berg
2011-12-15 19:31 ` Tejun Heo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.