Re: [PATCH wq/for-6.9 v4 2/4] workqueue: Enable unbound cpumask update on ordered workqueues

[Waiman Long <longman@redhat.com>]

On 2/7/24 12:25, Tejun Heo wrote:
> Hello, Waiman.
>
> On Tue, Feb 06, 2024 at 08:19:09PM -0500, Waiman Long wrote:
> ...
>> + * The unplugging is done either in apply_wqattrs_cleanup() [fast path] when
>> + * the workqueue was idle or in pwq_release_workfn() [slow path] when the
>> + * workqueue was busy.
> I'm not sure the distinction between fast and slow paths is all that useful
> here. Both are really cold paths.
Yes, both are cold paths. Maybe a more accurate description is with 
respect to the latency that a new work item may experience since 
apply_wqattrs_cleanup() should be executed earlier than 
pwq_release_workfn().
>
>> +static void unplug_oldest_pwq(struct workqueue_struct *wq,
>> +			      struct pool_workqueue *exlude_pwq)
>> +{
>> +	struct pool_workqueue *pwq;
>> +	unsigned long flags;
>> +	bool found = false;
>> +
>> +	for_each_pwq(pwq, wq) {
>> +		if (pwq == exlude_pwq)
>> +			continue;
>> +		if (!pwq->plugged)
>> +			return;	/* No unplug needed */
>> +		found = true;
>> +		break;
>> +	}
>> +	if (WARN_ON_ONCE(!found))
>> +		return;
>> +
>> +	raw_spin_lock_irqsave(&pwq->pool->lock, flags);
>> +	if (!pwq->plugged)
>> +		goto out_unlock;
>> +	pwq->plugged = false;
>> +	if (pwq_activate_first_inactive(pwq, true))
>> +		kick_pool(pwq->pool);
>> +out_unlock:
>> +	raw_spin_unlock_irqrestore(&pwq->pool->lock, flags);
>> +}
> I don't quite understand why this needs iteration and @exclude_pwq.
> Shouldn't something like the following be enough?
>
> static void unplug_oldest_pwq(struct workqueue_struct *wq)
> {
> 	struct pool_workqueue *pwq;
>
> 	raw_spin_lock_irq(&pwq->pool->lock);
> 	pwq = list_first_entry_or_null(&pwq->pwqs, ...);
> 	if (pwq)
> 		pwq->plugged = false;
> 	raw_spin_unlock_irq(&pwq->pool->lock);
> }
>
It is because this function can be called from apply_wqattrs_cleanup() 
where I need to exclude ctx->dfl_pwq from being considered.
>> @@ -4740,6 +4796,13 @@ static void pwq_release_workfn(struct kthread_work *work)
>>   		mutex_lock(&wq->mutex);
>>   		list_del_rcu(&pwq->pwqs_node);
>>   		is_last = list_empty(&wq->pwqs);
>> +
>> +		/*
>> +		 * For ordered workqueue with a plugged dfl_pwq, restart it now.
>> +		 */
>> +		if (!is_last && (wq->flags & __WQ_ORDERED))
>> +			unplug_oldest_pwq(wq, NULL);
> This makes sense.
>
>> @@ -4906,8 +4969,26 @@ static void apply_wqattrs_cleanup(struct apply_wqattrs_ctx *ctx)
> ...
>> +		/*
>> +		 * It is possible that ctx->dfl_pwq (previous wq->dfl_pwq)
>> +		 * may not be the oldest one with the plugged flag still set.
>> +		 * unplug_oldest_pwq() will still do the right thing to allow
>> +		 * only one unplugged pwq in the workqueue.
>> +		 */
>> +		if ((ctx->wq->flags & __WQ_ORDERED) &&
>> +		     ctx->dfl_pwq && !ctx->dfl_pwq->refcnt)
>> +			unplug_oldest_pwq(ctx->wq, ctx->dfl_pwq);
>> +		rcu_read_unlock();
> But why do we need this? Isn't all that needed to call unplug_oldest during
> workqueue initialization and chaining unplugging from pwq release from there
> on?

Yes, it is possible to just do unplug_oldest_pwq() in 
pwq_release_workfn() and don't do it in apply_wqattrs_cleanup(). As said 
above, I just want to reduce the latency when the old pwq to be retired 
is idle. I can certainly update the patch to just do it in 
pwq_release_workfn() if you don't that it is necessary to do that too in 
apply_wqattrs_cleanup(). That will eliminate the need for the extra 
arugment and simplify unplug_oldest_pwq().

Cheers,
Longman