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

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

Hi,

Is this a human review, or AI review, or combined?

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.

> 
>> +		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.

>> +		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.

> 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.

> 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.

> affinity sysfs files can still undo the forced affinity.

I am opting letting the admins shoot themselves in the foot.

> 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.

> With the rebase and fixes, please add:
> 
> Reviewed-by: Bradley Morgan <include@grrlz.net> # kernel/
> 
> Thanks for the patch!

Thank you for reading through it!

Regards,

Tvrtko