system_highpri_wq

PATCH Superseded Joonsoo Kim 2 participants 11 messages Aug 14, 2012 Workqueue

lore

Share & tools

lore

Joonsoo Kim

author

2012-08-14 18:11 UTC #0 lore raw

v2 cover system_highpri_wq

Change from v1
[ALL] Add cover-letter
[ALL] Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-3.7

[1/6] No change
[2/6] First added into this series.
[3/6] Add change log to clarify what pacth do,
Add logic to handle the case "@cpu is WORK_CPU_UNBOUND"
[4/6] Fix awsome mistake: Set WQ_HIGHPRI for system_highpri_wq [5/6] Adopt Tejun's comment about selection system_wq [6/6] No change

This patchset introduce system_highpri_wq
in order to use proper cwq for highpri worker.

First 3 patches are not related to that purpose.
Just fix arbitrary issues.
Last 3 patches are for our purpose.

Joonsoo Kim (6):
workqueue: use enum value to set array size of pools in gcwq
workqueue: correct req_cpu in trace_workqueue_queue_work()
workqueue: change value of lcpu in __queue_delayed_work_on()
workqueue: introduce system_highpri_wq
workqueue: use system_highpri_wq for highpri workers in
rebind_workers()
workqueue: use system_highpri_wq for unbind_work

 kernel/workqueue.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

Signature · 1 line

1.7.9.5

Joonsoo Kim

author

2012-08-14 18:11 UTC #1 lore raw

PATCH v2 1/6 workqueue: use enum value to set array size of pools in gcwq

Commit 3270476a6c0ce322354df8679652f060d66526dc ('workqueue: reimplement WQ_HIGHPRI using a separate worker_pool') introduce separate worker_pool for HIGHPRI. Although there is NR_WORKER_POOLS enum value which represent size of pools, definition of worker_pool in gcwq doesn't use it. Using it makes code robust and prevent future mistakes.
So change code to use this enum value.

Signed-off-by Joonsoo Kim <[email protected]>

kernel/workqueue.c +2 -1

index 4fef952..49d8f4a 100644
@@ -183,7 +183,8 @@ struct global_cwq {
 	struct hlist_head	busy_hash[BUSY_WORKER_HASH_SIZE];
 						/* L: hash of busy workers */
 
-	struct worker_pool	pools[2];	/* normal and highpri pools */
+	struct worker_pool	pools[NR_WORKER_POOLS];
+						/* normal and highpri pools */
 
 	wait_queue_head_t	rebind_hold;	/* rebind hold wait */
 } ____cacheline_aligned_in_smp;

Signature · 1 line

1.7.9.5

Joonsoo Kim

author

2012-08-14 18:11 UTC #2 lore raw

PATCH v2 2/6 workqueue: correct req_cpu in trace_workqueue_queue_work()

In reply to Joonsoo Kim

When we do tracing workqueue_queue_work(), it records requested cpu. But, if !(@wq->flag & WQ_UNBOUND) and @cpu is WORK_CPU_UNBOUND, requested cpu is changed as local cpu.
In case of @wq->flag & WQ_UNBOUND, above change is not occured, therefore it is reasonable to correct it.

Use temporary local variable for storing local cpu.

Signed-off-by Joonsoo Kim <[email protected]>

kernel/workqueue.c +5 -2

index 49d8f4a..6a17ab0 100644
@@ -1198,6 +1198,7 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
 	struct cpu_workqueue_struct *cwq;
 	struct list_head *worklist;
 	unsigned int work_flags;
+	unsigned int lcpu;
 
 	/*
 	 * While a work item is PENDING && off queue, a task trying to

@@ -1219,7 +1220,9 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
 		struct global_cwq *last_gcwq;
 
 		if (cpu == WORK_CPU_UNBOUND)
-			cpu = raw_smp_processor_id();
+			lcpu = raw_smp_processor_id();
+		else
+			lcpu = cpu;
 
 		/*
 		 * It's multi cpu.  If @wq is non-reentrant and @work

@@ -1227,7 +1230,7 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
 		 * be running there, in which case the work needs to
 		 * be queued on that cpu to guarantee non-reentrance.
 		 */
