[PATCH v8 0/6] Realtime workqueues and panthor realtime submission

[PATCH v8 0/6] 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 can add significant 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_RT, submit latency becomes completely controlled with the median of 14us and
95 and 98-th percentiles at 23us and 25us respectively.

[And updated latency measurements from v8:]

        us      7.3.0   WQ_RT   +strict_affinity

       median     30     21           11
        min        2      3            2
        max     1038    941          108
       95pct    2384     49           28
       98pct    2922     63           51

[/]

Important to note is that VK_QUEUE_GLOBAL_PRIORITY_REALTIME already requires
that userspace either has CAP_SYS_NICE, or the DRM master capability. The latter
can be had by a single DRM client per GPU, usually the compositor since DRM
master is required to drive the display hardware. It is not possible to have two
DRM master clients simultaneously.

On the overall this limits the number of userspace threads which can access the
RT queue - it either has to be a compositor, or a rendering client which was
explicitly granted CAP_SYS_NICE.

v2:
 * See patch 1 changelog.

v3:
 * See patch 1 changelog + apologies for v3 following so quickly after v2.
   I have found a race condition as I expanded the testing to a second platform.

v4:
 * See patch 1 changelog.

v5:
 * Rebased on top of tj/for-7.4 branch.
 * New patch in series simplifies unbound sysfs attribute handling.
 * WQ_RTPRI reworked to fold RT marker into attr->nice.

v6:
 * Fixed accidental sysfs attribute rename (patch 1).
 * Various review feedback as per patch 2 changelog.

v7:
 * Describe DRM master in the cover letter.
 * New patch in series to separate attribute sanitizing steps.
 * WQ_RT updated per the respective change log.

v8:
 * Split out the WQ_RT patch into nice to prio conversion and strict CPU
   affinity as follow up.
 * Added more latency measurements.
 * Dropped bouncing Bradley Morgan's email from cc.
 * Other tweaks as per individual patch change logs.

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 (6):
  workqueue: Simplify unbound sysfs attribute registration
  workqueue: Separate sysfs mutable vs immutable wq attribute sanitizing
  workqueue: Convert nice value tracking to priority based
  workqueue: Add support for real-time workers
  workqueue: Imply strict CPU affinity for WQ_RT
  drm/panthor: Create per queue priority workqueues

 Documentation/core-api/workqueue.rst    |   8 +
 drivers/gpu/drm/panthor/panthor_sched.c |  37 +++-
 include/linux/workqueue.h               |   5 +-
 kernel/workqueue.c                      | 238 +++++++++++++++---------
 tools/workqueue/wq_dump.py              |  34 +++-
 5 files changed, 220 insertions(+), 102 deletions(-)

-- 
2.56.0

[PATCH v8 1/6] workqueue: Simplify unbound sysfs attribute registration

[Tvrtko Ursulin]

Instead of manually registering each attribute we can put them in an
attribute group with a visibility check and device core will handle the
rest, which simplifies the registration and error unwind.

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>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> # v3
---
v2:
 * Fix accidental rename of affinity_scope.

v3:
 * SYSFS_GROUP_INVISIBLE should not be used for un-named groups.

v4:
 * Use DEVICE_ATTR_RW_NAMED. (Tejun)
---
 kernel/workqueue.c | 97 ++++++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 46 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f2e8071a6f4e..14b87e972ce9 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -7623,8 +7623,8 @@ static const struct attribute_group wq_sysfs_group = {
 };
 __ATTRIBUTE_GROUPS(wq_sysfs);
 
-static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
-			    char *buf)
+static ssize_t nice_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 	int written;
@@ -7651,8 +7651,8 @@ static struct workqueue_attrs *wq_sysfs_prep_attrs(struct workqueue_struct *wq)
 	return attrs;
 }
 
-static ssize_t wq_nice_store(struct device *dev, struct device_attribute *attr,
-			     const char *buf, size_t count)
+static ssize_t nice_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 	struct workqueue_attrs *attrs;
@@ -7676,8 +7676,8 @@ static ssize_t wq_nice_store(struct device *dev, struct device_attribute *attr,
 	return ret ?: count;
 }
 
-static ssize_t wq_cpumask_show(struct device *dev,
-			       struct device_attribute *attr, char *buf)
+static ssize_t unbound_cpumask_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 	int written;
@@ -7689,9 +7689,9 @@ static ssize_t wq_cpumask_show(struct device *dev,
 	return written;
 }
 
-static ssize_t wq_cpumask_store(struct device *dev,
-				struct device_attribute *attr,
-				const char *buf, size_t count)
+static ssize_t unbound_cpumask_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 	struct workqueue_attrs *attrs;
@@ -7713,8 +7713,8 @@ static ssize_t wq_cpumask_store(struct device *dev,
 	return ret ?: count;
 }
 
-static ssize_t wq_affn_scope_show(struct device *dev,
-				  struct device_attribute *attr, char *buf)
+static ssize_t affinity_scope_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 	int written;
@@ -7732,9 +7732,9 @@ static ssize_t wq_affn_scope_show(struct device *dev,
 	return written;
 }
 
-static ssize_t wq_affn_scope_store(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t count)
+static ssize_t affinity_scope_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 	struct workqueue_attrs *attrs;
@@ -7755,8 +7755,8 @@ static ssize_t wq_affn_scope_store(struct device *dev,
 	return ret ?: count;
 }
 
-static ssize_t wq_affinity_strict_show(struct device *dev,
-				       struct device_attribute *attr, char *buf)
+static ssize_t affinity_strict_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 
@@ -7764,9 +7764,9 @@ static ssize_t wq_affinity_strict_show(struct device *dev,
 			 wq->attrs->affn_strict);
 }
 
