On July 17, 2026 12:51:06 PM GMT+01:00, Tvrtko Ursulin
<[email protected]> 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 <[email protected]> # kernel/
>>
>> Thanks for the patch!
>
>Thank you for reading through it!
>
>Regards,
>
>Tvrtko
>
>
>
Thanks!
