workqueue: use enum value to set array size of pools in gcwq

We assign cpu id into work struct 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 sub-optimal path in case of WQ_NON_REENTRANT.
Change it to cpu argument is prevent to go into sub-optimal path.

Hello,

Quoted 7 lines · Joonsoo Kim

We assign cpu id into work struct 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 sub-optimal path in case of WQ_NON_REENTRANT.
Change it to cpu argument is prevent to go into sub-optimal path.

Which part is suboptimal? Also, what if @cpu is WQ_UNBOUND?

wq = WQ_NON_REENTRANT.
queue_delayed_work_on(CPU B) is invoked in CPU A, so lcpu = CPU A, cpu = CPU B.

In this case, we call add_time_on(CPU B), then delayed_work_timer_fn() is invoked on CPU B.
delayed_work_timer_fn() calls __queue_work(), then following comparisons return true!

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

I thinks that if we assign cpu to lcpu, above comparisons return false, so save some overheads.
Is there any missing part?

And, do u mean @cpu is WORK_CPU_UNBOUND?

Quoted 1 line

And, do u mean @cpu is WORK_CPU_UNBOUND?

@cpu could be WORK_CPU_UNBOUND at that point. The timer will be added to local CPU but @work->data would be pointing to WORK_CPU_UNBOUND, again triggering the condition. Given that @cpu being WORK_CPU_UNBOUND is far more common than an actual CPU number, the patch would actually increase spurious nrt lookups. The right thing to do is probably setting cpu to raw_smp_processor_id() beforehand.

Quoted 8 lines

Quoted 1 line

And, do u mean @cpu is WORK_CPU_UNBOUND?

@cpu could be WORK_CPU_UNBOUND at that point. The timer will be added to local CPU but @work->data would be pointing to WORK_CPU_UNBOUND, again triggering the condition. Given that @cpu being WORK_CPU_UNBOUND is far more common than an actual CPU number, the patch would actually increase spurious nrt lookups. The right thing to do is probably setting cpu to raw_smp_processor_id() beforehand.

I got your point.
Thanks for kind illustration.

Following is a alternative implementation for this.
I thinks this is too rare case, so it doesn't help in any real workload. But how do you thinks?

(quoted diff) +3 -1

@@ -1156,7 +1156,9 @@ int queue_delayed_work_on(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 (!gcwq)
+                               lcpu = cpu;
+                       else if (gcwq->cpu != WORK_CPU_UNBOUND)
                                lcpu = gcwq->cpu;
                        else

                                lcpu = raw_smp_processor_id();

Why not just do

        if (cpu == WORK_CPU_UNBOUND)
                cpu = raw_smp_processor_id();

        if (!(wq->flags...) {
                ...
                if (gcwq && gcwq->cpu != WORK_CPU_UNBOUND)
                        lcpu = gcwq->cpu;
                else
                        lcpu = cpu;
        }
        ...

        add_timer_on(timer, cpu);

Also, can you please base the patches on top of the following git branch?

git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-3.7

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 +5 -0

index f69f094..e0e1d41 100644
@@ -489,6 +489,11 @@ static int worker_pool_pri(struct worker_pool *pool)
 	return pool - pool->gcwq->pools;
 }
 
+static struct workqueue_struct *get_pool_system_wq(struct worker_pool *pool)
+{
+	return worker_pool_pri(pool) ? system_highpri_wq : system_wq;
+}
+
 static struct global_cwq *get_gcwq(unsigned int cpu)
 {
 	if (cpu != WORK_CPU_UNBOUND)

I think it would be better to just opencode system_wq selection in rebind_workers().

Quoted 2 lines

I think it would be better to just opencode system_wq selection in rebind_workers().

Sorry for my poor English skill.
Could you elaborate "opencode system_wq selection" means?

Quoted 5 lines

Quoted 2 lines

I think it would be better to just opencode system_wq selection in rebind_workers().

Sorry for my poor English skill.
Could you elaborate "opencode system_wq selection" means?

Dropping get_pool_system_wq() and putting the logic inside rebind_workers() directly. I tend to dislike short helpers which are used only once. They tend to obfuscate while not really contributing to anything else.

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.

Quoted 4 lines

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.

Is this from an actual workload? ie. do you have a test case where this matters?

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

Quoted 3 lines · Joonsoo Kim

Is this from an actual workload? ie. do you have a test case where this matters?

No. it is not from an actual workload.
I got this idea suddenly, during implementing others. I will do a test for this tomorrow.

Hello,

Quoted 5 lines · JoonSoo Kim

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

Quoted 3 lines · Joonsoo Kim

Is this from an actual workload? ie. do you have a test case where this matters?

Quoted 3 lines

No. it is not from an actual workload.
I got this idea suddenly, during implementing others. I will do a test for this tomorrow.

I don't think it's a bad idea and guess this wouldn't make much difference under most workload. Was mostly wondering whether this came from some crazy phone powersave optimization.