[RFC 0/2] Realtime workqueues and panthor realtime submission

[RFC 0/2] Realtime workqueues and panthor realtime submission

[Tvrtko Ursulin]

This is a continuation of the previous discussion which was here:
https://lore.kernel.org/dri-devel/20260702143745.79293-1-tvrtko.ursulin@igalia.com/

Work is now converted to a much simpler approach by adding real-time scheduling
workqueues based on Tejun's feedback

To re-cap, when an userspace thread submits GPU work, due how the DRM scheduler
uses workqueues to feed the GPU, and regardless of the GPU rendering context
priority, or the CPU scheduling priority of the userspace thread itself, the
use of workqueues adds (a lot of) latency to the submit path.

When CPU is busy with enough backround load this translates to severe latency
spikes measured as time between userspace submitting work and GPU actually being
given that work to execute.

With the panthor workqueue upgraded to use WQ_HIGHPRI and varying the CPU
priority of the submit thread, the test program from
https://gitlab.freedesktop.org/panfrost/linux/-/work_items/49 reproduces these
kind of latencies:

         .    N    RT
    M   27   28    32
  95%  163  246   809
  98%  924  991  1882

Legend:

   M = Median submit latency in us
 95% = Percentile latency in us

   . = Userspace submit thread SCHED_OTHER
   N = -||= nice -1
  RT = -||- FIFO 1 

Upgrading the panthor workqueues so that the realtime GPU priority queue uses
WQ_RTPRI, submit latency becomes completely controlled with the median of 14us
and 95 and 98-th percentiles at 23us and 25us respectively.

Important to note is that VK_QUEUE_GLOBAL_PRIORITY_REALTIME already required
the userspace to have CAP_SYS_NICE, meaning access to real-time workqueues is
effectively also guared behind this capability. The implementation of WQ_RTPRI
itself also adds some limites on the maximum number of worker threads in order
to prevent scheduling starvation.

Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Chia-I Wu <olv@google.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Tejun Heo <tj@kernel.org>

Tvrtko Ursulin (2):
  workqueue: Add support for real-time workers
  drm/panthor: Create per queue priority workqueues

 drivers/gpu/drm/panthor/panthor_sched.c |  38 +++++-
 include/linux/workqueue.h               |  22 +++-
 kernel/workqueue.c                      | 154 ++++++++++++++++++------
 3 files changed, 164 insertions(+), 50 deletions(-)

-- 
2.54.0

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

[Tvrtko Ursulin]

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 successfuly 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.

For these situations the existing WQ_HIPRI does not bring a noticeable
improvemet and a stronger hint is needed.

Lets add WQ_RTPRI which creates workers with a SCHED_FIFO scheduling class
to improve this.

We use a mininum priority level since we only care about winning the
contest against normal background CPU load. We also limit the number of
instantiated threads, both per workqueue to a maximum of two, and system-
wide to a maximum of either two or one less than the number of online
CPUs. The idea of that is to prevent system wide starvation cause by
potentially misbehaving work items.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Chia-I Wu <olv@google.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Tejun Heo <tj@kernel.org>
---
 include/linux/workqueue.h |  22 ++++--
 kernel/workqueue.c        | 154 ++++++++++++++++++++++++++++----------
 2 files changed, 131 insertions(+), 45 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 6177624539b3..bc313b9705cb 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -140,6 +140,12 @@ enum wq_affn_scope {
 	WQ_AFFN_NR_TYPES,
 };
 
+enum wq_priority {
+	WQ_PRIO_NORMAL = 0,
+	WQ_PRIO_HIGH = 1,
+	WQ_PRIO_RT = 2
+};
+
 /**
  * struct workqueue_attrs - A struct for workqueue attributes.
  *
@@ -147,7 +153,12 @@ enum wq_affn_scope {
  */
 struct workqueue_attrs {
 	/**
-	 * @nice: nice level
+	 * @prio: priority level
+	 */
+	enum wq_priority prio;
+
+	/**
+	 * @nice: nice level for WQ_PRIO_HIGH
 	 */
 	int nice;
 
@@ -374,8 +385,9 @@ enum wq_flags {
 	WQ_FREEZABLE		= 1 << 2, /* freeze during suspend */
 	WQ_MEM_RECLAIM		= 1 << 3, /* may be used for memory reclaim */
 	WQ_HIGHPRI		= 1 << 4, /* high priority */
-	WQ_CPU_INTENSIVE	= 1 << 5, /* cpu intensive workqueue */
-	WQ_SYSFS		= 1 << 6, /* visible in sysfs, see workqueue_sysfs_register() */
+	WQ_RTPRI		= 1 << 5, /* real-time priority */
+	WQ_CPU_INTENSIVE	= 1 << 6, /* cpu intensive workqueue */
+	WQ_SYSFS		= 1 << 7, /* visible in sysfs, see workqueue_sysfs_register() */
 
 	/*
 	 * Per-cpu workqueues are generally preferred because they tend to
@@ -402,8 +414,8 @@ enum wq_flags {
 	 *
 	 * http://thread.gmane.org/gmane.linux.kernel/1480396
 	 */
-	WQ_POWER_EFFICIENT	= 1 << 7,
-	WQ_PERCPU		= 1 << 8, /* bound to a specific cpu */
+	WQ_POWER_EFFICIENT	= 1 << 8,
+	WQ_PERCPU		= 1 << 9, /* bound to a specific cpu */
 
 	__WQ_DESTROYING		= 1 << 15, /* internal: workqueue is destroying */
 	__WQ_DRAINING		= 1 << 16, /* internal: workqueue is draining */
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 33b721a9af02..0473c009690f 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -80,9 +80,10 @@ enum worker_pool_flags {
 	 * BH pool is per-CPU and always DISASSOCIATED.
 	 */
 	POOL_BH			= 1 << 0,	/* is a BH pool */
-	POOL_MANAGER_ACTIVE	= 1 << 1,	/* being managed */
-	POOL_DISASSOCIATED	= 1 << 2,	/* cpu can't serve workers */
-	POOL_BH_DRAINING	= 1 << 3,	/* draining after CPU offline */
+	POOL_RT			= 1 << 1,	/* is a RT pool */
+	POOL_MANAGER_ACTIVE	= 1 << 2,	/* being managed */
+	POOL_DISASSOCIATED	= 1 << 3,	/* cpu can't serve workers */
+	POOL_BH_DRAINING	= 1 << 4,	/* draining after CPU offline */
 };
 
 enum worker_flags {
@@ -104,7 +105,8 @@ enum work_cancel_flags {
 };
 
 enum wq_internal_consts {
-	NR_STD_WORKER_POOLS	= 2,		/* # standard pools per cpu */
+	NR_STD_WORKER_POOLS	= 3,		/* # standard pools per cpu */
+	NR_BH_WORKER_POOLS	= 2,		/* # bottom-half pools per cpu */
 
 	UNBOUND_POOL_HASH_ORDER	= 6,		/* hashed by pool->attrs */
 	BUSY_WORKER_HASH_ORDER	= 6,		/* 64 pointers */
@@ -495,10 +497,10 @@ static bool wq_debug_force_rr_cpu = false;
 module_param_named(debug_force_rr_cpu, wq_debug_force_rr_cpu, bool, 0644);
 
 /* to raise softirq for the BH worker pools on other CPUs */
-static DEFINE_PER_CPU_SHARED_ALIGNED(struct irq_work [NR_STD_WORKER_POOLS], bh_pool_irq_works);
+static DEFINE_PER_CPU_SHARED_ALIGNED(struct irq_work [NR_BH_WORKER_POOLS], bh_pool_irq_works);
 
 /* the BH worker pools */
-static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_STD_WORKER_POOLS], bh_worker_pools);
+static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_BH_WORKER_POOLS], bh_worker_pools);
 
 /* the per-cpu worker pools */
 static DEFINE_PER_CPU_SHARED_ALIGNED(struct worker_pool [NR_STD_WORKER_POOLS], cpu_worker_pools);
