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..
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;
> + 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);
> + 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;
+ }
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?
Also the rescuer is not FIFO, so WQ_MEM_RECLAIM plus WQ_RTPRI loses
the latency claim exactly under memory pressure.
Nits: the "R" suffix branch is dead code, RT is unbound only. The
affinity sysfs files can still undo the forced affinity.
Id prefer NUM_WQ_PRIO over NR_STD_WORKER_POOLS + 1. workqueue.rst needs
WQ_RTPRI.
With the rebase and fixes, please add:
Reviewed-by: Bradley Morgan <[email protected]> # kernel/
Thanks for the patch!
