Skip to contentSkip to conversation

system_highpri_wq

PATCH Applied Joonsoo Kim 2 participants 9 messages Aug 15, 2012 – Aug 17, 2012 Workqueue

lore
Share & tools
lore

Joonsoo Kim

author
#0 lore raw

v3 cover system_highpri_wq

Change from v2
[1/6] No change
[2/6] Change local variable name and use it directly for TP [3/6] Add a comment.
[4/6] No change
[5/6] Add a comment. Fix ugly indentation.
[6/6] No change

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 |   42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH v3 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
author
#2 lore raw

PATCH v3 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 requested cpu.

Signed-off-by Joonsoo Kim <[email protected]>
kernel/workqueue.c +2 -1
index 49d8f4a..c29f2dc 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 req_cpu = cpu;
 
 	/*
 	 * While a work item is PENDING && off queue, a task trying to

@@ -1253,7 +1254,7 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
 
 	/* gcwq determined, get cwq and queue */
 	cwq = get_cwq(gcwq->cpu, wq);
-	trace_workqueue_queue_work(cpu, cwq, work);
+	trace_workqueue_queue_work(req_cpu, cwq, work);
 
 	if (WARN_ON(!list_empty(&work->entry))) {
 		spin_unlock(&gcwq->lock);
Signature · 1 line
1.7.9.5
author
#3 lore raw

PATCH v3 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 +9 -2
index c29f2dc..32c4f79 100644
@@ -1356,9 +1356,16 @@ 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)
+		/*
+		 * If we cannot get gcwq from work directly, we should
+		 * deliberately select last cpu not to go into sub-optimal
+		 * path of reentrance detection for delayed work. In this case,
+		 * we assign requested cpu to lcpu except 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
author
#4 lore raw

PATCH v3 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 32c4f79..a768ffd 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);

@@ -3929,6 +3931,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,

@@ -3937,8 +3940,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
author
#5 lore raw

PATCH v3 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 +12 -4
index a768ffd..2945734 100644
@@ -1742,6 +1742,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;

@@ -1751,11 +1752,18 @@ retry:
 				     work_data_bits(rebind_work)))
 			continue;
 
-		/* 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));
+		/*
+		 * Now, we have two types of worker based on priority.
+		 * Therefore, each should choose the correct wq.
+		 */
+		if (worker_pool_pri(worker->pool))
+			wq = system_highpri_wq;
+		else
+			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
author
#6 lore raw

PATCH v3 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 2945734..e17326e 100644
@@ -3679,7 +3679,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
maintainer Applied
#7 lore raw
In reply to Joonsoo Kim
Quoted 20 lines · Joonsoo Kim

Change from v2
[1/6] No change
[2/6] Change local variable name and use it directly for TP [3/6] Add a comment.
[4/6] No change
[5/6] Add a comment. Fix ugly indentation.
[6/6] No change

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

Quoted 6 lines

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.

Applied to wq/for-3.7 w/ minor updates.

Thanks.

Signature · 1 line
tejun
author
#8 lore raw

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

Quoted 28 lines · Joonsoo Kim
Quoted 20 lines

Change from v2
[1/6] No change
[2/6] Change local variable name and use it directly for TP [3/6] Add a comment.
[4/6] No change
[5/6] Add a comment. Fix ugly indentation.
[6/6] No change

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

Quoted 6 lines

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.

Quoted 6 lines

Applied to wq/for-3.7 w/ minor updates.

Thanks.

--
tejun

Hello.
I checked your minor update and it looks better.
Sorry for my poor explanation for patches.

Thank you very much.