-		gcwq = get_gcwq(cpu);
+		gcwq = get_gcwq(lcpu);
 		if (wq->flags & WQ_NON_REENTRANT &&
 		    (last_gcwq = get_work_gcwq(work)) && last_gcwq != gcwq) {
 			struct worker *worker;

Signature · 1 line

1.7.9.5

Tejun Heo

maintainer

2012-08-14 18:35 UTC #3 lore raw

Re: [PATCH v2 2/6] workqueue: correct req_cpu in trace_workqueue_queue_work()

Hello,

Quoted 15 lines · Joonsoo Kim

When we do tracing workqueue_queue_work(), it records requested cpu. But, if !(@wq->flag & WQ_UNBOUND) and @cpu is WORK_CPU_UNBOUND, requested cpu is changed as local cpu.
In case of @wq->flag & WQ_UNBOUND, above change is not occured, therefore it is reasonable to correct it.
Use temporary local variable for storing local cpu.
Signed-off-by Joonsoo Kim <[email protected]>
kernel/workqueue.c +0 -0
index 49d8f4a..6a17ab0 100644

kernel/workqueue.c static void __queue_work(unsigned int cpu, struct workqueue_struct *wq, · line 1201

Quoted 4 lines

kernel/workqueue.c +1 -0

@@ -1198,6 +1198,7 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
 	struct cpu_workqueue_struct *cwq;
 	struct list_head *worklist;
 	unsigned int work_flags;
+	unsigned int lcpu;

@lcpu in __queue_delayed_work() stands for "last cpu", which is kinda weird here. Maybe just add "unsigned int req_cpu = cpu" at the top of the function and use it for the TP?

Thanks.

Signature · 1 line

tejun

JoonSoo Kim

author

2012-08-14 18:56 UTC #4 lore raw

Re: [PATCH v2 2/6] workqueue: correct req_cpu in trace_workqueue_queue_work()

2012/8/15 Tejun Heo <[email protected]>:

Quoted 13 lines

Hello,
Quoted 10 lines · Joonsoo Kim
When we do tracing workqueue_queue_work(), it records requested cpu. But, if !(@wq->flag & WQ_UNBOUND) and @cpu is WORK_CPU_UNBOUND, requested cpu is changed as local cpu.
In case of @wq->flag & WQ_UNBOUND, above change is not occured, therefore it is reasonable to correct it.
Use temporary local variable for storing local cpu.
Signed-off-by Joonsoo Kim <[email protected]>

Quoted 13 lines

kernel/workqueue.c static void __queue_work(unsigned int cpu, struct workqueue_struct *wq, · line 1201
Quoted 9 lines
kernel/workqueue.c +1 -0
index 49d8f4a..6a17ab0 100644
@@ -1198,6 +1198,7 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
      struct cpu_workqueue_struct *cwq;
      struct list_head *worklist;
      unsigned int work_flags;
+     unsigned int lcpu;
@lcpu in __queue_delayed_work() stands for "last cpu", which is kinda weird here. Maybe just add "unsigned int req_cpu = cpu" at the top of the function and use it for the TP?

Okay! It looks better.
I will re-send v3 of two patches commented from u. Thanks!

Joonsoo Kim

author

2012-08-14 18:12 UTC #5 lore raw

PATCH v2 3/6 workqueue: change value of lcpu in __queue_delayed_work_on()

In reply to Joonsoo Kim

We assign cpu id into work struct's data field in __queue_delayed_work_on(). In current implementation, when work is come in first time, current running cpu id is assigned.
If we do __queue_delayed_work_on() with CPU A on CPU B, __queue_work() invoked in delayed_work_timer_fn() go into
the following sub-optimal path in case of WQ_NON_REENTRANT.

	gcwq = get_gcwq(cpu);
	if (wq->flags & WQ_NON_REENTRANT &&
		(last_gcwq = get_work_gcwq(work)) && last_gcwq != gcwq) {

Change lcpu to @cpu and rechange lcpu to local cpu if lcpu is WORK_CPU_UNBOUND. It is sufficient to prevent to go into sub-optimal path.

Signed-off-by Joonsoo Kim <[email protected]>

kernel/workqueue.c +3 -2

index 6a17ab0..f55ac26 100644
@@ -1358,9 +1358,10 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
 	if (!(wq->flags & WQ_UNBOUND)) {
 		struct global_cwq *gcwq = get_work_gcwq(work);
 
-		if (gcwq && gcwq->cpu != WORK_CPU_UNBOUND)
+		lcpu = cpu;
+		if (gcwq)
 			lcpu = gcwq->cpu;
-		else
+		if (lcpu == WORK_CPU_UNBOUND)
 			lcpu = raw_smp_processor_id();
 	} else {
 		lcpu = WORK_CPU_UNBOUND;

Signature · 1 line

1.7.9.5

Tejun Heo

maintainer

2012-08-14 19:00 UTC #6 lore raw

Re: [PATCH v2 3/6] workqueue: change value of lcpu in __queue_delayed_work_on()

Quoted 21 lines · Joonsoo Kim

We assign cpu id into work struct's data field in __queue_delayed_work_on(). In current implementation, when work is come in first time, current running cpu id is assigned.
If we do __queue_delayed_work_on() with CPU A on CPU B, __queue_work() invoked in delayed_work_timer_fn() go into
the following sub-optimal path in case of WQ_NON_REENTRANT.
	gcwq = get_gcwq(cpu);
	if (wq->flags & WQ_NON_REENTRANT &&
		(last_gcwq = get_work_gcwq(work)) && last_gcwq != gcwq) {
Change lcpu to @cpu and rechange lcpu to local cpu if lcpu is WORK_CPU_UNBOUND. It is sufficient to prevent to go into sub-optimal path.
Signed-off-by Joonsoo Kim <[email protected]>
kernel/workqueue.c +0 -0
index 6a17ab0..f55ac26 100644

kernel/workqueue.c static void __queue_delayed_work(int cpu, struct workqueue_struct *wq, · line 1365

Quoted 10 lines

kernel/workqueue.c +3 -2

@@ -1358,9 +1358,10 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
 	if (!(wq->flags & WQ_UNBOUND)) {
 		struct global_cwq *gcwq = get_work_gcwq(work);
 
-		if (gcwq && gcwq->cpu != WORK_CPU_UNBOUND)
+		lcpu = cpu;
+		if (gcwq)
 			lcpu = gcwq->cpu;
-		else
+		if (lcpu == WORK_CPU_UNBOUND)
 			lcpu = raw_smp_processor_id();

Can you please add a comment explanining what's going on?

Signature · 1 line

tejun

Joonsoo Kim

author

2012-08-14 18:11 UTC #7 lore raw

PATCH v2 4/6 workqueue: introduce system_highpri_wq

In reply to Joonsoo Kim

Commit 3270476a6c0ce322354df8679652f060d66526dc ('workqueue: reimplement WQ_HIGHPRI using a separate worker_pool') introduce separate worker pool for HIGHPRI. When we handle busyworkers for gcwq, it can be normal worker or highpri worker. But, we don't consider this difference in rebind_workers(), we use just system_wq for highpri worker. It makes mismatch between cwq->pool and worker->pool.

It doesn't make error in current implementation, but possible in the future. Now, we introduce system_highpri_wq to use proper cwq for highpri workers in rebind_workers(). Following patch fix this issue properly.

Signed-off-by Joonsoo Kim <[email protected]>

kernel/workqueue.c +5 -2

index f55ac26..470b0eb 100644
@@ -269,12 +269,14 @@ struct workqueue_struct {
 };
 
 struct workqueue_struct *system_wq __read_mostly;
+struct workqueue_struct *system_highpri_wq __read_mostly;
 struct workqueue_struct *system_long_wq __read_mostly;
 struct workqueue_struct *system_nrt_wq __read_mostly;
 struct workqueue_struct *system_unbound_wq __read_mostly;
 struct workqueue_struct *system_freezable_wq __read_mostly;
 struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
 EXPORT_SYMBOL_GPL(system_wq);
+EXPORT_SYMBOL_GPL(system_highpri_wq);
 EXPORT_SYMBOL_GPL(system_long_wq);
 EXPORT_SYMBOL_GPL(system_nrt_wq);
 EXPORT_SYMBOL_GPL(system_unbound_wq);

@@ -3925,6 +3927,7 @@ static int __init init_workqueues(void)
 	}
 
 	system_wq = alloc_workqueue("events", 0, 0);
+	system_highpri_wq = alloc_workqueue("events_highpri", WQ_HIGHPRI, 0);
 	system_long_wq = alloc_workqueue("events_long", 0, 0);
 	system_nrt_wq = alloc_workqueue("events_nrt", WQ_NON_REENTRANT, 0);
 	system_unbound_wq = alloc_workqueue("events_unbound", WQ_UNBOUND,

@@ -3933,8 +3936,8 @@ static int __init init_workqueues(void)
 					      WQ_FREEZABLE, 0);
 	system_nrt_freezable_wq = alloc_workqueue("events_nrt_freezable",
 			WQ_NON_REENTRANT | WQ_FREEZABLE, 0);
-	BUG_ON(!system_wq || !system_long_wq || !system_nrt_wq ||
-	       !system_unbound_wq || !system_freezable_wq ||
+	BUG_ON(!system_wq || !system_highpri_wq || !system_long_wq ||
+		!system_nrt_wq || !system_unbound_wq || !system_freezable_wq ||
 		!system_nrt_freezable_wq);
 	return 0;
 }

Signature · 1 line

1.7.9.5

Joonsoo Kim

author

2012-08-14 18:11 UTC #8 lore raw

PATCH v2 5/6 workqueue: use system_highpri_wq for highpri workers in rebind_workers()

In reply to Joonsoo Kim

In rebind_workers(), we do inserting a work to rebind to cpu for busy workers. Currently, in this case, we use only system_wq. This makes a possible error situation as there is mismatch between cwq->pool and worker->pool.

To prevent this, we should use system_highpri_wq for highpri worker to match theses. This implements it.

Signed-off-by Joonsoo Kim <[email protected]>

kernel/workqueue.c +7 -3

index 470b0eb..4c5733c1 100644
@@ -1738,6 +1738,7 @@ retry:
 	/* rebind busy workers */
 	for_each_busy_worker(worker, i, pos, gcwq) {
 		struct work_struct *rebind_work = &worker->rebind_work;
+		struct workqueue_struct *wq;
 
 		/* morph UNBOUND to REBIND */
 		worker->flags &= ~WORKER_UNBOUND;

@@ -1749,9 +1750,12 @@ retry:
 
 		/* wq doesn't matter, use the default one */
 		debug_work_activate(rebind_work);
-		insert_work(get_cwq(gcwq->cpu, system_wq), rebind_work,
-			    worker->scheduled.next,
-			    work_color_to_flags(WORK_NO_COLOR));
+		wq = worker_pool_pri(worker->pool) ? system_highpri_wq :
+								system_wq;
+		insert_work(
+			get_cwq(gcwq->cpu, wq),
+			rebind_work, worker->scheduled.next,
+			work_color_to_flags(WORK_NO_COLOR));
 	}
 }

Signature · 1 line

1.7.9.5

Tejun Heo

maintainer

2012-08-14 18:29 UTC #9 lore raw

Re: [PATCH v2 5/6] workqueue: use system_highpri_wq for highpri workers in rebind_workers()

Quoted 24 lines · Joonsoo Kim

In rebind_workers(), we do inserting a work to rebind to cpu for busy workers. Currently, in this case, we use only system_wq. This makes a possible error situation as there is mismatch between cwq->pool and worker->pool.
To prevent this, we should use system_highpri_wq for highpri worker to match theses. This implements it.
Signed-off-by Joonsoo Kim <[email protected]>
kernel/workqueue.c +1 -0
index 470b0eb..4c5733c1 100644
@@ -1738,6 +1738,7 @@ retry:
 	/* rebind busy workers */
 	for_each_busy_worker(worker, i, pos, gcwq) {
 		struct work_struct *rebind_work = &worker->rebind_work;
+		struct workqueue_struct *wq;
 
 		/* morph UNBOUND to REBIND */
 		worker->flags &= ~WORKER_UNBOUND;
@@ -1749,9 +1750,12 @@ retry:
 
 		/* wq doesn't matter, use the default one */

kernel/workqueue.c retry: · line 1758

Quoted 10 lines

kernel/workqueue.c +6 -3

 		debug_work_activate(rebind_work);
-		insert_work(get_cwq(gcwq->cpu, system_wq), rebind_work,
-			    worker->scheduled.next,
-			    work_color_to_flags(WORK_NO_COLOR));
+		wq = worker_pool_pri(worker->pool) ? system_highpri_wq :
+								system_wq;
+		insert_work(
+			get_cwq(gcwq->cpu, wq),
+			rebind_work, worker->scheduled.next,
+			work_color_to_flags(WORK_NO_COLOR));

Umm... this indentation is ugly. Please follow the indentation of the surrounding code. If ?: gets too long, using if/else might be better. Please also comment why the above is necessary. The comment above says "wq doesn't matter" and then we're choosing workqueue, which is confusing.

Thanks.

Signature · 1 line

tejun

Joonsoo Kim

author

2012-08-14 18:11 UTC #10 lore raw

PATCH v2 6/6 workqueue: use system_highpri_wq for unbind_work

In reply to Joonsoo Kim

To speed cpu down processing up, use system_highpri_wq. As scheduling priority of workers on it is higher than system_wq and it is not contended by other normal works on this cpu, work on it is processed faster than system_wq.

Signed-off-by Joonsoo Kim <[email protected]>

kernel/workqueue.c +1 -1

index 4c5733c1..fa5ec4c 100644
@@ -3671,7 +3671,7 @@ static int __devinit workqueue_cpu_down_callback(struct notifier_block *nfb,
 	case CPU_DOWN_PREPARE:
 		/* unbinding should happen on the local CPU */
 		INIT_WORK_ONSTACK(&unbind_work, gcwq_unbind_fn);
-		schedule_work_on(cpu, &unbind_work);
+		queue_work_on(cpu, system_highpri_wq, &unbind_work);
 		flush_work(&unbind_work);
 		break;
 	}

Signature · 1 line

1.7.9.5