Re: [RFC v6 2/3] workqueue: Add support for real-time workers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Zero-initialization in alloc_workqueue_attrs_noprof() promotes background workqueues to RT
- [Medium] WQ_RT workers are created as standard CFS tasks due to incorrect RT_PRIORITY
- [Medium] Sysfs cpumask attribute allows modifying strict CPU affinity of WQ_RT workqueues
--

commit c69a2e22b185b64246092a869b9af42c347b149b
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

    workqueue: Add support for real-time workers
    
    For use cases such as the DRM scheduler submitting work to the GPU on
    behalf of low latency userspace applications, where latter have sufficient
    privileges to have had successfully obtained realtime Vulkan global
    priority, competing with random background CPU load can create large
    latency spikes which gets in the way of a smooth user experience.

> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index c8a36423cb346..d37b8bee37c6f 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -147,9 +147,9 @@ enum wq_affn_scope {
>   */
>  struct workqueue_attrs {
>  	/**
> -	 * @nice: nice level
> +	 * @prio: priority encoded analoguous to task_struct->prio.
>  	 */
> -	int nice;
> +	int prio;

[Severity: High]
Does introducing this prio field cause a regression where
alloc_workqueue_attrs_noprof() implicitly promotes background workqueues to
real-time?

When alloc_workqueue_attrs_noprof() allocates the attribute structure with
kzalloc_obj(), the newly introduced prio field is zero-initialized rather than
being explicitly set to DEFAULT_PRIO (120).

In the kernel priority scale, 0 is equivalent to MAX_DL_PRIO. When these
attributes are applied to a pool, rt_prio(0) evaluates to true, causing
create_worker() to call sched_set_fifo_low(worker->task).

Subsystems that dynamically allocate attributes without manually assigning a
priority (like padata) may inadvertently apply real-time priority to parallel
CPU-intensive workloads.

[ ... ]

> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 71423d9b7cf96..93a73ebefecd3 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -126,7 +127,8 @@ enum wq_internal_consts {
>  	 * all cpus.  Give MIN_NICE.
>  	 */
>  	RESCUER_NICE_LEVEL	= MIN_NICE,
> -	HIGHPRI_NICE_LEVEL	= MIN_NICE,
> +	HIGHPRI_PRIORITY	= NICE_TO_PRIO(MIN_NICE),
> +	RT_PRIORITY		= MAX_PRIO,

[Severity: Medium]
Will this assignment cause a regression where WQ_RT workers are created as
standard CFS tasks instead of real-time tasks?

When creating a WQ_RT worker, its priority is set to RT_PRIORITY. However, since
RT_PRIORITY is defined as MAX_PRIO (140), passing it to rt_prio(140) evaluates
to false, as 140 is not < 100.

[ ... ]

> @@ -3018,7 +3021,12 @@ static struct worker *create_worker(struct worker_pool *pool)
>  			goto fail;
>  		}
>  
> -		set_user_nice(worker->task, pool->attrs->nice);
> +		if (rt_prio(pool->attrs->prio))
> +			sched_set_fifo_low(worker->task);
> +		else
> +			set_user_nice(worker->task,
> +				      PRIO_TO_NICE(pool->attrs->prio));
> +

[Severity: Medium]
When execution falls back here to set_user_nice() for WQ_RT workers due to
the RT_PRIORITY value, won't it pass PRIO_TO_NICE(140) which equals 20?

Since 20 exceeds MAX_NICE (19), the nice change is silently ignored and the
real-time workqueues fail to receive the intended real-time scheduling policy.

>  		kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
>  	}

[ ... ]

> @@ -7740,6 +7771,10 @@ static ssize_t affinity_scope_store(struct device *dev,
>  	struct workqueue_attrs *attrs;
>  	int affn, ret = -ENOMEM;
>  
> +	/* Do not allow affinity changes for RT workers. */
> +	if (wq->flags & WQ_RT)
> +		return -EINVAL;
> +
>  	affn = parse_affn_scope(buf);
>  	if (affn < 0)
>  		return affn;

[Severity: Medium]
Should unbound_cpumask_store() also restrict affinity changes for RT workers
to prevent a regression?

While affinity_scope_store() and affinity_strict_store() explicitly block
modifications for WQ_RT workqueues by returning -EINVAL here, the adjacent
unbound_cpumask_store() lacks this check.

This omission allows users with write permissions to the sysfs node to break
the strict CPU affinity binding of WQ_RT workqueues, violating the documented
invariant.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001160711.59888-1-tvrtko.ursulin@igalia.com?part=2