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