-static ssize_t wq_affinity_strict_store(struct device *dev,
-					struct device_attribute *attr,
-					const char *buf, size_t count)
+static ssize_t affinity_strict_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
 {
 	struct workqueue_struct *wq = dev_to_wq(dev);
 	struct workqueue_attrs *attrs;
@@ -7786,14 +7786,39 @@ static ssize_t wq_affinity_strict_store(struct device *dev,
 	return ret ?: count;
 }
 
-static struct device_attribute wq_sysfs_unbound_attrs[] = {
-	__ATTR(nice, 0644, wq_nice_show, wq_nice_store),
-	__ATTR(cpumask, 0644, wq_cpumask_show, wq_cpumask_store),
-	__ATTR(affinity_scope, 0644, wq_affn_scope_show, wq_affn_scope_store),
-	__ATTR(affinity_strict, 0644, wq_affinity_strict_show, wq_affinity_strict_store),
-	__ATTR_NULL,
+static DEVICE_ATTR_RW(nice);
+static DEVICE_ATTR_RW(affinity_scope);
+static DEVICE_ATTR_RW(affinity_strict);
+/* Avoid naming clash with the other cpumask */
+static DEVICE_ATTR_RW_NAMED(unbound_cpumask, "cpumask");
+
+static struct attribute *wq_sysfs_unbound_attrs[] = {
+	&dev_attr_nice.attr,
+	&dev_attr_unbound_cpumask.attr,
+	&dev_attr_affinity_scope.attr,
+	&dev_attr_affinity_strict.attr,
+	NULL,
 };
 
+static umode_t wq_sysfs_unbound_group_visible(struct kobject *kobj,
+					      struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct workqueue_struct *wq = dev_to_wq(dev);
+
+	if (!(wq->flags & WQ_UNBOUND))
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group wq_sysfs_unbound_group = {
+	.is_visible = wq_sysfs_unbound_group_visible,
+	.attrs = wq_sysfs_unbound_attrs,
+};
+
+__ATTRIBUTE_GROUPS(wq_sysfs_unbound);
+
 static const struct bus_type wq_subsys = {
 	.name				= "workqueue",
 	.dev_groups			= wq_sysfs_groups,
@@ -7931,14 +7956,9 @@ int workqueue_sysfs_register(struct workqueue_struct *wq)
 	wq_dev->wq = wq;
 	wq_dev->dev.bus = &wq_subsys;
 	wq_dev->dev.release = wq_device_release;
+	wq_dev->dev.groups = wq_sysfs_unbound_groups;
 	dev_set_name(&wq_dev->dev, "%s", wq->name);
 
-	/*
-	 * attrs are created separately.  Suppress uevent until
-	 * everything is ready.
-	 */
-	dev_set_uevent_suppress(&wq_dev->dev, true);
-
 	ret = device_register(&wq_dev->dev);
 	if (ret) {
 		put_device(&wq_dev->dev);
@@ -7946,21 +7966,6 @@ int workqueue_sysfs_register(struct workqueue_struct *wq)
 		return ret;
 	}
 
-	if (wq->flags & WQ_UNBOUND) {
-		struct device_attribute *attr;
-
-		for (attr = wq_sysfs_unbound_attrs; attr->attr.name; attr++) {
-			ret = device_create_file(&wq_dev->dev, attr);
-			if (ret) {
-				device_unregister(&wq_dev->dev);
-				wq->wq_dev = NULL;
-				return ret;
-			}
-		}
-	}
-
-	dev_set_uevent_suppress(&wq_dev->dev, false);
-	kobject_uevent(&wq_dev->dev.kobj, KOBJ_ADD);
 	return 0;
 }
 
-- 
2.56.0

[PATCH v8 2/6] workqueue: Separate sysfs mutable vs immutable wq attribute sanitizing

[Tvrtko Ursulin]

In preparation for adding more sanitization steps split the current ones
into ones which are immutable and the ones which can be modified from
sysfs.

Point being that the mutable steps must only run on the initial workqueue
creation, while the immutable properties need to be sanitized every time
attributes are applied.

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>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> # v1
---
v2:
 * Drop a redundant helper and improve commentary. (Tejun)
---
 kernel/workqueue.c | 47 +++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 14b87e972ce9..2f0c3f738c70 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5706,6 +5706,30 @@ static void apply_wqattrs_cleanup(struct apply_wqattrs_ctx *ctx)
 	}
 }
 
+/*
+ * Sanitize attributes derived from the workqueue flags and which need to be
+ * enforced on every attribute application.
+ */
+static void
+sanitize_immutable_workqueue_attrs(struct workqueue_struct *wq,
+				   struct workqueue_attrs *attrs)
+{
+	if (wq->flags & __WQ_ORDERED)
+		attrs->ordered = true;
+}
+
+/*
+ * Sanitize attributes derived from the workqueue flags and which need to be
+ * enforced only on workqueue creation.
+ */
+static void
+sanitize_mutable_workqueue_attrs(struct workqueue_struct *wq,
+				 struct workqueue_attrs *attrs)
+{
+	if (wq->flags & WQ_HIGHPRI)
+		attrs->nice = HIGHPRI_NICE_LEVEL;
+}
+
 /* allocate the attrs and pwqs for later installation */
 static struct apply_wqattrs_ctx *
 apply_wqattrs_prepare(struct workqueue_struct *wq,
@@ -5735,6 +5759,7 @@ apply_wqattrs_prepare(struct workqueue_struct *wq,
 	 * every possible CPU and never falls back, so it has no default.
 	 */
 	copy_workqueue_attrs(new_attrs, attrs);
+	sanitize_immutable_workqueue_attrs(wq, new_attrs);
 	wqattrs_actualize_cpumask(new_attrs, unbound_cpumask);
 	cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask);
 	if (!(wq->flags & WQ_PERCPU)) {
@@ -5757,6 +5782,7 @@ apply_wqattrs_prepare(struct workqueue_struct *wq,
 
 	/* save the user configured attrs and sanitize it. */
 	copy_workqueue_attrs(new_attrs, attrs);
+	sanitize_immutable_workqueue_attrs(wq, new_attrs);
 	cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask);
 	cpumask_copy(new_attrs->__pod_cpumask, new_attrs->cpumask);
 	ctx->attrs = new_attrs;
@@ -5919,24 +5945,6 @@ static void unbound_wq_update_pwq(struct workqueue_struct *wq, int cpu)
 	put_pwq_unlocked(old_pwq);
 }
 
-/* the attrs @wq asked for, as spelled by its flags */
-static struct workqueue_attrs *alloc_wq_std_attrs(struct workqueue_struct *wq)
-{
-	struct workqueue_attrs *attrs;
-
-	attrs = alloc_workqueue_attrs();
-	if (!attrs)
-		return NULL;
-
-	if (wq->flags & WQ_HIGHPRI)
-		attrs->nice = HIGHPRI_NICE_LEVEL;
-
-	if (wq->flags & __WQ_ORDERED)
-		attrs->ordered = true;
-
-	return attrs;
-}
-
 static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 {
 	struct workqueue_attrs *attrs;
@@ -5948,10 +5956,11 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
 	if (!wq->cpu_pwq)
 		goto enomem;
 
-	attrs = alloc_wq_std_attrs(wq);
+	attrs = alloc_workqueue_attrs();
 	if (!attrs)
 		goto enomem;
 
+	sanitize_mutable_workqueue_attrs(wq, attrs);
 	ret = apply_workqueue_attrs_locked(wq, attrs);
 	free_workqueue_attrs(attrs);
 	if (ret)
-- 
2.56.0

[PATCH v8 3/6] workqueue: Convert nice value tracking to priority based

[Tvrtko Ursulin]

In preparation for adding real-time priority workqueues lets convert the
tracking of the nice level to be priority based, ie. following the same
"encoding" as trask_struct->prio.

No functional change.

wq_dump.py is extended to support both old and new kernels by auto-
detecting the available struct member.

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  |  4 +--
 kernel/workqueue.c         | 62 +++++++++++++++++++++-----------------
 tools/workqueue/wq_dump.py | 27 +++++++++++++++--
 3 files changed, 60 insertions(+), 33 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index c8a36423cb34..985fff90f033 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 analogous to task_struct->prio.
 	 */
-	int nice;
+	int prio;
 
 	/**
 	 * @cpumask: allowed CPUs
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2f0c3f738c70..0ed93b263389 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -126,7 +126,7 @@ 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),
 
 	WQ_NAME_LEN		= 32,
 	WORKER_ID_LEN		= 10 + WQ_NAME_LEN, /* "kworker/R-" + WQ_NAME_LEN */
@@ -1275,7 +1275,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;
+	int high = pool->attrs->prio == HIGHPRI_PRIORITY ? 1 : 0;
 
 	return &per_cpu(bh_pool_irq_works, pool->cpu)[high];
 }
@@ -1290,7 +1290,7 @@ static void kick_bh_pool(struct worker_pool *pool)
 		return;
 	}
 #endif
-	if (pool->attrs->nice == HIGHPRI_NICE_LEVEL)
+	if (pool->attrs->prio == HIGHPRI_PRIORITY)
 		raise_softirq_irqoff(HI_SOFTIRQ);
 	else
 		raise_softirq_irqoff(TASKLET_SOFTIRQ);
@@ -2959,7 +2959,8 @@ static int format_worker_id(char *buf, size_t size, struct worker *worker,
 		if (pool->cpu >= 0)
 			return scnprintf(buf, size, "kworker/%d:%d%s",
 					 pool->cpu, worker->id,
-					 pool->attrs->nice < 0  ? "H" : "");
+					 pool->attrs->prio < NICE_TO_PRIO(0) ?
+					 "H" : "");
 		else
 			return scnprintf(buf, size, "kworker/u%d:%d",
 					 pool->id, worker->id);
@@ -3018,7 +3019,8 @@ static struct worker *create_worker(struct worker_pool *pool)
 			goto fail;
 		}
 