@@ -546,6 +548,8 @@ EXPORT_SYMBOL_GPL(system_bh_highpri_wq);
 struct workqueue_struct *system_dfl_long_wq __ro_after_init;
 EXPORT_SYMBOL_GPL(system_dfl_long_wq);
 
+static atomic_t total_rtpri_workers = ATOMIC_INIT(0);
+
 static int worker_thread(void *__worker);
 static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
 static void show_pwq(struct pool_workqueue *pwq);
@@ -561,7 +565,7 @@ static void show_one_worker_pool(struct worker_pool *pool);
 
 #define for_each_bh_worker_pool(pool, cpu)				\
 	for ((pool) = &per_cpu(bh_worker_pools, cpu)[0];		\
-	     (pool) < &per_cpu(bh_worker_pools, cpu)[NR_STD_WORKER_POOLS]; \
+	     (pool) < &per_cpu(bh_worker_pools, cpu)[NR_BH_WORKER_POOLS]; \
 	     (pool)++)
 
 #define for_each_cpu_worker_pool(pool, cpu)				\
@@ -1236,9 +1240,7 @@ static bool assign_work(struct work_struct *work, struct worker *worker,
 
 static struct irq_work *bh_pool_irq_work(struct worker_pool *pool)
 {
-	int high = pool->attrs->nice == HIGHPRI_NICE_LEVEL ? 1 : 0;
-
-	return &per_cpu(bh_pool_irq_works, pool->cpu)[high];
+	return &per_cpu(bh_pool_irq_works, pool->cpu)[pool->attrs->prio];
 }
 
 static void kick_bh_pool(struct worker_pool *pool)
@@ -1251,7 +1253,7 @@ static void kick_bh_pool(struct worker_pool *pool)
 		return;
 	}
 #endif
-	if (pool->attrs->nice == HIGHPRI_NICE_LEVEL)
+	if (pool->attrs->prio >= WQ_PRIO_HIGH)
 		raise_softirq_irqoff(HI_SOFTIRQ);
 	else
 		raise_softirq_irqoff(TASKLET_SOFTIRQ);
@@ -2801,13 +2803,16 @@ static int format_worker_id(char *buf, size_t size, struct worker *worker,
 				 worker->rescue_wq->name);
 
 	if (pool) {
-		if (pool->cpu >= 0)
+		if (pool->cpu >= 0) {
+			const char *suffix[NR_STD_WORKER_POOLS] = { "", "H", "R" };
+
 			return scnprintf(buf, size, "kworker/%d:%d%s",
 					 pool->cpu, worker->id,
-					 pool->attrs->nice < 0  ? "H" : "");
-		else
+					 suffix[pool->attrs->prio]);
+		} else {
 			return scnprintf(buf, size, "kworker/u%d:%d",
 					 pool->id, worker->id);
+		}
 	} else {
 		return scnprintf(buf, size, "kworker/dying");
 	}
@@ -2863,7 +2868,11 @@ static struct worker *create_worker(struct worker_pool *pool)
 			goto fail;
 		}
 
-		set_user_nice(worker->task, pool->attrs->nice);
+		if (pool->attrs->prio == WQ_PRIO_RT)
+			sched_set_fifo_low(worker->task);
+		else if (pool->attrs->prio == WQ_PRIO_HIGH)
+			set_user_nice(worker->task, pool->attrs->nice);
+
 		kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
 	}
 
@@ -2876,6 +2885,9 @@ static struct worker *create_worker(struct worker_pool *pool)
 	worker->pool->nr_workers++;
 	worker_enter_idle(worker);
 
+	if (pool->flags & POOL_RT)
+		atomic_inc(&total_rtpri_workers);
+
 	/*
 	 * @worker is waiting on a completion in kthread() and will trigger hung
 	 * check if not woken up soon. As kick_pool() is noop if @pool is empty,
@@ -2909,6 +2921,8 @@ static void reap_dying_workers(struct list_head *cull_list)
 	list_for_each_entry_safe(worker, tmp, cull_list, entry) {
 		list_del_init(&worker->entry);
 		kthread_stop_put(worker->task);
+		if (worker->flags & WQ_RTPRI)
+			atomic_dec(&total_rtpri_workers);
 		kfree(worker);
 	}
 }
@@ -3110,6 +3124,19 @@ __acquires(&pool->lock)
 	mod_timer(&pool->mayday_timer, jiffies + MAYDAY_INITIAL_TIMEOUT);
 
 	while (true) {
+		if (pool->flags & POOL_RT) {
+			unsigned int max = num_online_cpus();
+
+			if (max > 2)
+				max = max - 1;
+			else
+				max = 2;
+
+			/* Global cap on the number of RT workers */
+			if (atomic_read(&total_rtpri_workers) >= max)
+				break;
+		}
+
 		if (create_worker(pool) || !need_to_create_worker(pool))
 			break;
 
