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 <[email protected]> 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/[email protected]?part=2