-		set_user_nice(worker->task, pool->attrs->nice);
+		set_user_nice(worker->task, PRIO_TO_NICE(pool->attrs->prio));
+
 		kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
 	}
 
@@ -3910,7 +3912,7 @@ static void bh_worker(struct worker *worker)
 
 	if (budget_exhausted)
 		trace_workqueue_bh_budget_yield(pool, restarts, timeout,
-						pool->attrs->nice == HIGHPRI_NICE_LEVEL);
+						pool->attrs->prio == HIGHPRI_PRIORITY);
 }
 
 /*
@@ -3969,7 +3971,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 == HIGHPRI_PRIORITY)
 			queue_work(system_bh_highpri_wq, work);
 		else
 			queue_work(system_bh_wq, work);
@@ -4001,7 +4003,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 == HIGHPRI_PRIORITY)
 			queue_work(system_bh_highpri_wq, &dead_work.work);
 		else
 			queue_work(system_bh_wq, &dead_work.work);
@@ -5005,6 +5007,7 @@ struct workqueue_attrs *alloc_workqueue_attrs_noprof(void)
 		goto fail;
 
 	cpumask_copy(attrs->cpumask, cpu_possible_mask);
+	attrs->prio = DEFAULT_PRIO;
 	attrs->affn_scope = WQ_AFFN_DFL;
 	return attrs;
 fail:
@@ -5015,7 +5018,7 @@ struct workqueue_attrs *alloc_workqueue_attrs_noprof(void)
 static void copy_workqueue_attrs(struct workqueue_attrs *to,
 				 const struct workqueue_attrs *from)
 {
-	to->nice = from->nice;
+	to->prio = from->prio;
 	cpumask_copy(to->cpumask, from->cpumask);
 	cpumask_copy(to->__pod_cpumask, from->__pod_cpumask);
 	to->affn_strict = from->affn_strict;
@@ -5046,7 +5049,7 @@ static u32 wqattrs_hash(const struct workqueue_attrs *attrs)
 {
 	u32 hash = 0;
 
-	hash = jhash_1word(attrs->nice, hash);
+	hash = jhash_1word(attrs->prio, hash);
 	hash = jhash_1word(attrs->affn_strict, hash);
 	hash = jhash(cpumask_bits(attrs->__pod_cpumask),
 		     BITS_TO_LONGS(nr_cpumask_bits) * sizeof(long), hash);
@@ -5060,7 +5063,7 @@ 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->nice != b->nice)
+	if (a->prio != b->prio)
 		return false;
 	if (a->affn_strict != b->affn_strict)
 		return false;
@@ -5727,7 +5730,7 @@ sanitize_mutable_workqueue_attrs(struct workqueue_struct *wq,
 				 struct workqueue_attrs *attrs)
 {
 	if (wq->flags & WQ_HIGHPRI)
-		attrs->nice = HIGHPRI_NICE_LEVEL;
+		attrs->prio = HIGHPRI_PRIORITY;
 }
 
 /* allocate the attrs and pwqs for later installation */
