Re: [RFC v2 1/2] workqueue: Add support for real-time workers

[Bradley Morgan]

On July 17, 2026 12:51:06 PM GMT+01:00, Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxx> wrote:
>
>Hi,
>
>Is this a human review, or AI review, or combined?

Human :)


>On 17/07/2026 12:25, Bradley Morgan wrote:
>> Hi Tvrtko,
>> 
>> Applies to master. On linux-next one hunk rejects, the WQ_RTPRI block
>> in __alloc_workqueue(), the rest is clean. Rebase v3 onto the wq tree,
>> please..
>
>Fixed in v3 already.
>
>> 
>> Things to fix:
>> 
>>> +fail_ida:
>>> +	if (pool->attrs->prio == WQ_PRIO_RT)
>>> +		atomic_dec(&total_rtpri_workers);
>>>   fail:
>>>   	ida_free(&pool->worker_ida, id);
>>>   	kfree(worker);
>> 
>> Labels swapped. The ida_alloc() failure falls into fail:, so ida_free()
>> gets a negative id and kfree() an uninitialized pointer, and the
>> later failure paths leak the counter. The fix is something like,
>> 
>> 	id = ida_alloc(&pool->worker_ida, GFP_KERNEL);
>> 	if (id < 0)
>> 		goto fail_dec;
>> 	...
>> fail:
>> 	ida_free(&pool->worker_ida, id);
>> 	kfree(worker);
>> fail_dec:
>> 	if (pool->attrs->prio == WQ_PRIO_RT)
>> 		atomic_dec(&total_rtpri_workers);
>> 	return NULL;
>
>Already fixed locally after Sashiko flagged the onion unwind fail.

Thanks.

>> 
>>> +		if (worker->pool->attrs->prio == WQ_PRIO_RT)
>>> +			atomic_dec(&total_rtpri_workers);
>> 
>> worker->pool is NULL here, the WORKER_DIE path clears it before
>> kthread_stop_put() returns, so every cull oopses.
>> 
>> Flag the worker at creation, a bool rtpri in struct worker:
>> 
>> 	worker->rtpri = pool->attrs->prio == WQ_PRIO_RT;
>> 
>> and in reap_dying_workers():
>> 
>> -		if (worker->pool->attrs->prio == WQ_PRIO_RT)
>> +		if (worker->rtpri)
>>   			atomic_dec(&total_rtpri_workers);
>
>Ditto but differently.

Thanks.

>>> +		wq->unbound_attrs->affn_scope = WQ_AFFN_CPU;
>>> +		wq->unbound_attrs->affn_strict = true;
>> 
>> Its a dead store, apply_wqattrs_commit() overwrites both, so the
>headline
>> v2
>> change never engages. Drop these two lines and mark the RT entries in
>> the workqueue_init_early() attrs loops instead, for both
>> unbound_std_wq_attrs and ordered_wq_attrs:
>> 
>> 		attrs->prio = std_prio[i];
>> 		attrs->nice = std_nice[i];
>> 
>> +		if (attrs->prio == WQ_PRIO_RT) {
>> +			attrs->affn_scope = WQ_AFFN_CPU;
>> +			attrs->affn_strict = true;
>> +		}
>
>Already done locally exactly like this.

Thanks.


>> Questions: the changelog says two workers per workqueue, but the code
>clamps max_active, which caps work items, not threads. At the global cap
>create_worker() fails silently and maybe_create_worker() retries forever,
>so the pool stalls; with strict pods the last pod on an N CPU
>> box may never get a worker. pr_warn() or fall back to a normal worker?
>
>Yeah it's a design open. I will probably just drop the global limits if
>Tejun agrees.

Ok.

>> Also the rescuer is not FIFO, so WQ_MEM_RECLAIM plus WQ_RTPRI loses
>> the latency claim exactly under memory pressure.
>
>Sashiko commented on this already. But I don't think latency is super
>relevant under memory pressure.
>
>> Nits: the "R" suffix branch is dead code, RT is unbound only. The
>
>Curiosly lkml Sashiko missed this but I am aware of it. It is leftover
>from v1. And it's not a branch but an extra array element.
>

okay.

>> affinity sysfs files can still undo the forced affinity.
>
>I am opting letting the admins shoot themselves in the foot.

Heh. Fair enough 

>> Id prefer NUM_WQ_PRIO over NR_STD_WORKER_POOLS + 1. workqueue.rst needs
>> WQ_RTPRI.
>
>I was on the fence. Either way is not ideal.
>
>RST noted.

Yeah. Agreed.

>> With the rebase and fixes, please add:
>> 
>> Reviewed-by: Bradley Morgan <include@xxxxxxxxx> # kernel/
>> 
>> Thanks for the patch!
>
>Thank you for reading through it!
>
>Regards,
>
>Tvrtko
>
>
>

Thanks!