@@ -3765,7 +3792,7 @@ static void drain_dead_softirq_workfn(struct work_struct *work)
 	 * don't hog this CPU's BH.
 	 */
 	if (repeat) {
-		if (pool->attrs->nice == HIGHPRI_NICE_LEVEL)
+		if (pool->attrs->prio >= WQ_PRIO_HIGH)
 			queue_work(system_bh_highpri_wq, work);
 		else
 			queue_work(system_bh_wq, work);
@@ -3786,7 +3813,7 @@ void workqueue_softirq_dead(unsigned int cpu)
 {
 	int i;
 
-	for (i = 0; i < NR_STD_WORKER_POOLS; i++) {
+	for (i = 0; i < NR_BH_WORKER_POOLS; i++) {
 		struct worker_pool *pool = &per_cpu(bh_worker_pools, cpu)[i];
 		struct wq_drain_dead_softirq_work dead_work;
 
@@ -3797,7 +3824,7 @@ void workqueue_softirq_dead(unsigned int cpu)
 		dead_work.pool = pool;
 		init_completion(&dead_work.done);
 
-		if (pool->attrs->nice == HIGHPRI_NICE_LEVEL)
+		if (pool->attrs->prio >= WQ_PRIO_HIGH)
 			queue_work(system_bh_highpri_wq, &dead_work.work);
 		else
 			queue_work(system_bh_wq, &dead_work.work);
@@ -4772,6 +4799,7 @@ struct workqueue_attrs *alloc_workqueue_attrs_noprof(void)
 static void copy_workqueue_attrs(struct workqueue_attrs *to,
 				 const struct workqueue_attrs *from)
 {
+	to->prio = from->prio;
 	to->nice = from->nice;
 	cpumask_copy(to->cpumask, from->cpumask);
 	cpumask_copy(to->__pod_cpumask, from->__pod_cpumask);
@@ -4803,6 +4831,7 @@ static u32 wqattrs_hash(const struct workqueue_attrs *attrs)
 {
 	u32 hash = 0;
 
+	hash = jhash_1word(attrs->prio, hash);
 	hash = jhash_1word(attrs->nice, hash);
 	hash = jhash_1word(attrs->affn_strict, hash);
 	hash = jhash(cpumask_bits(attrs->__pod_cpumask),
@@ -4817,6 +4846,8 @@ static u32 wqattrs_hash(const struct workqueue_attrs *attrs)
 static bool wqattrs_equal(const struct workqueue_attrs *a,
 			  const struct workqueue_attrs *b)
 {
+	if (a->prio != b->prio)
+		return false;
 	if (a->nice != b->nice)
 		return false;
 	if (a->affn_strict != b->affn_strict)
@@ -5603,11 +5634,17 @@ static void unbound_wq_update_pwq(struct workqueue_struct *wq, int cpu)
 
 static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 {
-	bool highpri = wq->flags & WQ_HIGHPRI;
-	int cpu, ret;
+	int prio, cpu, ret;
 
 	lockdep_assert_held(&wq_pool_mutex);
 
+	if (wq->flags & WQ_RTPRI)
+		prio = WQ_PRIO_RT;
+	else if (wq->flags & WQ_HIGHPRI)
+		prio = WQ_PRIO_HIGH;
+	else
+		prio = WQ_PRIO_NORMAL;
+
 	wq->cpu_pwq = alloc_percpu(struct pool_workqueue *);
 	if (!wq->cpu_pwq)
 		goto enomem;
@@ -5624,7 +5661,7 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 			struct pool_workqueue **pwq_p;
 			struct worker_pool *pool;
 
-			pool = &(per_cpu_ptr(pools, cpu)[highpri]);
+			pool = &(per_cpu_ptr(pools, cpu)[prio]);
 			pwq_p = per_cpu_ptr(wq->cpu_pwq, cpu);
 
 			*pwq_p = kmem_cache_alloc_node(pwq_cache, GFP_KERNEL,
@@ -5644,14 +5681,14 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 	if (wq->flags & __WQ_ORDERED) {
 		struct pool_workqueue *dfl_pwq;
 
-		ret = apply_workqueue_attrs_locked(wq, ordered_wq_attrs[highpri]);
+		ret = apply_workqueue_attrs_locked(wq, ordered_wq_attrs[prio]);
 		/* there should only be single pwq for ordering guarantee */
 		dfl_pwq = rcu_access_pointer(wq->dfl_pwq);
 		WARN(!ret && (wq->pwqs.next != &dfl_pwq->pwqs_node ||
 			      wq->pwqs.prev != &dfl_pwq->pwqs_node),
 		     "ordering guarantee broken for workqueue %s\n", wq->name);
 	} else {
-		ret = apply_workqueue_attrs_locked(wq, unbound_std_wq_attrs[highpri]);
+		ret = apply_workqueue_attrs_locked(wq, unbound_std_wq_attrs[prio]);
 	}
 
 	if (ret)
@@ -5816,6 +5853,11 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
 			return NULL;
 	}
 
+	if (flags & WQ_RTPRI) {
+		if (WARN_ON_ONCE(flags & WQ_HIGHPRI))
+			return NULL;
+	}
+
 	/* see the comment above the definition of WQ_POWER_EFFICIENT */
 	if ((flags & WQ_POWER_EFFICIENT) && wq_power_efficient)
 		flags |= WQ_UNBOUND;
@@ -5842,7 +5884,12 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
 		pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n",
 			     wq->name);
 
-	if (flags & WQ_BH) {
+	if (flags & WQ_RTPRI) {
+		/*
+		 * RT workqueues are limited to max half of the online CPUs.
+		 */
+		max_active = min_t(int, max_active, num_online_cpus() / 2);
+	} else if (flags & WQ_BH) {
 		/*
 		 * BH workqueues always share a single execution context per CPU
 		 * and don't impose any max_active limit.
@@ -6344,17 +6391,26 @@ void print_worker_info(const char *log_lvl, struct task_struct *task)
 	}
 }
 
+static void pr_cont_bh_suffix(enum wq_priority prio)
+{
+	if (prio == WQ_PRIO_RT)
+		pr_cont("bh-rt");
+	else if (prio == WQ_PRIO_HIGH)
+		pr_cont("bh-hi");
+	else
+		pr_cont("bh");
+}
+
 static void pr_cont_pool_info(struct worker_pool *pool)
 {
 	pr_cont(" cpus=%*pbl", nr_cpumask_bits, pool->attrs->cpumask);
 	if (pool->node != NUMA_NO_NODE)
 		pr_cont(" node=%d", pool->node);
-	pr_cont(" flags=0x%x", pool->flags);
+	pr_cont(" flags=0x%x ", pool->flags);
 	if (pool->flags & POOL_BH)
-		pr_cont(" bh%s",
-			pool->attrs->nice == HIGHPRI_NICE_LEVEL ? "-hi" : "");
+		pr_cont_bh_suffix(pool->attrs->prio);
 	else
-		pr_cont(" nice=%d", pool->attrs->nice);
+		pr_cont("prio=%d", pool->attrs->prio);
 }
 
 static void pr_cont_worker_id(struct worker *worker)
@@ -6362,8 +6418,7 @@ static void pr_cont_worker_id(struct worker *worker)
 	struct worker_pool *pool = worker->pool;
 
 	if (pool->flags & POOL_BH)
-		pr_cont("bh%s",
-			pool->attrs->nice == HIGHPRI_NICE_LEVEL ? "-hi" : "");
+		pr_cont_bh_suffix(pool->attrs->prio);
 	else
 		pr_cont("%d%s", task_pid_nr(worker->task),
 			worker->rescue_wq ? "(RESCUER)" : "");
@@ -7284,10 +7339,14 @@ static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
-	int written;
+	int written, nice;
 
 	mutex_lock(&wq->mutex);
-	written = scnprintf(buf, PAGE_SIZE, "%d\n", wq->unbound_attrs->nice);
+	if (wq->unbound_attrs->prio == WQ_PRIO_RT)
+		nice = INT_MIN;
+	else
+		nice = wq->unbound_attrs->nice;
+	written = scnprintf(buf, PAGE_SIZE, "%d\n", nice);
 	mutex_unlock(&wq->mutex);
 
 	return written;
@@ -7313,13 +7372,20 @@ static ssize_t wq_nice_store(struct device *dev, struct device_attribute *attr,
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 	struct workqueue_attrs *attrs;
-	int ret = -ENOMEM;
+	int ret;
 
 	apply_wqattrs_lock();
 
 	attrs = wq_sysfs_prep_attrs(wq);
-	if (!attrs)
+	if (!attrs) {
+		ret = -ENOMEM;
 		goto out_unlock;
+	}
+
+	if (attrs->prio == WQ_PRIO_RT) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
 
 	if (sscanf(buf, "%d", &attrs->nice) == 1 &&
 	    attrs->nice >= MIN_NICE && attrs->nice <= MAX_NICE)
@@ -7927,12 +7993,13 @@ static void __init restrict_unbound_cpumask(const char *name, const struct cpuma
 	cpumask_and(wq_unbound_cpumask, wq_unbound_cpumask, mask);
 }
 
-static void __init init_cpu_worker_pool(struct worker_pool *pool, int cpu, int nice)
+static void __init init_cpu_worker_pool(struct worker_pool *pool, int cpu, enum wq_priority prio, int nice)
 {
 	BUG_ON(init_worker_pool(pool));
 	pool->cpu = cpu;
 	cpumask_copy(pool->attrs->cpumask, cpumask_of(cpu));
 	cpumask_copy(pool->attrs->__pod_cpumask, cpumask_of(cpu));
+	pool->attrs->prio = prio;
 	pool->attrs->nice = nice;
 	pool->attrs->affn_strict = true;
 	pool->node = cpu_to_node(cpu);
@@ -7956,8 +8023,9 @@ static void __init init_cpu_worker_pool(struct worker_pool *pool, int cpu, int n
 void __init workqueue_init_early(void)
 {
 	struct wq_pod_type *pt = &wq_pod_types[WQ_AFFN_SYSTEM];
-	int std_nice[NR_STD_WORKER_POOLS] = { 0, HIGHPRI_NICE_LEVEL };
-	void (*irq_work_fns[NR_STD_WORKER_POOLS])(struct irq_work *) =
+	int std_prio[NR_STD_WORKER_POOLS] = { 0, WQ_PRIO_HIGH, WQ_PRIO_RT };
+	int std_nice[NR_STD_WORKER_POOLS] = { 0, HIGHPRI_NICE_LEVEL, 0 };
+	void (*irq_work_fns[NR_BH_WORKER_POOLS])(struct irq_work *) =
 		{ bh_pool_kick_normal, bh_pool_kick_highpri };
 	int i, cpu;
 
@@ -8008,15 +8076,19 @@ void __init workqueue_init_early(void)
 
 		i = 0;
 		for_each_bh_worker_pool(pool, cpu) {
-			init_cpu_worker_pool(pool, cpu, std_nice[i]);
+			init_cpu_worker_pool(pool, cpu, std_prio[i], std_nice[i]);
 			pool->flags |= POOL_BH;
 			init_irq_work(bh_pool_irq_work(pool), irq_work_fns[i]);
 			i++;
 		}
 
 		i = 0;
-		for_each_cpu_worker_pool(pool, cpu)
-			init_cpu_worker_pool(pool, cpu, std_nice[i++]);
+		for_each_cpu_worker_pool(pool, cpu) {
+			init_cpu_worker_pool(pool, cpu, std_prio[i], std_nice[i]);
+			if (i == WQ_PRIO_RT)
+				pool->flags |= POOL_RT;
+			i++;
+		}
 	}
 
 	/* create default unbound and ordered wq attrs */
@@ -8024,6 +8096,7 @@ void __init workqueue_init_early(void)
 		struct workqueue_attrs *attrs;
 
 		BUG_ON(!(attrs = alloc_workqueue_attrs()));
+		attrs->prio = std_prio[i];
 		attrs->nice = std_nice[i];
 		unbound_std_wq_attrs[i] = attrs;
 
@@ -8032,6 +8105,7 @@ void __init workqueue_init_early(void)
 		 * guaranteed by max_active which is enforced by pwqs.
 		 */
 		BUG_ON(!(attrs = alloc_workqueue_attrs()));
+		attrs->prio = std_prio[i];
 		attrs->nice = std_nice[i];
 		attrs->ordered = true;
 		ordered_wq_attrs[i] = attrs;
-- 
2.54.0

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

[Tejun Heo]

Hello, Tvrtko.

On Mon, Jul 13, 2026 at 03:14:35PM +0100, Tvrtko Ursulin wrote:
> We use a mininum priority level since we only care about winning the
> contest against normal background CPU load. We also limit the number of
> instantiated threads, both per workqueue to a maximum of two, and system-
> wide to a maximum of either two or one less than the number of online
> CPUs. The idea of that is to prevent system wide starvation cause by
> potentially misbehaving work items.

Do your use cases need concurrency management on per-cpu workqueues?
Concurrency management is what per-cpu pools do to reduce the number of
parallel worker contexts when there are multiple pending work items
competing for the same CPU - the next work item starts executing only
after the current worker blocks. That doesn't seem applicable to RT
usage where the point is running each work item as soon as possible.

If it isn't needed, how about supporting RT only on unbound workqueues?
Where per-cpu execution is wanted, an unbound workqueue with the
affinity scope set to "cpu" and affn_strict on behaves like a per-cpu
workqueue sans concurrency management. That'd leave the per-cpu worker
pools alone and confine the changes to the unbound side.

The following is an AI review of the patch:

> 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 successfuly 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.
>
> For these situations the existing WQ_HIPRI does not bring a noticeable
> improvemet and a stronger hint is needed.
>
> Lets add WQ_RTPRI which creates workers with a SCHED_FIFO scheduling class
> to improve this.
>
> We use a mininum priority level since we only care about winning the
> contest against normal background CPU load. We also limit the number of
> instantiated threads, both per workqueue to a maximum of two, and system-
> wide to a maximum of either two or one less than the number of online
> CPUs. The idea of that is to prevent system wide starvation cause by
> potentially misbehaving work items.

A few typos: "successfuly", "improvemet", "WQ_HIPRI", "mininum",
"cause by".

Where does the "per workqueue to a maximum of two" limit come from?
__alloc_workqueue() clamps max_active to num_online_cpus() / 2, which is
four on an eight-CPU machine; the two in this series comes from the
max_active the panthor patch passes in.

The system-wide limit also doesn't seem to apply to the panthor use case
at all, see the POOL_RT questions below.

> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 33b721a9af02..0473c009690f 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c

[ ... ]

> @@ -2863,7 +2868,11 @@ static struct worker *create_worker(struct worker_pool *pool)
>  			goto fail;
>  		}
>
> -		set_user_nice(worker->task, pool->attrs->nice);
> +		if (pool->attrs->prio == WQ_PRIO_RT)
> +			sched_set_fifo_low(worker->task);
> +		else if (pool->attrs->prio == WQ_PRIO_HIGH)
> +			set_user_nice(worker->task, pool->attrs->nice);
> +
>  		kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
>  	}

Doesn't this stop applying attrs->nice to WQ_PRIO_NORMAL pools?

Unbound workqueues have a user-settable nice level via the sysfs "nice"
attribute, and wq_nice_store() still accepts it for non-RT workqueues
and stores it in the pool attrs.  For example the "writeback" workqueue
is WQ_UNBOUND | WQ_SYSFS with normal priority.

After this change, a pool created from attrs with prio == WQ_PRIO_NORMAL
and nice != 0 spawns workers that never get the nice value applied, so
the existing sysfs knob silently stops having any effect.

> @@ -2876,6 +2885,9 @@ static struct worker *create_worker(struct worker_pool *pool)
>  	worker->pool->nr_workers++;
>  	worker_enter_idle(worker);
>
> +	if (pool->flags & POOL_RT)
> +		atomic_inc(&total_rtpri_workers);
> +

[ ... ]

> @@ -2909,6 +2921,8 @@ static void reap_dying_workers(struct list_head *cull_list)
>  	list_for_each_entry_safe(worker, tmp, cull_list, entry) {
>  		list_del_init(&worker->entry);
>  		kthread_stop_put(worker->task);
> +		if (worker->flags & WQ_RTPRI)
> +			atomic_dec(&total_rtpri_workers);
>  		kfree(worker);
>  	}
>  }

Is this decrement testing the right flags field?  worker->flags holds
enum worker_flags values (WORKER_DIE, WORKER_IDLE, ...) and no worker
flag uses the bit that WQ_RTPRI (an enum wq_flags value) occupies, so
this condition is never true.

That means total_rtpri_workers is only ever incremented and can never
drop when workers of an RT pool are culled or their pool is released.
The increment above keys off pool->flags & POOL_RT, so the two sides of
the accounting can't pair up as written.

> @@ -3110,6 +3124,19 @@ __acquires(&pool->lock)
>  	mod_timer(&pool->mayday_timer, jiffies + MAYDAY_INITIAL_TIMEOUT);
>
>  	while (true) {
> +		if (pool->flags & POOL_RT) {
> +			unsigned int max = num_online_cpus();
> +
> +			if (max > 2)
> +				max = max - 1;
> +			else
> +				max = 2;
> +
> +			/* Global cap on the number of RT workers */
> +			if (atomic_read(&total_rtpri_workers) >= max)
> +				break;
> +		}
> +
>  		if (create_worker(pool) || !need_to_create_worker(pool))
>  			break;

Three questions about this cap.

1) Isn't the cap already fully consumed at boot?  workqueue_init() runs
create_worker() for every per-CPU standard pool of every online CPU, and
with NR_STD_WORKER_POOLS now 3 that includes the new POOL_RT pool:

	for_each_online_cpu(cpu) {
		for_each_cpu_worker_pool(pool, cpu) {
			pool->flags &= ~POOL_DISASSOCIATED;
			BUG_ON(!create_worker(pool));
		}
	}

On an N-CPU machine total_rtpri_workers is N right after boot while the
cap is max(N - 1, 2), so for any N >= 2 the break above already fires
before the first WQ_RTPRI user exists, and with the decrement never
running (see above) it stays that way.  A side effect is that every
machine now boots one idle SCHED_FIFO kworker per CPU whether or not
anything uses WQ_RTPRI.

2) What happens when this break fires while need_to_create_worker() is
still true?  The code after the loop is:

	timer_delete_sync(&pool->mayday_timer);
	raw_spin_lock_irq(&pool->lock);
	if (need_to_create_worker(pool))
		goto restart;

so the manager goes back to restart and loops forever.  Nothing in the
cap path sleeps, and each pass re-arms and then deletes the mayday timer
before it can expire, so pool_mayday_timeout() never runs and no rescuer
is engaged either.

Since worker_thread() only starts processing work once
may_start_working() holds, the first work item ever queued on a per-CPU
WQ_RTPRI workqueue would leave its SCHED_FIFO worker spinning in
worker_thread() -> manage_workers() -> maybe_create_worker() on that CPU
while the work item is never executed.  Nothing in this series creates a
per-CPU WQ_RTPRI workqueue, but __alloc_workqueue() only rejects the
WQ_HIGHPRI combination, so this is reachable as soon as one appears.

3) POOL_RT is only set on the per-CPU pools in workqueue_init_early().
get_unbound_pool() never sets it, so neither the cap nor the accounting
applies to unbound RT pools, which is the only configuration the panthor
patch in this series uses (WQ_RTPRI | WQ_MEM_RECLAIM | WQ_UNBOUND).

That seems to invert the commit message: the system-wide limit doesn't
restrict the posted consumer at all, while it permanently prevents
worker creation in the per-CPU pools it does cover.

[ ... ]

> @@ -5842,7 +5884,12 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
>  		pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n",
>  			     wq->name);
>
> -	if (flags & WQ_BH) {
> +	if (flags & WQ_RTPRI) {
> +		/*
> +		 * RT workqueues are limited to max half of the online CPUs.
> +		 */
> +		max_active = min_t(int, max_active, num_online_cpus() / 2);
> +	} else if (flags & WQ_BH) {

Can max_active end up 0 here?  Two ways:

  - With one online CPU (nr_cpus=1 or maxcpus=1 boots),
    num_online_cpus() / 2 is 0, so the panthor workqueue created with
    max_active == 2 gets 0.

  - Passing max_active == 0, the documented "use default" value, stays
    0 because the "max_active ?: WQ_DFL_ACTIVE" fallback and
    wq_clamp_max_active(), which enforces a minimum of 1, are in the
    else branch and are skipped for WQ_RTPRI.

With wq->max_active == 0, wq_adjust_max_active() returns before
activating anything and pwq_tryinc_nr_active() can never succeed, so
every work item queued on such a workqueue stays on the inactive list
forever and flush/destroy hang.

Also, num_online_cpus() is sampled once at allocation time, so a
workqueue allocated before secondary CPUs are brought online keeps a
limit computed from the momentary CPU count.

While in this area: for WQ_RTPRI | WQ_MEM_RECLAIM, should the rescuer
get SCHED_FIFO as well?  rescuer_thread() runs at RESCUER_NICE_LEVEL,
so under a mayday the work this flag is trying to keep low latency
executes below the promised priority.

[ ... ]

> @@ -7284,10 +7339,14 @@ static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
>  {
>  	struct workqueue_struct *wq = dev_to_wq(dev);
> -	int written;
> +	int written, nice;
>
>  	mutex_lock(&wq->mutex);
> -	written = scnprintf(buf, PAGE_SIZE, "%d\n", wq->unbound_attrs->nice);
> +	if (wq->unbound_attrs->prio == WQ_PRIO_RT)
> +		nice = INT_MIN;
> +	else
> +		nice = wq->unbound_attrs->nice;
> +	written = scnprintf(buf, PAGE_SIZE, "%d\n", nice);

This isn't a bug, but is INT_MIN the value userspace should read from
the nice attribute of an RT workqueue?  This file has so far only ever
produced values in the [-20, 19] range.

[ ... ]

Thanks.

-- 
tejun

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

[Matthew Brost]

On Mon, Jul 13, 2026 at 08:59:58AM -1000, Tejun Heo wrote:
> Hello, Tvrtko.
> 
> On Mon, Jul 13, 2026 at 03:14:35PM +0100, Tvrtko Ursulin wrote:
> > We use a mininum priority level since we only care about winning the
> > contest against normal background CPU load. We also limit the number of
> > instantiated threads, both per workqueue to a maximum of two, and system-
> > wide to a maximum of either two or one less than the number of online
> > CPUs. The idea of that is to prevent system wide starvation cause by
> > potentially misbehaving work items.
> 
> Do your use cases need concurrency management on per-cpu workqueues?
> Concurrency management is what per-cpu pools do to reduce the number of
> parallel worker contexts when there are multiple pending work items
> competing for the same CPU - the next work item starts executing only
> after the current worker blocks. That doesn't seem applicable to RT
> usage where the point is running each work item as soon as possible.
> 
> If it isn't needed, how about supporting RT only on unbound workqueues?

The DRM cases I can think of are all unbound workqueues afiak.

Matt

> Where per-cpu execution is wanted, an unbound workqueue with the
> affinity scope set to "cpu" and affn_strict on behaves like a per-cpu
> workqueue sans concurrency management. That'd leave the per-cpu worker
> pools alone and confine the changes to the unbound side.
> 
> The following is an AI review of the patch:
> 
> > 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 successfuly 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.
> >
> > For these situations the existing WQ_HIPRI does not bring a noticeable
> > improvemet and a stronger hint is needed.
> >
> > Lets add WQ_RTPRI which creates workers with a SCHED_FIFO scheduling class
> > to improve this.
> >
> > We use a mininum priority level since we only care about winning the
> > contest against normal background CPU load. We also limit the number of
> > instantiated threads, both per workqueue to a maximum of two, and system-
> > wide to a maximum of either two or one less than the number of online
> > CPUs. The idea of that is to prevent system wide starvation cause by
> > potentially misbehaving work items.
> 
> A few typos: "successfuly", "improvemet", "WQ_HIPRI", "mininum",
> "cause by".
> 
> Where does the "per workqueue to a maximum of two" limit come from?
> __alloc_workqueue() clamps max_active to num_online_cpus() / 2, which is
> four on an eight-CPU machine; the two in this series comes from the
> max_active the panthor patch passes in.
> 
> The system-wide limit also doesn't seem to apply to the panthor use case
> at all, see the POOL_RT questions below.
> 
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 33b721a9af02..0473c009690f 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> 
> [ ... ]
> 
> > @@ -2863,7 +2868,11 @@ static struct worker *create_worker(struct worker_pool *pool)
> >  			goto fail;
> >  		}
> >
> > -		set_user_nice(worker->task, pool->attrs->nice);
> > +		if (pool->attrs->prio == WQ_PRIO_RT)
> > +			sched_set_fifo_low(worker->task);
> > +		else if (pool->attrs->prio == WQ_PRIO_HIGH)
> > +			set_user_nice(worker->task, pool->attrs->nice);
> > +
> >  		kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
> >  	}
> 
> Doesn't this stop applying attrs->nice to WQ_PRIO_NORMAL pools?
> 
> Unbound workqueues have a user-settable nice level via the sysfs "nice"
> attribute, and wq_nice_store() still accepts it for non-RT workqueues
> and stores it in the pool attrs.  For example the "writeback" workqueue
> is WQ_UNBOUND | WQ_SYSFS with normal priority.
> 
> After this change, a pool created from attrs with prio == WQ_PRIO_NORMAL
> and nice != 0 spawns workers that never get the nice value applied, so
> the existing sysfs knob silently stops having any effect.
> 
> > @@ -2876,6 +2885,9 @@ static struct worker *create_worker(struct worker_pool *pool)
> >  	worker->pool->nr_workers++;
> >  	worker_enter_idle(worker);
> >
> > +	if (pool->flags & POOL_RT)
> > +		atomic_inc(&total_rtpri_workers);
> > +
> 
> [ ... ]
> 
> > @@ -2909,6 +2921,8 @@ static void reap_dying_workers(struct list_head *cull_list)
> >  	list_for_each_entry_safe(worker, tmp, cull_list, entry) {
> >  		list_del_init(&worker->entry);
> >  		kthread_stop_put(worker->task);
> > +		if (worker->flags & WQ_RTPRI)
> > +			atomic_dec(&total_rtpri_workers);
> >  		kfree(worker);
> >  	}
> >  }
> 
> Is this decrement testing the right flags field?  worker->flags holds
> enum worker_flags values (WORKER_DIE, WORKER_IDLE, ...) and no worker
> flag uses the bit that WQ_RTPRI (an enum wq_flags value) occupies, so
> this condition is never true.
> 
> That means total_rtpri_workers is only ever incremented and can never
> drop when workers of an RT pool are culled or their pool is released.
> The increment above keys off pool->flags & POOL_RT, so the two sides of
> the accounting can't pair up as written.
> 
> > @@ -3110,6 +3124,19 @@ __acquires(&pool->lock)
> >  	mod_timer(&pool->mayday_timer, jiffies + MAYDAY_INITIAL_TIMEOUT);
> >
> >  	while (true) {
> > +		if (pool->flags & POOL_RT) {
> > +			unsigned int max = num_online_cpus();
> > +
> > +			if (max > 2)
> > +				max = max - 1;
> > +			else
> > +				max = 2;
> > +
> > +			/* Global cap on the number of RT workers */
> > +			if (atomic_read(&total_rtpri_workers) >= max)
> > +				break;
> > +		}
> > +
> >  		if (create_worker(pool) || !need_to_create_worker(pool))
> >  			break;
> 
> Three questions about this cap.
> 
> 1) Isn't the cap already fully consumed at boot?  workqueue_init() runs
> create_worker() for every per-CPU standard pool of every online CPU, and
> with NR_STD_WORKER_POOLS now 3 that includes the new POOL_RT pool:
> 
> 	for_each_online_cpu(cpu) {
> 		for_each_cpu_worker_pool(pool, cpu) {
> 			pool->flags &= ~POOL_DISASSOCIATED;
> 			BUG_ON(!create_worker(pool));
> 		}
> 	}
> 
> On an N-CPU machine total_rtpri_workers is N right after boot while the
> cap is max(N - 1, 2), so for any N >= 2 the break above already fires
> before the first WQ_RTPRI user exists, and with the decrement never
> running (see above) it stays that way.  A side effect is that every
> machine now boots one idle SCHED_FIFO kworker per CPU whether or not
> anything uses WQ_RTPRI.
> 
> 2) What happens when this break fires while need_to_create_worker() is
> still true?  The code after the loop is:
> 
> 	timer_delete_sync(&pool->mayday_timer);
> 	raw_spin_lock_irq(&pool->lock);
> 	if (need_to_create_worker(pool))
> 		goto restart;
> 
> so the manager goes back to restart and loops forever.  Nothing in the
> cap path sleeps, and each pass re-arms and then deletes the mayday timer
> before it can expire, so pool_mayday_timeout() never runs and no rescuer
> is engaged either.
> 
> Since worker_thread() only starts processing work once
> may_start_working() holds, the first work item ever queued on a per-CPU
> WQ_RTPRI workqueue would leave its SCHED_FIFO worker spinning in
> worker_thread() -> manage_workers() -> maybe_create_worker() on that CPU
> while the work item is never executed.  Nothing in this series creates a
> per-CPU WQ_RTPRI workqueue, but __alloc_workqueue() only rejects the
> WQ_HIGHPRI combination, so this is reachable as soon as one appears.
> 
> 3) POOL_RT is only set on the per-CPU pools in workqueue_init_early().
> get_unbound_pool() never sets it, so neither the cap nor the accounting
> applies to unbound RT pools, which is the only configuration the panthor
> patch in this series uses (WQ_RTPRI | WQ_MEM_RECLAIM | WQ_UNBOUND).
> 
> That seems to invert the commit message: the system-wide limit doesn't
> restrict the posted consumer at all, while it permanently prevents
> worker creation in the per-CPU pools it does cover.
> 
> [ ... ]
> 
> > @@ -5842,7 +5884,12 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
> >  		pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n",
> >  			     wq->name);
> >
> > -	if (flags & WQ_BH) {
> > +	if (flags & WQ_RTPRI) {
> > +		/*
> > +		 * RT workqueues are limited to max half of the online CPUs.
> > +		 */
> > +		max_active = min_t(int, max_active, num_online_cpus() / 2);
> > +	} else if (flags & WQ_BH) {
> 
> Can max_active end up 0 here?  Two ways:
> 
>   - With one online CPU (nr_cpus=1 or maxcpus=1 boots),
>     num_online_cpus() / 2 is 0, so the panthor workqueue created with
>     max_active == 2 gets 0.
> 
>   - Passing max_active == 0, the documented "use default" value, stays
>     0 because the "max_active ?: WQ_DFL_ACTIVE" fallback and
>     wq_clamp_max_active(), which enforces a minimum of 1, are in the
>     else branch and are skipped for WQ_RTPRI.
> 
> With wq->max_active == 0, wq_adjust_max_active() returns before
> activating anything and pwq_tryinc_nr_active() can never succeed, so
> every work item queued on such a workqueue stays on the inactive list
> forever and flush/destroy hang.
> 
> Also, num_online_cpus() is sampled once at allocation time, so a
> workqueue allocated before secondary CPUs are brought online keeps a
> limit computed from the momentary CPU count.
> 
> While in this area: for WQ_RTPRI | WQ_MEM_RECLAIM, should the rescuer
> get SCHED_FIFO as well?  rescuer_thread() runs at RESCUER_NICE_LEVEL,
> so under a mayday the work this flag is trying to keep low latency
> executes below the promised priority.
> 
> [ ... ]
> 
> > @@ -7284,10 +7339,14 @@ static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
> >  			    char *buf)
> >  {
> >  	struct workqueue_struct *wq = dev_to_wq(dev);
> > -	int written;
> > +	int written, nice;
> >
> >  	mutex_lock(&wq->mutex);
> > -	written = scnprintf(buf, PAGE_SIZE, "%d\n", wq->unbound_attrs->nice);
> > +	if (wq->unbound_attrs->prio == WQ_PRIO_RT)
> > +		nice = INT_MIN;
> > +	else
> > +		nice = wq->unbound_attrs->nice;
> > +	written = scnprintf(buf, PAGE_SIZE, "%d\n", nice);
> 
> This isn't a bug, but is INT_MIN the value userspace should read from
> the nice attribute of an RT workqueue?  This file has so far only ever
> produced values in the [-20, 19] range.
> 
> [ ... ]
> 
> Thanks.
> 
> -- 
> tejun

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

[Tvrtko Ursulin]

Hi,

On 13/07/2026 19:59, Tejun Heo wrote:
> Hello, Tvrtko.
> 
> On Mon, Jul 13, 2026 at 03:14:35PM +0100, Tvrtko Ursulin wrote:
>> We use a mininum priority level since we only care about winning the
>> contest against normal background CPU load. We also limit the number of
>> instantiated threads, both per workqueue to a maximum of two, and system-
>> wide to a maximum of either two or one less than the number of online
>> CPUs. The idea of that is to prevent system wide starvation cause by
>> potentially misbehaving work items.
> 
> Do your use cases need concurrency management on per-cpu workqueues?
> Concurrency management is what per-cpu pools do to reduce the number of
> parallel worker contexts when there are multiple pending work items
> competing for the same CPU - the next work item starts executing only
> after the current worker blocks. That doesn't seem applicable to RT
> usage where the point is running each work item as soon as possible.
> 
> If it isn't needed, how about supporting RT only on unbound workqueues?
> Where per-cpu execution is wanted, an unbound workqueue with the
> affinity scope set to "cpu" and affn_strict on behaves like a per-cpu
> workqueue sans concurrency management. That'd leave the per-cpu worker
> pools alone and confine the changes to the unbound side.

Panthor and xe are the two drivers where we are currently looking to 
improve submission latency, so talking about only those two for is I 
think fine and definitely makes sense to narrow the scope of RT workers.

For panthor submission, unbound only would be fine since we have already 
agreed it is okay to limit to two workers per device. So there we are 
quite controlled - there will never be more than two threads.

Xe on the other hand is creating one ordered wq per userspace context so 
in theory could explode the number of RT threads. Although it is 
probably only hypothetical since creating then requires CAP_SYS_NICE so 
maybe not a concern.

Given how it also uses WQ_MEM_RECLAIM, so creates a rescuer thread per 
userspace context regardless, perhaps that is okay with everyone?

[1] 746ae46c1113 ("drm/sched: Mark scheduler work queues with 
WQ_MEM_RECLAIM")

> The following is an AI review of the patch:

Thank you, the RFC status is nicely reflected in the number of bugs and 
typos. I will come to them later when the design opens from above are 
agreed upon.

Regards,

Tvrtko

>> 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 successfuly 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.
>>
>> For these situations the existing WQ_HIPRI does not bring a noticeable
>> improvemet and a stronger hint is needed.
>>
>> Lets add WQ_RTPRI which creates workers with a SCHED_FIFO scheduling class
>> to improve this.
>>
>> We use a mininum priority level since we only care about winning the
>> contest against normal background CPU load. We also limit the number of
>> instantiated threads, both per workqueue to a maximum of two, and system-
>> wide to a maximum of either two or one less than the number of online
>> CPUs. The idea of that is to prevent system wide starvation cause by
>> potentially misbehaving work items.
> 
> A few typos: "successfuly", "improvemet", "WQ_HIPRI", "mininum",
> "cause by".
> 
> Where does the "per workqueue to a maximum of two" limit come from?
> __alloc_workqueue() clamps max_active to num_online_cpus() / 2, which is
> four on an eight-CPU machine; the two in this series comes from the
> max_active the panthor patch passes in.
> 
> The system-wide limit also doesn't seem to apply to the panthor use case
> at all, see the POOL_RT questions below.
> 
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 33b721a9af02..0473c009690f 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
> 
> [ ... ]
> 
>> @@ -2863,7 +2868,11 @@ static struct worker *create_worker(struct worker_pool *pool)
>>   			goto fail;
>>   		}
>>
>> -		set_user_nice(worker->task, pool->attrs->nice);
>> +		if (pool->attrs->prio == WQ_PRIO_RT)
>> +			sched_set_fifo_low(worker->task);
>> +		else if (pool->attrs->prio == WQ_PRIO_HIGH)
>> +			set_user_nice(worker->task, pool->attrs->nice);
>> +
>>   		kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
>>   	}
> 
> Doesn't this stop applying attrs->nice to WQ_PRIO_NORMAL pools?
> 
> Unbound workqueues have a user-settable nice level via the sysfs "nice"
> attribute, and wq_nice_store() still accepts it for non-RT workqueues
> and stores it in the pool attrs.  For example the "writeback" workqueue
> is WQ_UNBOUND | WQ_SYSFS with normal priority.
> 
> After this change, a pool created from attrs with prio == WQ_PRIO_NORMAL
> and nice != 0 spawns workers that never get the nice value applied, so
> the existing sysfs knob silently stops having any effect.
> 
>> @@ -2876,6 +2885,9 @@ static struct worker *create_worker(struct worker_pool *pool)
>>   	worker->pool->nr_workers++;
>>   	worker_enter_idle(worker);
>>
>> +	if (pool->flags & POOL_RT)
>> +		atomic_inc(&total_rtpri_workers);
>> +
> 
> [ ... ]
> 
>> @@ -2909,6 +2921,8 @@ static void reap_dying_workers(struct list_head *cull_list)
>>   	list_for_each_entry_safe(worker, tmp, cull_list, entry) {
>>   		list_del_init(&worker->entry);
>>   		kthread_stop_put(worker->task);
>> +		if (worker->flags & WQ_RTPRI)
>> +			atomic_dec(&total_rtpri_workers);
>>   		kfree(worker);
>>   	}
>>   }
> 
> Is this decrement testing the right flags field?  worker->flags holds
> enum worker_flags values (WORKER_DIE, WORKER_IDLE, ...) and no worker
> flag uses the bit that WQ_RTPRI (an enum wq_flags value) occupies, so
> this condition is never true.
> 
> That means total_rtpri_workers is only ever incremented and can never
> drop when workers of an RT pool are culled or their pool is released.
> The increment above keys off pool->flags & POOL_RT, so the two sides of
> the accounting can't pair up as written.
> 
>> @@ -3110,6 +3124,19 @@ __acquires(&pool->lock)
>>   	mod_timer(&pool->mayday_timer, jiffies + MAYDAY_INITIAL_TIMEOUT);
>>
>>   	while (true) {
>> +		if (pool->flags & POOL_RT) {
>> +			unsigned int max = num_online_cpus();
>> +
>> +			if (max > 2)
>> +				max = max - 1;
>> +			else
>> +				max = 2;
>> +
>> +			/* Global cap on the number of RT workers */
>> +			if (atomic_read(&total_rtpri_workers) >= max)
>> +				break;
>> +		}
>> +
>>   		if (create_worker(pool) || !need_to_create_worker(pool))
>>   			break;
> 
> Three questions about this cap.
> 
> 1) Isn't the cap already fully consumed at boot?  workqueue_init() runs
> create_worker() for every per-CPU standard pool of every online CPU, and
> with NR_STD_WORKER_POOLS now 3 that includes the new POOL_RT pool:
> 
> 	for_each_online_cpu(cpu) {
> 		for_each_cpu_worker_pool(pool, cpu) {
> 			pool->flags &= ~POOL_DISASSOCIATED;
> 			BUG_ON(!create_worker(pool));
> 		}
> 	}
> 
> On an N-CPU machine total_rtpri_workers is N right after boot while the
> cap is max(N - 1, 2), so for any N >= 2 the break above already fires
> before the first WQ_RTPRI user exists, and with the decrement never
> running (see above) it stays that way.  A side effect is that every
> machine now boots one idle SCHED_FIFO kworker per CPU whether or not
> anything uses WQ_RTPRI.
> 
> 2) What happens when this break fires while need_to_create_worker() is
> still true?  The code after the loop is:
> 
> 	timer_delete_sync(&pool->mayday_timer);
> 	raw_spin_lock_irq(&pool->lock);
> 	if (need_to_create_worker(pool))
> 		goto restart;
> 
> so the manager goes back to restart and loops forever.  Nothing in the
> cap path sleeps, and each pass re-arms and then deletes the mayday timer
> before it can expire, so pool_mayday_timeout() never runs and no rescuer
> is engaged either.
> 
> Since worker_thread() only starts processing work once
> may_start_working() holds, the first work item ever queued on a per-CPU
> WQ_RTPRI workqueue would leave its SCHED_FIFO worker spinning in
> worker_thread() -> manage_workers() -> maybe_create_worker() on that CPU
> while the work item is never executed.  Nothing in this series creates a
> per-CPU WQ_RTPRI workqueue, but __alloc_workqueue() only rejects the
> WQ_HIGHPRI combination, so this is reachable as soon as one appears.
> 
> 3) POOL_RT is only set on the per-CPU pools in workqueue_init_early().
> get_unbound_pool() never sets it, so neither the cap nor the accounting
> applies to unbound RT pools, which is the only configuration the panthor
> patch in this series uses (WQ_RTPRI | WQ_MEM_RECLAIM | WQ_UNBOUND).
> 
> That seems to invert the commit message: the system-wide limit doesn't
> restrict the posted consumer at all, while it permanently prevents
> worker creation in the per-CPU pools it does cover.
> 
> [ ... ]
> 
>> @@ -5842,7 +5884,12 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
>>   		pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n",
>>   			     wq->name);
>>
>> -	if (flags & WQ_BH) {
>> +	if (flags & WQ_RTPRI) {
>> +		/*
>> +		 * RT workqueues are limited to max half of the online CPUs.
>> +		 */
>> +		max_active = min_t(int, max_active, num_online_cpus() / 2);
>> +	} else if (flags & WQ_BH) {
> 
> Can max_active end up 0 here?  Two ways:
> 
>    - With one online CPU (nr_cpus=1 or maxcpus=1 boots),
>      num_online_cpus() / 2 is 0, so the panthor workqueue created with
>      max_active == 2 gets 0.
> 
>    - Passing max_active == 0, the documented "use default" value, stays
>      0 because the "max_active ?: WQ_DFL_ACTIVE" fallback and
>      wq_clamp_max_active(), which enforces a minimum of 1, are in the
>      else branch and are skipped for WQ_RTPRI.
> 
> With wq->max_active == 0, wq_adjust_max_active() returns before
> activating anything and pwq_tryinc_nr_active() can never succeed, so
> every work item queued on such a workqueue stays on the inactive list
> forever and flush/destroy hang.
> 
> Also, num_online_cpus() is sampled once at allocation time, so a
> workqueue allocated before secondary CPUs are brought online keeps a
> limit computed from the momentary CPU count.
> 
> While in this area: for WQ_RTPRI | WQ_MEM_RECLAIM, should the rescuer
> get SCHED_FIFO as well?  rescuer_thread() runs at RESCUER_NICE_LEVEL,
> so under a mayday the work this flag is trying to keep low latency
> executes below the promised priority.
> 
> [ ... ]
> 
>> @@ -7284,10 +7339,14 @@ static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
>>   			    char *buf)
>>   {
>>   	struct workqueue_struct *wq = dev_to_wq(dev);
>> -	int written;
>> +	int written, nice;
>>
>>   	mutex_lock(&wq->mutex);
>> -	written = scnprintf(buf, PAGE_SIZE, "%d\n", wq->unbound_attrs->nice);
>> +	if (wq->unbound_attrs->prio == WQ_PRIO_RT)
>> +		nice = INT_MIN;
>> +	else
>> +		nice = wq->unbound_attrs->nice;
>> +	written = scnprintf(buf, PAGE_SIZE, "%d\n", nice);
> 
> This isn't a bug, but is INT_MIN the value userspace should read from
> the nice attribute of an RT workqueue?  This file has so far only ever
> produced values in the [-20, 19] range.
> 
> [ ... ]
> 
> Thanks.
> 

[RFC 2/2] drm/panthor: Create per queue priority workqueues

[Tvrtko Ursulin]

Split the single workqueue shared between the driver internal logic and
DRM scheduler use into separate ones, where the DRM scheduler one is
created per GPU priority level using the appropriate mapping to
workqueue priorities.

Low and medium GPU priority are served by a normal workqueue,
high is server by a WQ_HIGHPRI instance, while realtime GPU priority is
using the newly added WQ_RTPRI flag for lowest possible latency.

These workqueues are device global and for all three we set the maximum
concurrency to two in order to keep the GPU optimally fed with work.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Chia-I Wu <olv@google.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 38 +++++++++++++++++++++----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 2bee1c92fb9e..f01adc8c024c 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -152,11 +152,18 @@ struct panthor_scheduler {
 	 *
 	 * Used for the scheduler tick, group update or other kind of FW
 	 * event processing that can't be handled in the threaded interrupt
-	 * path. Also passed to the drm_gpu_scheduler instances embedded
-	 * in panthor_queue.
+	 * path.
 	 */
 	struct workqueue_struct *wq;
 
+	/**
+	 * @submit_wq: Per priority workqueues for the DRM scheduler
+	 *
+	 * Passed to the drm_gpu_scheduler instances embedded
+	 * in panthor_queue based on the queue priority.
+	 */
+	struct workqueue_struct *submit_wq[PANTHOR_CSG_PRIORITY_COUNT];
+
 	/**
 	 * @heap_alloc_wq: Workqueue used to schedule tiler_oom works.
 	 *
@@ -3488,7 +3495,6 @@ group_create_queue(struct panthor_group *group,
 {
 	struct drm_sched_init_args sched_args = {
 		.ops = &panthor_queue_sched_ops,
-		.submit_wq = group->ptdev->scheduler->wq,
 		/*
 		 * The credit limit argument tells us the total number of
 		 * instructions across all CS slots in the ringbuffer, with
@@ -3581,8 +3587,14 @@ group_create_queue(struct panthor_group *group,
 		goto err_free_queue;
 	}
 
+	if (group->priority >= ARRAY_SIZE(group->ptdev->scheduler->submit_wq) ||
+	    !group->ptdev->scheduler->submit_wq[group->priority]) {
+		ret = -EINVAL;
+		goto err_free_queue;
+	}
+
 	sched_args.name = queue->name;
-
+	sched_args.submit_wq = group->ptdev->scheduler->submit_wq[group->priority];
 	ret = drm_sched_init(&queue->scheduler, &sched_args);
 	if (ret)
 		goto err_free_queue;
@@ -4072,6 +4084,15 @@ static void panthor_sched_fini(struct drm_device *ddev, void *res)
 	if (!sched || !sched->csg_slot_count)
 		return;
 
+	if (sched->submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM])
+		destroy_workqueue(sched->submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM]);
+
+	if (sched->submit_wq[PANTHOR_CSG_PRIORITY_HIGH])
+		destroy_workqueue(sched->submit_wq[PANTHOR_CSG_PRIORITY_HIGH]);
+
+	if (sched->submit_wq[PANTHOR_CSG_PRIORITY_RT])
+		destroy_workqueue(sched->submit_wq[PANTHOR_CSG_PRIORITY_RT]);
+
 	if (sched->wq)
 		destroy_workqueue(sched->wq);
 
@@ -4173,7 +4194,14 @@ int panthor_sched_init(struct panthor_device *ptdev)
 	 */
 	sched->heap_alloc_wq = alloc_workqueue("panthor-heap-alloc", WQ_UNBOUND, 0);
 	sched->wq = alloc_workqueue("panthor-csf-sched", WQ_MEM_RECLAIM | WQ_UNBOUND, 0);
-	if (!sched->wq || !sched->heap_alloc_wq) {
+	sched->submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM] = alloc_workqueue("panthor-drm", WQ_MEM_RECLAIM | WQ_UNBOUND, 2);
+	sched->submit_wq[PANTHOR_CSG_PRIORITY_LOW] = sched->submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM];
+	sched->submit_wq[PANTHOR_CSG_PRIORITY_HIGH] = alloc_workqueue("panthor-drm-high", WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND, 2);
+	sched->submit_wq[PANTHOR_CSG_PRIORITY_RT] = alloc_workqueue("panthor-drm-rt", WQ_RTPRI | WQ_MEM_RECLAIM | WQ_UNBOUND, 2);
+	if (!sched->wq || !sched->heap_alloc_wq ||
+	    !sched->submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM] ||
+	    !sched->submit_wq[PANTHOR_CSG_PRIORITY_HIGH] ||
+	    !sched->submit_wq[PANTHOR_CSG_PRIORITY_RT]) {
 		panthor_sched_fini(&ptdev->base, sched);
 		drm_err(&ptdev->base, "Failed to allocate the workqueues");
 		return -ENOMEM;
-- 
2.54.0