@@ -6704,9 +6707,9 @@ static void pr_cont_pool_info(struct worker_pool *pool)
 	pr_cont(" flags=0x%x", pool->flags);
 	if (pool->flags & POOL_BH)
 		pr_cont(" bh%s",
-			pool->attrs->nice == HIGHPRI_NICE_LEVEL ? "-hi" : "");
+			pool->attrs->prio == HIGHPRI_PRIORITY ? "-hi" : "");
 	else
-		pr_cont(" nice=%d", pool->attrs->nice);
+		pr_cont(" nice=%d", PRIO_TO_NICE(pool->attrs->prio));
 }
 
 static void pr_cont_worker_id(struct worker *worker)
@@ -6715,7 +6718,7 @@ static void pr_cont_worker_id(struct worker *worker)
 
 	if (pool->flags & POOL_BH)
 		pr_cont("bh%s",
-			pool->attrs->nice == HIGHPRI_NICE_LEVEL ? "-hi" : "");
+			pool->attrs->prio == HIGHPRI_PRIORITY ? "-hi" : "");
 	else
 		pr_cont("%d%s", task_pid_nr(worker->task),
 			worker->rescue_wq ? "(RESCUER)" : "");
@@ -7639,7 +7642,8 @@ static ssize_t nice_show(struct device *dev, struct device_attribute *attr,
 	int written;
 
 	mutex_lock(&wq->mutex);
-	written = scnprintf(buf, PAGE_SIZE, "%d\n", wq->attrs->nice);
+	written = scnprintf(buf, PAGE_SIZE, "%d\n",
+			    PRIO_TO_NICE(wq->attrs->prio));
 	mutex_unlock(&wq->mutex);
 
 	return written;
@@ -7665,19 +7669,21 @@ static ssize_t 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, nice = 0;
+
+	if (sscanf(buf, "%d", &nice) != 1 || nice < MIN_NICE || nice > MAX_NICE)
+		return -EINVAL;
 
 	mutex_lock(&wq_pool_mutex);
 
 	attrs = wq_sysfs_prep_attrs(wq);
-	if (!attrs)
+	if (!attrs) {
+		ret = -ENOMEM;
 		goto out_unlock;
+	}
 
-	if (sscanf(buf, "%d", &attrs->nice) == 1 &&
-	    attrs->nice >= MIN_NICE && attrs->nice <= MAX_NICE)
-		ret = apply_workqueue_attrs_locked(wq, attrs);
-	else
-		ret = -EINVAL;
+	attrs->prio = NICE_TO_PRIO(nice);
+	ret = apply_workqueue_attrs_locked(wq, attrs);
 
 out_unlock:
 	mutex_unlock(&wq_pool_mutex);
@@ -8342,13 +8348,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, int prio)
 {
 	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->nice = nice;
+	pool->attrs->prio = prio;
 	pool->attrs->affn_strict = true;
 	pool->node = cpu_to_node(cpu);
 
@@ -8371,7 +8377,7 @@ 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 };
+	int std_prio[NR_STD_WORKER_POOLS] = { DEFAULT_PRIO, HIGHPRI_PRIORITY };
 	void (*irq_work_fns[NR_STD_WORKER_POOLS])(struct irq_work *) =
 		{ bh_pool_kick_normal, bh_pool_kick_highpri };
 	int i, cpu;
@@ -8423,7 +8429,7 @@ 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]);
 			pool->flags |= POOL_BH;
 			init_irq_work(bh_pool_irq_work(pool), irq_work_fns[i]);
 			i++;
