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

[Bradley Morgan]

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 <include@xxxxxxxxx> # kernel/

Thanks for the patch!