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

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

On 02/10/2026 20:30, Tejun Heo wrote:
> Hello,
> 
> The following is a Claude-generated review.
> 
> On Thu, Oct 01, 2026 at 07:48:48PM +0100, Tvrtko Ursulin wrote:
>> 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.
> 
> Can you add the compositor / DRM master usage model from the v5 discussion
> here? The cover letter also still says CAP_SYS_NICE is required while
> group_priority_permit() accepts DRM master too.

Yeah forgot about this, will add.

> 
>> +	HIGHPRI_PRIORITY	= NICE_TO_PRIO(MIN_NICE),
>> +	RT_PRIORITY		= MAX_RT_PRIO - 1,
> 
> MAX_RT_PRIO - 1 is what rt_priority 0 would map to, while
> sched_set_fifo_low() gives the workers prio 98. Nothing depends on it today,
> but maybe MAX_RT_PRIO - 2 with a comment tying it to sched_set_fifo_low(),
> or soften the kerneldoc which says the encoding matches task_struct->prio?
> Also, s/analoguous/analogous/ there.

Done.

>> +	if (wq->flags & WQ_RT) {
>> +		attrs->prio = RT_PRIORITY;
>> +		/*
>> +		 * RT workqueues have strict CPU affinity for low
>> +		 * latency execution.
>> +		 */
>> +		attrs->affn_scope = WQ_AFFN_CPU;
>> +		attrs->affn_strict = true;
> 
> apply_workqueue_attrs() is unrestricted, so a caller can hand a WQ_RT wq the
> attrs from alloc_workqueue_attrs() and turn it into a normal non-strict wq
> while the flag stays set. Maybe apply_wqattrs_prepare() should force prio
> and affinity for WQ_RT instead of restricting only the sysfs side?

I am not entirely sure about this one. It could be done in 
apply_wqattrs_prepare(), but it would end up redundant on the sysfs 
path. Because currently there are two paths:

1)
alloc_wq_std_attrs
  -> apply_workqueue_attrs_locked

This is the hunk this review comment is about. Affinity and scope is set 
from wq->flags.

2)
wq_sysfs_prep_attrs
  -> apply_workqueue_attrs_locked

Here the attributes have had the affinity and scope copied from the 
respective workqueue (wq_sysfs_prep_attrs -> copy_workqueue_attrs).

If we moved the scope and affinity setting from alloc_wq_std_attrs to 
apply_wqattrs_prepare it would work, but on this path it would be redundant.

Am I missing something?

>> +	if (flags & WQ_RT) {
>> +		if (WARN_ON_ONCE((flags & (WQ_HIGHPRI | WQ_UNBOUND)) !=
>> +			         WQ_UNBOUND))
>> +			return NULL;
>> +	}
> 
> alloc_ordered_workqueue() with WQ_RT passes this and ends up with a single
> pool spanning all CPUs because ordered wqs use dfl_pwq everywhere, while the
> doc and sysfs say strict per-CPU. Should __WQ_ORDERED be rejected too? The
> nested ifs can also be a single condition.

Not sure - Tejun what is your guidance here? Is this purely an 
implementation challenge because conceptually it should work, no?

P.S. The nested ifs could be a single condition but I wanted to mimic 
the existing style so it looks nicer:

	if (flags & WQ_BH) {
		if (WARN_ON_ONCE(flags & ~__WQ_BH_ALLOWS))
			return NULL;
		if (WARN_ON_ONCE(max_active))
			return NULL;
	}

	if (flags & WQ_RT) {
		if (WARN_ON_ONCE((flags & (WQ_HIGHPRI | WQ_UNBOUND)) !=
			         WQ_UNBOUND))
			return NULL;
	}

I can change if desired.

> 
>> -		pr_cont(" nice=%d", pool->attrs->nice);
>> +		pr_cont(" nice=%d", PRIO_TO_NICE(pool->attrs->prio));
> 
> This prints nice=-21 for RT pools. Can you show "rt" here like nice_show()?

Done.

>> +	/* Do not allow cpumask changes for RT workers. */
>> +	if (wq->flags & WQ_RT)
>> +		return -EINVAL;
> 
> The three stores check WQ_RT and return -EINVAL while nice goes read-only
> through is_visible below. Can all four go through
> wq_sysfs_unbound_group_visible() returning 0444 for WQ_RT? That drops the
> three store checks. Note that the global cpumask still applies to WQ_RT wqs
> through workqueue_apply_unbound_cpumask(), so the comment overstates a bit.
> The interface comment at the top of the sysfs section also still says nice
> is RW int.

Could do that if it would be acceptable for root to be still able to 
write to them - Tejun?

> 
>> +	/* Do not allow priority changes for RT workers. */
>> +	if ((wq->flags & WQ_RT) && !strcmp(attr->name, "nice"))
>> +		return 0444;
> 
> attr == &dev_attr_nice.attr would avoid the strcmp.

Ah.. :)

>> +    prio = pool.attrs.prio.value_()
>> +    if prio == rt_prio:
>> +        prio = 'rt'
>> +    print(f'pool[{pi:0{max_pool_id_len}}] flags=0x{pool.flags.value_():02x} ref={pool.refcnt.value_():{max_ref_len}} prio={prio:3} ', end='')
> 
> Can we keep printing nice for non-RT pools? prio=120 is the internal
> encoding, sysfs and the pool dumps print nice, and the example output in
> workqueue.rst would go stale. prog['RT_PRIORITY'] would match the rest of
> the file too.

We probably could if we wanted to. Tejun?

Complication would be having to hardcode some priority conversion 
helpers into the script.

> One more thing which isn't in the diff. The rescuer of a WQ_RT |
> WQ_MEM_RECLAIM wq, which is what panthor-drm-rt is, still runs at nice -20,
> so under memory pressure the RT wq's work items run as CFS. Should
> rescuer_thread() use sched_set_fifo_low() for WQ_RT?

I did not think we should care much about latency when under severe 
memory pressure. It should be enough to guarantee forward progress and 
the latency restores back to normal once memory pressure stabilises.

Regards,

Tvrtko