@@ -8431,7 +8437,7 @@ void __init workqueue_init_early(void)
 
 		i = 0;
 		for_each_cpu_worker_pool(pool, cpu)
-			init_cpu_worker_pool(pool, cpu, std_nice[i++]);
+			init_cpu_worker_pool(pool, cpu, std_prio[i++]);
 	}
 
 	system_wq = alloc_workqueue("events", WQ_PERCPU | __WQ_DEPRECATED, 0);
diff --git a/tools/workqueue/wq_dump.py b/tools/workqueue/wq_dump.py
index f81d6ee01c79..b3b33a2d1f7c 100644
--- a/tools/workqueue/wq_dump.py
+++ b/tools/workqueue/wq_dump.py
@@ -95,6 +95,20 @@ def wq_attrs(wq):
     except AttributeError:
         return wq.unbound_attrs
 
+def pool_nice(pool):
+    if HAS_PRIO:
+        return pool.attrs.prio.value_() - 120 # PRIO_TO_NICE
+    else:
+        return pool.attrs.nice.value_()
+
+def pool_bh_type(pool):
+    if HAS_PRIO:
+        bh_hi = pool.attrs.prio.value_() == HIGHPRI_PRIORITY.value_()
+    else:
+        bh_hi = pool.attrs.nice.value_() == HIGHPRI_NICE_LEVEL.value_()
+
+    return 'bh-hi' if bh_hi else 'bh'
+
 def worker_current_start(worker):
     try:
         return worker.current_start.value_()
@@ -137,7 +151,6 @@ WQ_AFFN_SYSTEM          = prog['WQ_AFFN_SYSTEM']
 POOL_BH                 = prog['POOL_BH']
 POOL_BH_DRAINING        = prog['POOL_BH_DRAINING']
 POOL_DISASSOCIATED      = prog['POOL_DISASSOCIATED']
-HIGHPRI_NICE_LEVEL      = prog['HIGHPRI_NICE_LEVEL']
 WORKER_IDLE             = prog['WORKER_IDLE']
 
 WQ_NAME_LEN             = prog['WQ_NAME_LEN'].value_()
@@ -145,6 +158,13 @@ cpumask_str_len         = len(cpumask_str(wq_unbound_cpumask))
 hz                      = get_hz()
 jiffies_mask            = (1 << (prog['jiffies'].type_.size * 8)) - 1 if 'jiffies' in prog else 0
 
+HAS_PRIO = prog.type('struct workqueue_attrs').has_member('prio')
+
+if HAS_PRIO:
+    HIGHPRI_PRIORITY = prog['HIGHPRI_PRIORITY']
+else:
+    HIGHPRI_NICE_LEVEL = prog['HIGHPRI_NICE_LEVEL']
+
 print('Affinity Scopes')
 print('===============')
 
@@ -186,13 +206,14 @@ for pi, pool in idr_for_each(worker_pool_idr):
 
 for pi, pool in idr_for_each(worker_pool_idr):
     pool = drgn.Object(prog, 'struct worker_pool', address=pool)
-    print(f'pool[{pi:0{max_pool_id_len}}] flags=0x{pool.flags.value_():02x} ref={pool.refcnt.value_():{max_ref_len}} nice={pool.attrs.nice.value_():3} ', end='')
+    nice = pool_nice(pool)
+    print(f'pool[{pi:0{max_pool_id_len}}] flags=0x{pool.flags.value_():02x} ref={pool.refcnt.value_():{max_ref_len}} nice={nice:3} ', end='')
     print(f'idle/workers={pool.nr_idle.value_():3}/{pool.nr_workers.value_():3} ', end='')
     bh_type = None
     if pool.cpu >= 0:
         print(f'cpu={pool.cpu.value_():3}', end='')
         if pool.flags & POOL_BH:
-            bh_type = 'bh-hi' if pool.attrs.nice == HIGHPRI_NICE_LEVEL else 'bh'
+            bh_type = pool_bh_type(pool)
             print(f' {bh_type}', end='')
             if pool.flags & POOL_BH_DRAINING:
                 print(' draining', end='')
-- 
2.56.0

[PATCH v8 4/6] 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 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.

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

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

We use a minimum priority level since we only care about winning the
contest against normal background CPU load.

With this, a benchmark which measures the GPU submit latency against a
real-time Vulkan queue when faced with CPU saturation, highlights a
dramatic reduction in the 95- and 98-th percentiles:

    us      7.3.0   WQ_RT

   median     30     21
    min        2      3
    max     1038    941
   95pct    2384     49
   98pct    2922     63

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>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> # v9
---
v2:
 * Limit WQ_RTPRI to unbound workqueues and make it have strict CPU
   affinitity. (Tejun)
 * Fixed commit message typos. (AI)
 * Fixed sysfs handling, max_active setting and user modified nice
   application. (AI)

v3:
 * Fix worker->pool null pointer dereference race by moving the
   global decrement to detach_dying_workers().
 * Rebase for upstream changes.

v4:
 * Fixed onion unwind.
 * Moved affinity setting to default attributes.

v5:
 * Dropped global and local limits.
 * Documented in workqueue.rst.
 * Added NR_WQ_ATTRIBUTES.
 * Reverted BH handling changes.

v6:
 * Dropped separate attr->prio in favour of RTPRI_NICE_LEVEL checks.
   (Tejun)
 * Reworked on top of tj/for-7.4.

v7:
 * Convert to attrs->prio encoded analoguous to task_struct->prio.
 * Rename flag to WQ_PRIO and do not re-order enums.
 * Forbid WQ_RT affinity modifications via sysfs.
 * Added wq_dump.py support.

