[RFC 0/4] workqueue: fix selecting cpu for queuing work and cleanup

[RFC 0/4] workqueue: fix selecting cpu for queuing work and cleanup

[Hillf Danton]

Selecting cpu is fixed for queuing work, which is followed by a couple
of minor cleanups.


[RFC 1/4] workqueue: fix selecting cpu for queuing work
[RFC 2/4] workqueue: use smp_processor_id() on queuing work
[RFC 3/4] workqueue: release dead pool workqueue on queuing work
[RFC 4/4] workqueue: use int for cpu on queuing work

[RFC 1/4] workqueue: fix selecting cpu for queuing work

[Hillf Danton]

Round robin is needed only for unbound workqueue and wq_unbound_cpumask
has nothing to do with standard workqueues, so we have to select cpu in
case of WORK_CPU_UNBOUND also with workqueue type taken into account.

Fixes: ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on wq_unbound_cpumask CPUs")
Signed-off-by: Hillf Danton <hdanton@sina.com>
---

--- a/kernel/workqueue.c
+++ c/kernel/workqueue.c
@@ -1409,16 +1409,19 @@ static void __queue_work(int cpu, struct
 	if (unlikely(wq->flags & __WQ_DRAINING) &&
 	    WARN_ON_ONCE(!is_chained_work(wq)))
 		return;
+
 	rcu_read_lock();
 retry:
-	if (req_cpu == WORK_CPU_UNBOUND)
-		cpu = wq_select_unbound_cpu(raw_smp_processor_id());
-
 	/* pwq which will be used unless @work is executing elsewhere */
-	if (!(wq->flags & WQ_UNBOUND))
-		pwq = per_cpu_ptr(wq->cpu_pwqs, cpu);
-	else
+	if (wq->flags & WQ_UNBOUND) {
+		if (req_cpu == WORK_CPU_UNBOUND)
+			cpu = wq_select_unbound_cpu(raw_smp_processor_id());
 		pwq = unbound_pwq_by_node(wq, cpu_to_node(cpu));
+	} else {
+		if (req_cpu == WORK_CPU_UNBOUND)
+			cpu = raw_smp_processor_id();
+		pwq = per_cpu_ptr(wq->cpu_pwqs, cpu);
+	}
 
 	/*
 	 * If @work was previously on a different pool, it might still be

Re: [RFC 1/4] workqueue: fix selecting cpu for queuing work

[Daniel Jordan]

[please cc maintainers]

On Wed, Dec 11, 2019 at 06:59:19PM +0800, Hillf Danton wrote:
> Round robin is needed only for unbound workqueue and wq_unbound_cpumask
> has nothing to do with standard workqueues, so we have to select cpu in
> case of WORK_CPU_UNBOUND also with workqueue type taken into account.

Good catch.  I'd include something like this in the changelog.

  Otherwise, work queued on a bound workqueue with WORK_CPU_UNBOUND might
  not prefer the local CPU if wq_unbound_cpumask is non-empty and doesn't
  include that CPU.

With that you can add

Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>

Re: [RFC 1/4] workqueue: fix selecting cpu for queuing work

[Daniel Jordan]

On Wed, Dec 11, 2019 at 06:07:35PM -0500, Daniel Jordan wrote:
> [please cc maintainers]
> 
> On Wed, Dec 11, 2019 at 06:59:19PM +0800, Hillf Danton wrote:
> > Round robin is needed only for unbound workqueue and wq_unbound_cpumask
> > has nothing to do with standard workqueues, so we have to select cpu in
> > case of WORK_CPU_UNBOUND also with workqueue type taken into account.
> 
> Good catch.  I'd include something like this in the changelog.
> 
>   Otherwise, work queued on a bound workqueue with WORK_CPU_UNBOUND might
>   not prefer the local CPU if wq_unbound_cpumask is non-empty and doesn't
>   include that CPU.
> 
> With that you can add
> 
> Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>

Any plans to repost this patch, Hillf?  If not, I can do it while retaining
your authorship.

Adding back the context, which I forgot to keep when adding the maintainers.

> > Fixes: ef557180447f ("workqueue: schedule WORK_CPU_UNBOUND work on wq_unbound_cpumask CPUs")
> > Signed-off-by: Hillf Danton <hdanton@sina.com>
> > ---
> > 
> > --- a/kernel/workqueue.c
> > +++ c/kernel/workqueue.c
> > @@ -1409,16 +1409,19 @@ static void __queue_work(int cpu, struct
> >  	if (unlikely(wq->flags & __WQ_DRAINING) &&
> >  	    WARN_ON_ONCE(!is_chained_work(wq)))
> >  		return;
> > +
> >  	rcu_read_lock();
> >  retry:
> > -	if (req_cpu == WORK_CPU_UNBOUND)
> > -		cpu = wq_select_unbound_cpu(raw_smp_processor_id());
> > -
> >  	/* pwq which will be used unless @work is executing elsewhere */
> > -	if (!(wq->flags & WQ_UNBOUND))
> > -		pwq = per_cpu_ptr(wq->cpu_pwqs, cpu);
> > -	else
> > +	if (wq->flags & WQ_UNBOUND) {
> > +		if (req_cpu == WORK_CPU_UNBOUND)
> > +			cpu = wq_select_unbound_cpu(raw_smp_processor_id());
> >  		pwq = unbound_pwq_by_node(wq, cpu_to_node(cpu));
> > +	} else {
> > +		if (req_cpu == WORK_CPU_UNBOUND)
> > +			cpu = raw_smp_processor_id();
> > +		pwq = per_cpu_ptr(wq->cpu_pwqs, cpu);
> > +	}
> >  
> >  	/*
> >  	 * If @work was previously on a different pool, it might still be
> > 
> > 

Re: [RFC 1/4] workqueue: fix selecting cpu for queuing work

[Hillf Danton]

On Thu, 23 Jan 2020 17:37:43 -0500 Daniel Jordan wrote:
> 
> Any plans to repost this patch, Hillf?  If not, I can do it while retaining
> your authorship.


Feel free to do it please and a Cc is enough.

Thanks
Hillf

[RFC 2/4] workqueue: use smp_processor_id() on queuing work

[Hillf Danton]

Use the regular version with irq disabled.

Signed-off-by: Hillf Danton <hdanton@sina.com>
---

--- c/kernel/workqueue.c
+++ d/kernel/workqueue.c
@@ -1415,11 +1415,11 @@ retry:
 	/* pwq which will be used unless @work is executing elsewhere */
 	if (wq->flags & WQ_UNBOUND) {
 		if (req_cpu == WORK_CPU_UNBOUND)
-			cpu = wq_select_unbound_cpu(raw_smp_processor_id());
+			cpu = wq_select_unbound_cpu(smp_processor_id());
 		pwq = unbound_pwq_by_node(wq, cpu_to_node(cpu));
 	} else {
 		if (req_cpu == WORK_CPU_UNBOUND)
-			cpu = raw_smp_processor_id();
+			cpu = smp_processor_id();
 		pwq = per_cpu_ptr(wq->cpu_pwqs, cpu);
 	}
 

[RFC 3/4] workqueue: reap dead pool workqueue on queuing work

[Hillf Danton]

Release rcu lock to reap dead pool workqueue.

Signed-off-by: Hillf Danton <hdanton@sina.com>
---

--- d/kernel/workqueue.c
+++ e/kernel/workqueue.c
@@ -1409,9 +1409,9 @@ static void __queue_work(int cpu, struct
 	if (unlikely(wq->flags & __WQ_DRAINING) &&
 	    WARN_ON_ONCE(!is_chained_work(wq)))
 		return;
-
-	rcu_read_lock();
 retry:
+	rcu_read_lock();
+
 	/* pwq which will be used unless @work is executing elsewhere */
 	if (wq->flags & WQ_UNBOUND) {
 		if (req_cpu == WORK_CPU_UNBOUND)
@@ -1458,6 +1458,7 @@ retry:
 	if (unlikely(!pwq->refcnt)) {
 		if (wq->flags & WQ_UNBOUND) {
 			spin_unlock(&pwq->pool->lock);
+			rcu_read_unlock();
 			cpu_relax();
 			goto retry;
 		}

Re: [RFC 3/4] workqueue: reap dead pool workqueue on queuing work

[Daniel Jordan]

On Wed, Dec 11, 2019 at 07:22:29PM +0800, Hillf Danton wrote:
> Release rcu lock to reap dead pool workqueue.

What's to be gained by reaping the pwq (and possibly worker pool and wq) before
__queue_work() retries?  It'll just happen after the queueing finishes.

Re: [RFC 3/4] workqueue: reap dead pool workqueue on queuing work

[Hillf Danton]

On Wed, 11 Dec 2019 18:25:04 -0500 Daniel Jordan wrote:
> On Wed, Dec 11, 2019 at 07:22:29PM +0800, Hillf Danton wrote:
> > Release rcu lock to reap dead pool workqueue.
> 
> What's to be gained by reaping the pwq (and possibly worker pool and wq) before
> __queue_work() retries?  It'll just happen after the queueing finishes.

Releasing rcu lock just says that the dead pwp no longer makes sense
on the local cpu and it can go now, without the local queuing work AFAICS
affected because of irq disabled. But it's hard to say how it will be
reclaimed on other cpus, say before this queuing ends, and this does
not matter in terms of the local queuing.

Hillf

[RFC 4/4] workqueue: use integer for cpu on queuing work

[Hillf Danton]

It's a minor cleanup, see d84ff0512f1b ("workqueue: consistently use
int for @cpu variables") for reasons.

Signed-off-by: Hillf Danton <hdanton@sina.com>
---

--- e/kernel/workqueue.c
+++ f/kernel/workqueue.c
@@ -1393,7 +1393,7 @@ static void __queue_work(int cpu, struct
 	struct worker_pool *last_pool;
 	struct list_head *worklist;
 	unsigned int work_flags;
-	unsigned int req_cpu = cpu;
+	int req_cpu = cpu;
 
 	/*
 	 * While a work item is PENDING && off queue, a task trying to