v8:
 * Fix max rt prio confusion.
 * Initialize priority also in alloc_workqueue_attrs_noprof.
 * Restrict rt cpumask modifications.

v9:
 * Reject WQ_RT | __WQ_ORDERED.
 * Fixed spelling in a comment.
 * Align RT_PRIORITY with sched_set_fifo_low.
 * Simplify wq_sysfs_unbound_group_visible and use it to deny
   modifications.
 * Print -rt from pr_cont_pool_info.
 * Keep wq_dump.py more backward compatible.
 * Disallow RT prio change via apply_workqueue_attrs().

v10:
 * Split as separate patch in the series, decoupling from nice to prio
   conversion.
 * Extracted implicit strict CPU affinity to a follow up patch.
 * Update the comments describing sysfs attributes. (Tejun)
---
 Documentation/core-api/workqueue.rst |  7 ++++++
 include/linux/workqueue.h            |  1 +
 kernel/workqueue.c                   | 33 +++++++++++++++++++++++++---
 tools/workqueue/wq_dump.py           | 13 +++++++----
 4 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/Documentation/core-api/workqueue.rst b/Documentation/core-api/workqueue.rst
index bb770f556568..54697907f2b0 100644
--- a/Documentation/core-api/workqueue.rst
+++ b/Documentation/core-api/workqueue.rst
@@ -225,6 +225,13 @@ resources, scheduled and executed.
   each other.  Each maintains its separate pool of workers and
   implements concurrency management among its workers.
 
+``WQ_RT``
+  Real-time priority workqueues must be created as unbound. Their worker threads
+  use the FIFO scheduling policy with the lowest applicable priority.
+
+  To be used sparingly for use cases such as the real-time GPU rendering
+  contexts accessible to privileged clients.
+
 ``WQ_CPU_INTENSIVE``
   Work items of a CPU intensive wq do not contribute to the
   concurrency level.  In other words, runnable CPU intensive
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 985fff90f033..6a220a57a9c6 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -404,6 +404,7 @@ enum wq_flags {
 	 */
 	WQ_POWER_EFFICIENT	= 1 << 7,
 	WQ_PERCPU		= 1 << 8, /* bound to a specific cpu */
+	WQ_RT			= 1 << 9, /* real-time priority, valid only with WQ_UNBOUND */
 
 	__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 0ed93b263389..637240667cf7 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -47,6 +47,7 @@
 #include <linux/jhash.h>
 #include <linux/hashtable.h>
 #include <linux/rculist.h>
+#include <linux/sched/rt.h>
 #include <linux/nodemask.h>
 #include <linux/moduleparam.h>
 #include <linux/uaccess.h>
@@ -127,6 +128,7 @@ enum wq_internal_consts {
 	 */
 	RESCUER_NICE_LEVEL	= MIN_NICE,
 	HIGHPRI_PRIORITY	= NICE_TO_PRIO(MIN_NICE),
+	RT_PRIORITY		= MAX_RT_PRIO - 2, /* sched_set_fifo_low() */
 
 	WQ_NAME_LEN		= 32,
 	WORKER_ID_LEN		= 10 + WQ_NAME_LEN, /* "kworker/R-" + WQ_NAME_LEN */
@@ -3019,7 +3021,11 @@ static struct worker *create_worker(struct worker_pool *pool)
 			goto fail;
 		}
 
-		set_user_nice(worker->task, PRIO_TO_NICE(pool->attrs->prio));
+		if (rt_prio(pool->attrs->prio))
+			sched_set_fifo_low(worker->task);
+		else
+			set_user_nice(worker->task,
+				      PRIO_TO_NICE(pool->attrs->prio));
 
 		kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
 	}
@@ -5717,6 +5723,9 @@ static void
 sanitize_immutable_workqueue_attrs(struct workqueue_struct *wq,
 				   struct workqueue_attrs *attrs)
 {
+	if (wq->flags & WQ_RT)
+		attrs->prio = RT_PRIORITY;
+
 	if (wq->flags & __WQ_ORDERED)
 		attrs->ordered = true;
 }
@@ -6127,6 +6136,11 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
 			return NULL;
 	}
 
+	if (WARN_ON_ONCE((flags & WQ_RT) &&
+			 ((flags & (WQ_HIGHPRI | WQ_UNBOUND | __WQ_ORDERED)) !=
+			 WQ_UNBOUND)))
+		return NULL;
+
 	/* see the comment above the definition of WQ_POWER_EFFICIENT */
 	if ((flags & WQ_POWER_EFFICIENT) && wq_power_efficient)
 		flags = (flags & ~WQ_PERCPU) | WQ_UNBOUND;
@@ -6708,6 +6722,8 @@ static void pr_cont_pool_info(struct worker_pool *pool)
 	if (pool->flags & POOL_BH)
 		pr_cont(" bh%s",
 			pool->attrs->prio == HIGHPRI_PRIORITY ? "-hi" : "");
+	else if (rt_prio(pool->attrs->prio))
+		pr_cont(" rt");
 	else
 		pr_cont(" nice=%d", PRIO_TO_NICE(pool->attrs->prio));
 }
@@ -7564,6 +7580,10 @@ module_param_cb(default_affinity_scope, &wq_affn_dfl_ops, NULL, 0644);
  *  cpumask		RW mask	: bitmask of allowed CPUs for the workers
  *  affinity_scope	RW str  : worker CPU affinity scope (cache, numa, none)
  *  affinity_strict	RW bool : worker CPU affinity is strict
+ *
+ * As an exception, the WQ_RT workqueues export the nice attribute as:
+ *
+ *  nice		RO str	: "rt"
  */
 struct wq_device {
 	struct workqueue_struct		*wq;
@@ -7642,8 +7662,11 @@ static ssize_t nice_show(struct device *dev, struct device_attribute *attr,
 	int written;
 
 	mutex_lock(&wq->mutex);
-	written = scnprintf(buf, PAGE_SIZE, "%d\n",
-			    PRIO_TO_NICE(wq->attrs->prio));
+	if (wq->attrs->prio == RT_PRIORITY)
+		written = scnprintf(buf, PAGE_SIZE, "rt\n");
+	else
+		written = scnprintf(buf, PAGE_SIZE, "%d\n",
+				    PRIO_TO_NICE(wq->attrs->prio));
 	mutex_unlock(&wq->mutex);
 
 	return written;
@@ -7824,6 +7847,10 @@ static umode_t wq_sysfs_unbound_group_visible(struct kobject *kobj,
 	if (!(wq->flags & WQ_UNBOUND))
 		return 0;
 
+	/* Do not allow priority changes for RT workers. */
+	if ((wq->flags & WQ_RT) && (attr == &dev_attr_nice.attr))
+		return 0444;
+
 	return attr->mode;
 }
 
diff --git a/tools/workqueue/wq_dump.py b/tools/workqueue/wq_dump.py
index b3b33a2d1f7c..67186d83e86e 100644
--- a/tools/workqueue/wq_dump.py
+++ b/tools/workqueue/wq_dump.py
@@ -25,7 +25,7 @@ Lists all worker pools indexed by their ID. For each pool:
 
   flags     pool flags in hex
   ref       number of pool_workqueue's associated with this pool
-  nice      nice value of the worker threads in the pool
+  nice      nice value of the worker threads in the pool, or "rt"
   idle      number of idle workers
   workers   number of all workers
   cpu       CPU the pool is associated with (per-cpu pool)
@@ -97,9 +97,13 @@ def wq_attrs(wq):
 
 def pool_nice(pool):
     if HAS_PRIO:
-        return pool.attrs.prio.value_() - 120 # PRIO_TO_NICE
+        prio = pool.attrs.prio.value_()
+        if prio == rt_prio:
+            return 'rt'
+        else:
+            return str(prio - 120) # PRIO_TO_NICE
     else:
-        return pool.attrs.nice.value_()
+        return str(pool.attrs.nice.value_())
 
 def pool_bh_type(pool):
     if HAS_PRIO:
@@ -162,6 +166,7 @@ HAS_PRIO = prog.type('struct workqueue_attrs').has_member('prio')
 
 if HAS_PRIO:
     HIGHPRI_PRIORITY = prog['HIGHPRI_PRIORITY']
+    rt_prio = prog.constant('RT_PRIORITY', filename='kernel/workqueue.c').value_()
 else:
     HIGHPRI_NICE_LEVEL = prog['HIGHPRI_NICE_LEVEL']
 
@@ -207,7 +212,7 @@ for pi, pool in idr_for_each(worker_pool_idr):
 for pi, pool in idr_for_each(worker_pool_idr):
     pool = drgn.Object(prog, 'struct worker_pool', address=pool)
     nice = pool_nice(pool)
-    print(f'pool[{pi:0{max_pool_id_len}}] flags=0x{pool.flags.value_():02x} ref={pool.refcnt.value_():{max_ref_len}} nice={nice:3} ', end='')
+    print(f'pool[{pi:0{max_pool_id_len}}] flags=0x{pool.flags.value_():02x} ref={pool.refcnt.value_():{max_ref_len}} nice={nice:>3} ', end='')
     print(f'idle/workers={pool.nr_idle.value_():3}/{pool.nr_workers.value_():3} ', end='')
     bh_type = None
     if pool.cpu >= 0:
-- 
2.56.0

[PATCH v8 5/6] workqueue: Imply strict CPU affinity for WQ_RT

[Tvrtko Ursulin]

Imply and set strict CPU affinity for WQ_RT workqueues for even lower
worst case execution latency.

With this, a benchmark which measures the GPU submit latency against a
real-time Vulkan queue when faced with CPU saturation, highlights a
dramatic reduction in maximum observed latencies and further reduction in
the median, and 95- and 98-the percentiles:

    us      7.3.0   WQ_RT   +strict_affinity

   median     30     21           11
    min        2      3            2
    max     1038    941          108
   95pct    2384     49           28
   98pct    2922     63           51

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>
---
 Documentation/core-api/workqueue.rst |  5 +++--
 kernel/workqueue.c                   | 13 ++++++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/core-api/workqueue.rst b/Documentation/core-api/workqueue.rst
index 54697907f2b0..d699c3832b19 100644
--- a/Documentation/core-api/workqueue.rst
+++ b/Documentation/core-api/workqueue.rst
@@ -226,8 +226,9 @@ resources, scheduled and executed.
   implements concurrency management among its workers.
 
 ``WQ_RT``
-  Real-time priority workqueues must be created as unbound. Their worker threads
-  use the FIFO scheduling policy with the lowest applicable priority.
+  Real-time priority workqueues must be created as unbound and will be
+  configured with the strict CPU affinity set. Their worker threads use the FIFO
+  scheduling policy with the lowest applicable priority.
 
   To be used sparingly for use cases such as the real-time GPU rendering
   contexts accessible to privileged clients.
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 637240667cf7..26a26831bee7 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5723,8 +5723,12 @@ static void
 sanitize_immutable_workqueue_attrs(struct workqueue_struct *wq,
 				   struct workqueue_attrs *attrs)
 {
-	if (wq->flags & WQ_RT)
+	if (wq->flags & WQ_RT) {
 		attrs->prio = RT_PRIORITY;
+		/* Strict CPU affinity for low latency execution. */
+		attrs->affn_scope = WQ_AFFN_CPU;
+		attrs->affn_strict = true;
+	}
 
 	if (wq->flags & __WQ_ORDERED)
 		attrs->ordered = true;
@@ -7847,8 +7851,11 @@ static umode_t wq_sysfs_unbound_group_visible(struct kobject *kobj,
 	if (!(wq->flags & WQ_UNBOUND))
 		return 0;
 
-	/* Do not allow priority changes for RT workers. */
-	if ((wq->flags & WQ_RT) && (attr == &dev_attr_nice.attr))
+	/* Do not allow priority and affinity changes for RT workers. */
+	if ((wq->flags & WQ_RT) &&
+	     (attr == &dev_attr_nice.attr ||
+	      attr == &dev_attr_affinity_scope.attr ||
+	      attr == &dev_attr_affinity_strict.attr))
 		return 0444;
 
 	return attr->mode;
-- 
2.56.0

[PATCH v8 6/6] 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>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 37 ++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index a343c5180e01..cbff37e5e1d1 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -153,11 +153,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.
 	 *
@@ -3572,8 +3579,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;
@@ -4070,6 +4083,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);
 
@@ -4172,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_RT | 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.56.0

Re: [PATCH v8 6/6] drm/panthor: Create per queue priority workqueues

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/panthor: per-priority unbound submit_wq in panthor_sched_init() stalls on sched->lock and preempts tick_work()
- [Low] drm/panthor: dead submit_wq init and redundant check in group_create_queue()
--

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

drm/panthor: Create per queue priority workqueues

This commit splits the single workqueue shared between the Panthor driver's
internal scheduler logic and the DRM scheduler into separate per-priority
DRM scheduler submit workqueues.

> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index a343c5180e01e..cbff37e5e1d1c 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -153,11 +153,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.
>  	 *
> @@ -3572,8 +3579,14 @@ group_create_queue(struct panthor_group *group,
>  		goto err_free_queue;
>  	}
>  
> -	sched_args.name = queue->name;
> +	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;

[Severity: Low]
As reported by Tejun Heo on RFC v6 [1], this isn't a bug, but
group_create_queue() still initializes sched_args.submit_wq at the top of
the function before overwriting it here:

drivers/gpu/drm/panthor/panthor_sched.c:group_create_queue() {
	struct drm_sched_init_args sched_args = {
		.ops = &panthor_queue_sched_ops,
		.submit_wq = group->ptdev->scheduler->wq,
		...
	};
}

Similarly, the kernel-doc summary line for panthor_scheduler::wq still says
it is used by drm_gpu_scheduler.

In addition, can the bounds and NULL check on group->priority and
group->ptdev->scheduler->submit_wq[group->priority] in group_create_queue()
ever trigger? Its caller panthor_group_create() already rejects
group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT, and panthor_sched_init()
populates all PANTHOR_CSG_PRIORITY_COUNT entries or fails initialization.

Finally, the commit message mentions WQ_RTPRI ("high is server by a
WQ_HIGHPRI instance, while realtime GPU priority is using the newly added
WQ_RTPRI flag"), whereas the flag used in panthor_sched_init() is WQ_RT.
Should those be cleaned up?

[ ... ]

> @@ -4172,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_RT | 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;

[Severity: Medium]
As reported by Tejun Heo on RFC v6 [2], for a WQ_UNBOUND workqueue,
max_active applies across the entire workqueue rather than per CPU. Since
panthor_sched_init() allocates each submit_wq with WQ_UNBOUND and
max_active = 2 (whereas sched->wq previously used max_active = 0), and
group_create_queue() shares that submit_wq across all queues of a given
priority on the device for both drm_gpu_scheduler::work_run_job and
drm_gpu_scheduler::work_free_job, could two in-flight queue_run_job() calls
stalling on sched->lock block all other queues at that priority?

Specifically, in queue_run_job():

drivers/gpu/drm/panthor/panthor_sched.c:queue_run_job() {
	...
	ret = panthor_device_resume_and_get(ptdev);
	if (drm_WARN_ON(&ptdev->base, ret))
		return ERR_PTR(ret);

	mutex_lock(&sched->lock);
	...
}

tick_work() holds sched->lock across firmware round-trip waits in
csgs_upd_ctx_apply_locked(). When two queue_run_job() work items block on
sched->lock (or runtime PM resume), no other work_run_job (including
sync-only jobs with !job->call_info.size that do not take sched->lock) or
work_free_job (queue_free_job()) on that submit_wq can run. Because
submit_wq[PANTHOR_CSG_PRIORITY_LOW] aliases
submit_wq[PANTHOR_CSG_PRIORITY_MEDIUM], two blocked LOW-priority jobs will
also stall MEDIUM-priority job submissions and job completions.

Furthermore, because submit_wq[PANTHOR_CSG_PRIORITY_RT] is allocated with
WQ_RT while panthor_scheduler::wq (which runs tick_work and sync_upd_work)
remains a regular-priority workqueue, can a busy RT submission context
preempt tick_work() on sched->wq and prevent time-slicing and fairness
rotation between RT groups?

[1] https://lore.kernel.org/r/64d59ed5538c6af257058533c880740e@kernel.org
[2] https://lore.kernel.org/r/64d59ed5538c6af257058533c880740e@kernel.org

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