[PATCH v4 0/8] cgroup private data and DRM/i915 integration

[PATCH v4 0/8] cgroup private data and DRM/i915 integration

[Matt Roper]

This is the fourth iteration of the work previously posted here:
  (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
  (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
  (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html

The high level goal of this work is to allow non-cgroup-controller parts
of the kernel (e.g., device drivers) to register their own private
policy data for specific cgroups.  That mechanism is then made use of in
the i915 graphics driver to allow GPU priority to be assigned according
to the cgroup membership of the owning process.  Please see the v1 cover
letter linked above for a more in-depth explanation and justification.

v4 of this series brings large changes to the cgroup core half of the
series and moderate changes to the i915 side.

cgroup core changes:

 * The cgroup private data interface and implementation has changed
   again.  The internal implementation is ida+radixtree based to allow
   much faster lookups than the previous hashtable approach.  Private
   data is now registered and looked up via kref, which should allow
   the same private data to be set on multiple cgroups at the same
   time.  The interface now looks like:

	- cgroup_priv_getkey()
	     Obtain an integer key that will be used to store/lookup
	     cgroup private data.  This only needs to be called once
	     at startup, driver init, etc.; after that the same key
	     is used to store private data against any cgroup

	- cgroup_priv_destroykey(key)
	     Release a private data key and drop references to any
	     private data associated with the key on all cgroups.  This
	     function is very heavy, but will generally only be called
	     by module-based users of the interface when the module is
	     unloaded.

	- cgroup_priv_install(cgrp, key, ref)
	     Store private data for a cgroup under the given key and
	     increment the reference count.

	- cgroup_priv_get(cgrp, key)
	     Take and return a reference to a cgroup's private data
	     associated with the given key.

	- cgroup_priv_get_current(key)
	     Same as cgroup_priv_get, but operates on the current
	     task's cgroup.

	- cgroup_priv_release(cgrp, key)
	     Remove private data from a cgroup and decrement its
	     reference count.

 * Dropped the cgroup_permission() function.  My i915 usage of this
   functionality will take a different approach for determining access
   control.
	
i915 cgroup-based priority changes:

 * cgroup priority offset is now bounded such that (context+cgroup)
   adjustments fall within the range [-0x7fffff,0x7fffff].  This
   only takes 24 bits, leaving several effective priority bits for
   future flags or bookkeeping.

 * Display boost is added as a second cgroup parameter; each cgroup's
   processes can get differing boosts if a display operation is waiting
   on their completion.  If we have non-overlapping cgroup priority
   ranges, this allows a system administrator to decide whether
   workloads from the lower priority cgroup(s) should still jump past
   the workloads in some/all of the higher priority cgroups.

 * Access control for cgroup settings has been changed.  Instead of
   following cgroup filesystem permissions, we now restrict the access
   to either the DRM master or capable(CAP_SYS_RESOURCE).

Cc: Tejun Heo <tj@kernel.org>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Roman Gushchin <guro@fb.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>

Matt Roper (8):
  cgroup: Allow registration and lookup of cgroup private data (v2)
  cgroup: Introduce task_get_dfl_cgroup() (v2)
  cgroup: Introduce cgroup_priv_get_current
  drm/i915: Adjust internal priority definitions
  drm/i915: cgroup integration (v3)
  drm/i915: Introduce 'priority offset' for GPU contexts (v3)
  drm/i915: Introduce per-cgroup display boost setting
  drm/i915: Add context priority & priority offset to debugfs (v2)

 drivers/gpu/drm/i915/Makefile           |   1 +
 drivers/gpu/drm/i915/i915_cgroup.c      | 205 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_debugfs.c     |   3 +
 drivers/gpu/drm/i915/i915_drv.c         |   7 ++
 drivers/gpu/drm/i915/i915_drv.h         |  33 ++++-
 drivers/gpu/drm/i915/i915_gem_context.c |  11 +-
 drivers/gpu/drm/i915/i915_gem_context.h |   9 ++
 drivers/gpu/drm/i915/i915_request.h     |  18 ++-
 drivers/gpu/drm/i915/intel_display.c    |   5 +-
 include/linux/cgroup-defs.h             |   8 ++
 include/linux/cgroup.h                  |  37 ++++++
 include/uapi/drm/i915_drm.h             |  14 +++
 kernel/cgroup/cgroup.c                  | 208 +++++++++++++++++++++++++++++++-
 13 files changed, 545 insertions(+), 14 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_cgroup.c

-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 1/8] cgroup: Allow registration and lookup of cgroup private data (v2)

[Matt Roper]

There are cases where other parts of the kernel may wish to store data
associated with individual cgroups without building a full cgroup
controller.  Let's add interfaces to allow them to register and lookup
this private data for individual cgroups.

A kernel system (e.g., a driver) that wishes to register private data
for a cgroup should start by obtaining a unique private data key by
calling cgroup_priv_getkey().  It may then associate private data
with a cgroup by calling cgroup_priv_install(cgrp, key, ref) where 'ref'
is a pointer to a kref field inside the private data structure.  The
private data may later be looked up by calling cgroup_priv_get(cgrp,
key) to obtain a new reference to the private data.  Private data may be
unregistered via cgroup_priv_release(cgrp, key).

If a cgroup is removed, the reference count for all private data objects
will be decremented.

v2:  Significant overhaul suggested by Tejun, Alexei, and Roman
 - Rework interface to make consumers obtain an ida-based key rather
   than supplying their own arbitrary void*
 - Internal implementation now uses per-cgroup radixtrees which should
   allow much faster lookup than the previous hashtable approach
 - Private data is registered via kref, allowing a single private data
   structure to potentially be assigned to multiple cgroups.

Cc: Tejun Heo <tj@kernel.org>
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Roman Gushchin <guro@fb.com>
Cc: cgroups@vger.kernel.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 include/linux/cgroup-defs.h |   8 ++
 include/linux/cgroup.h      |   7 ++
 kernel/cgroup/cgroup.c      | 185 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 197 insertions(+), 3 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 9f242b876fde..465006274a84 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -427,6 +427,14 @@ struct cgroup {
 	/* used to store eBPF programs */
 	struct cgroup_bpf bpf;
 
+	/*
+	 * cgroup private data registered by other non-controller parts of the
+	 * kernel.  Insertions are protected by privdata_lock, lookups by
+	 * rcu_read_lock().
+	 */
+	struct radix_tree_root privdata;
+	spinlock_t privdata_lock;
+
 	/* ids of the ancestors at each level including self */
 	int ancestor_ids[];
 };
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 473e0c0abb86..63d22dfa00bd 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -833,4 +833,11 @@ static inline void put_cgroup_ns(struct cgroup_namespace *ns)
 		free_cgroup_ns(ns);
 }
 
+/* cgroup private data handling */
+int cgroup_priv_getkey(void (*free)(struct kref *));
+void cgroup_priv_destroykey(int key);
+int cgroup_priv_install(struct cgroup *cgrp, int key, struct kref *ref);
+struct kref *cgroup_priv_get(struct cgroup *cgrp, int key);
+void cgroup_priv_release(struct cgroup *cgrp, int key);
+
 #endif /* _LINUX_CGROUP_H */
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 8cda3bc3ae22..a5e2017c9a94 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -81,10 +81,17 @@ EXPORT_SYMBOL_GPL(css_set_lock);
 #endif
 
 /*
- * Protects cgroup_idr and css_idr so that IDs can be released without
- * grabbing cgroup_mutex.
+ * ID allocator for cgroup private data keys; the ID's allocated here will
+ * be used to index all per-cgroup radix trees.  The radix tree built into
+ * the IDR itself will store a key-specific function to be passed to kref_put.
  */
-static DEFINE_SPINLOCK(cgroup_idr_lock);
+static DEFINE_IDR(cgroup_priv_idr);
+
+/*
+ * Protects cgroup_idr, css_idr, and cgroup_priv_idr so that IDs can be
+ * released without grabbing cgroup_mutex.
+ */
+DEFINE_SPINLOCK(cgroup_idr_lock);
 
 /*
  * Protects cgroup_file->kn for !self csses.  It synchronizes notifications
@@ -1839,6 +1846,8 @@ static void init_cgroup_housekeeping(struct cgroup *cgrp)
 	INIT_LIST_HEAD(&cgrp->cset_links);
 	INIT_LIST_HEAD(&cgrp->pidlists);
 	mutex_init(&cgrp->pidlist_mutex);
+	INIT_RADIX_TREE(&cgrp->privdata, GFP_NOWAIT);
+	spin_lock_init(&cgrp->privdata_lock);
 	cgrp->self.cgroup = cgrp;
 	cgrp->self.flags |= CSS_ONLINE;
 	cgrp->dom_cgrp = cgrp;
@@ -4578,6 +4587,8 @@ static void css_release_work_fn(struct work_struct *work)
 		container_of(work, struct cgroup_subsys_state, destroy_work);
 	struct cgroup_subsys *ss = css->ss;
 	struct cgroup *cgrp = css->cgroup;
+	struct radix_tree_iter iter;
+	void **slot;
 
 	mutex_lock(&cgroup_mutex);
 
@@ -4617,6 +4628,12 @@ static void css_release_work_fn(struct work_struct *work)
 					 NULL);
 
 		cgroup_bpf_put(cgrp);
+
+		/* Drop reference on any private data */
+		rcu_read_lock();
+		radix_tree_for_each_slot(slot, &cgrp->privdata, &iter, 0)
+			cgroup_priv_release(cgrp, iter.index);
+		rcu_read_unlock();
 	}
 
 	mutex_unlock(&cgroup_mutex);
@@ -5932,3 +5949,165 @@ static int __init cgroup_sysfs_init(void)
 }
 subsys_initcall(cgroup_sysfs_init);
 #endif /* CONFIG_SYSFS */
+
+/**
+ * cgroup_priv_getkey - obtain a new cgroup_priv lookup key
+ * @free: Function to release private data associated with this key
+ *
+ * Allows non-controller kernel subsystems to register a new key that will
+ * be used to insert/lookup private data associated with individual cgroups.
+ * Private data lookup tables are implemented as per-cgroup radix trees.
+ *
+ * Returns:
+ * A positive integer lookup key if successful, or a negative error code
+ * on failure (e.g., if ID allocation fails).
+ */
+int
+cgroup_priv_getkey(void (*free)(struct kref *))
+{
+	int ret;
+
+	WARN_ON(!free);
+
+	idr_preload(GFP_KERNEL);
+	spin_lock_bh(&cgroup_idr_lock);
+	ret = idr_alloc(&cgroup_priv_idr, free, 1, 0, GFP_NOWAIT);
+	spin_unlock_bh(&cgroup_idr_lock);
+	idr_preload_end();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cgroup_priv_getkey);
+
+/**
+ * cgroup_priv_destroykey - release a cgroup_priv key
+ * @key: Private data key to be released
+ *
+ * Removes a cgroup private data key and all private data associated with this
+ * key in any cgroup.  This is a heavy operation that will take cgroup_mutex.
+ */
+void
+cgroup_priv_destroykey(int key)
+{
+	struct cgroup *cgrp;
+
+	WARN_ON(key == 0);
+
+	mutex_lock(&cgroup_mutex);
+	cgroup_for_each_live_child(cgrp, &cgrp_dfl_root.cgrp)
+		cgroup_priv_release(cgrp, key);
+	idr_remove(&cgroup_priv_idr, key);
+	mutex_unlock(&cgroup_mutex);
+}
+EXPORT_SYMBOL_GPL(cgroup_priv_destroykey);
+
+/**
+ * cgroup_priv_install - install new cgroup private data
+ * @cgrp: cgroup to store private data for
+ * @key: key uniquely identifying kernel owner of private data
+ * @data: pointer to kref field of private data structure
+ *
+ * Allows non-controller kernel subsystems to register their own private data
+ * associated with a cgroup.  This will often be used by drivers which wish to
+ * track their own per-cgroup data without building a full cgroup controller.
+ *
+ * The caller is responsible for ensuring that no private data already exists
+ * for the given key.
+ *
+ * Registering cgroup private data with this function will increment the
+ * reference count for the private data structure.  If the cgroup is removed,
+ * the reference count will be decremented, allowing the private data to
+ * be freed if there are no other outstanding references.
+ *
+ * Returns:
+ * 0 on success, otherwise a negative error code on failure.
+ */
+int
+cgroup_priv_install(struct cgroup *cgrp, int key, struct kref *ref)
+{
+	int ret;
+
+	WARN_ON(cgrp->root != &cgrp_dfl_root);
+	WARN_ON(key == 0);
+
+	kref_get(ref);
+
+	ret = radix_tree_preload(GFP_KERNEL);
+	if (ret)
+		return ret;
+
+	spin_lock_bh(&cgrp->privdata_lock);
+	ret = radix_tree_insert(&cgrp->privdata, key, ref);
+	spin_unlock_bh(&cgrp->privdata_lock);
+	radix_tree_preload_end();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cgroup_priv_install);
+
+/**
+ * cgroup_priv_get - looks up cgroup private data
+ * @cgrp: cgroup to lookup private data for
+ * @key: key uniquely identifying owner of private data to lookup
+ *
+ * Looks up the cgroup private data associated with a key.  The private
+ * data's reference count is incremented and a pointer to its kref field
+ * is returned to the caller (which can use container_of()) to obtain
+ * the private data itself.
+ *
+ * Returns:
+ * A pointer to the private data's kref field, or NULL if no private data has
+ * been registered.
+ */
+struct kref *
+cgroup_priv_get(struct cgroup *cgrp, int key)
+{
+	struct kref *ref;
+
+	WARN_ON(cgrp->root != &cgrp_dfl_root);
+	WARN_ON(key == 0);
+
+	rcu_read_lock();
+
+	ref = radix_tree_lookup(&cgrp->privdata, key);
+	if (ref)
+		kref_get(ref);
+
+	rcu_read_unlock();
+
+	return ref;
+}
+EXPORT_SYMBOL_GPL(cgroup_priv_get);
+
+/**
+ * cgroup_priv_free - free cgroup private data
+ * @cgrp: cgroup to release private data for
+ * @key: key uniquely identifying owner of private data to free
+ *
+ * Removes private data associated with the given key from a cgroup's internal
+ * table and decrements the reference count for the private data removed,
+ * allowing it to freed if no other references exist.
+ */
+void
+cgroup_priv_release(struct cgroup *cgrp, int key)
+{
+	struct kref *ref;
+	void (*free)(struct kref *);
+
+	WARN_ON(cgrp->root != &cgrp_dfl_root);
+	WARN_ON(key == 0);
+
+	rcu_read_lock();
+
+	free = idr_find(&cgroup_priv_idr, key);
+	WARN_ON(!free);
+
+	spin_lock_bh(&cgrp->privdata_lock);
+	ref = radix_tree_delete(&cgrp->privdata, key);
+	spin_unlock_bh(&cgrp->privdata_lock);
+	if (ref)
+		kref_put(ref, free);
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(cgroup_priv_release);
-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v4 1/8] cgroup: Allow registration and lookup of cgroup private data (v2)

[kbuild test robot]

Hi Matt,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20180309]
[also build test WARNING on v4.16-rc6]
[cannot apply to v4.16-rc4 v4.16-rc3 v4.16-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Roper/cgroup-private-data-and-DRM-i915-integration/20180319-071752
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> kernel/cgroup/cgroup.c:94:1: sparse: symbol 'cgroup_idr_lock' was not declared. Should it be static?
>> kernel/cgroup/cgroup.c:4655:17: sparse: incorrect type in assignment (different address spaces) @@    expected void **slot @@    got void [noderef] <avoid **slot @@
   kernel/cgroup/cgroup.c:4655:17:    expected void **slot
   kernel/cgroup/cgroup.c:4655:17:    got void [noderef] <asn:4>**
>> kernel/cgroup/cgroup.c:4655:17: sparse: incorrect type in assignment (different address spaces) @@    expected void **slot @@    got void [noderef] <avoid **slot @@
   kernel/cgroup/cgroup.c:4655:17:    expected void **slot
   kernel/cgroup/cgroup.c:4655:17:    got void [noderef] <asn:4>**
>> kernel/cgroup/cgroup.c:4655:17: sparse: incorrect type in argument 1 (different address spaces) @@    expected void [noderef] <asn:4>**slot @@    got sn:4>**slot @@
   kernel/cgroup/cgroup.c:4655:17:    expected void [noderef] <asn:4>**slot
   kernel/cgroup/cgroup.c:4655:17:    got void **slot
>> kernel/cgroup/cgroup.c:4655:17: sparse: incorrect type in assignment (different address spaces) @@    expected void **slot @@    got void [noderef] <avoid **slot @@
   kernel/cgroup/cgroup.c:4655:17:    expected void **slot
   kernel/cgroup/cgroup.c:4655:17:    got void [noderef] <asn:4>**
   kernel/cgroup/cgroup.c:2648:20: sparse: context imbalance in 'cgroup_procs_write_start' - wrong count at exit
   kernel/cgroup/cgroup.c:2704:9: sparse: context imbalance in 'cgroup_procs_write_finish' - wrong count at exit
   kernel/cgroup/cgroup.c:2815:9: sparse: context imbalance in 'cgroup_lock_and_drain_offline' - wrong count at exit
   kernel/cgroup/cgroup.c:4375:16: sparse: context imbalance in 'cgroup_procs_write' - wrong count at exit
   kernel/cgroup/cgroup.c:4416:16: sparse: context imbalance in 'cgroup_threads_write' - wrong count at exit

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH] cgroup: cgroup_idr_lock can be static

[kbuild test robot]

Fixes: d6de4e7e9e60 ("cgroup: Allow registration and lookup of cgroup private data (v2)")
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 cgroup.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index c778830..9af6086 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -91,7 +91,7 @@ static DEFINE_IDR(cgroup_priv_idr);
  * Protects cgroup_idr, css_idr, and cgroup_priv_idr so that IDs can be
  * released without grabbing cgroup_mutex.
  */
-DEFINE_SPINLOCK(cgroup_idr_lock);
+static DEFINE_SPINLOCK(cgroup_idr_lock);
 
 /*
  * Protects cgroup_file->kn for !self csses.  It synchronizes notifications
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 2/8] cgroup: Introduce task_get_dfl_cgroup() (v2)

[Matt Roper]

Wraps task_dfl_cgroup() to also take a reference to the cgroup.

v2:
 - Eliminate cgroup_mutex and make lighter-weight (Tejun)

Cc: Tejun Heo <tj@kernel.org>
Cc: cgroups@vger.kernel.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 include/linux/cgroup.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 63d22dfa00bd..74b435f913c1 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -527,6 +527,35 @@ static inline struct cgroup *task_dfl_cgroup(struct task_struct *task)
 	return task_css_set(task)->dfl_cgrp;
 }
 
+/**
+ * task_get_dfl_cgroup() - find and get the cgroup for a task
+ * @task: the target task
+ *
+ * Find the cgroup in the v2 hierarchy that a task belongs to, increment its
+ * reference count, and return it.
+ *
+ * Returns:
+ * The appropriate cgroup from the default hierarchy.
+ */
+static inline struct cgroup *
+task_get_dfl_cgroup(struct task_struct *task)
+{
+	struct cgroup *cgrp;
+
+	rcu_read_lock();
+	while (true) {
+		cgrp = task_dfl_cgroup(task);
+
+		if (likely(cgroup_tryget(cgrp)))
+			break;
+
+		cpu_relax();
+	}
+	rcu_read_unlock();
+
+	return cgrp;
+}
+
 static inline struct cgroup *cgroup_parent(struct cgroup *cgrp)
 {
 	struct cgroup_subsys_state *parent_css = cgrp->self.parent;
-- 
2.14.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 3/8] cgroup: Introduce cgroup_priv_get_current

[Matt Roper]

Getting cgroup private data for the current process' cgroup is such a
common pattern that we should add a convenience wrapper for it.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 include/linux/cgroup.h |  1 +
 kernel/cgroup/cgroup.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 74b435f913c1..64d3dc45efd0 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -867,6 +867,7 @@ int cgroup_priv_getkey(void (*free)(struct kref *));
 void cgroup_priv_destroykey(int key);
 int cgroup_priv_install(struct cgroup *cgrp, int key, struct kref *ref);
 struct kref *cgroup_priv_get(struct cgroup *cgrp, int key);
+struct kref *cgroup_priv_get_current(int key);
 void cgroup_priv_release(struct cgroup *cgrp, int key);
 
 #endif /* _LINUX_CGROUP_H */
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index a5e2017c9a94..56ed910beb8a 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6079,6 +6079,29 @@ cgroup_priv_get(struct cgroup *cgrp, int key)
 }
 EXPORT_SYMBOL_GPL(cgroup_priv_get);
 
+/**
+ * cgroup_priv_get_current - looks up cgroup private data for current task
+ * @key: key uniquely identifying owner of private data to lookup
+ *
+ * Convenience function that performs cgroup_priv_get() on the cgroup that owns
+ * %current.
+ *
+ * Returns:
+ * A pointer to the private data's kref field, or NULL if no private data has
+ * been registered.
+ */
+struct kref *
+cgroup_priv_get_current(int key)
+{
+	struct cgroup *cgrp = task_get_dfl_cgroup(current);
+	struct kref *ref = cgroup_priv_get(cgrp, key);
+
+	cgroup_put(cgrp);
+
+	return ref;
+}
+EXPORT_SYMBOL_GPL(cgroup_priv_get_current);
+
 /**
  * cgroup_priv_free - free cgroup private data
  * @cgrp: cgroup to release private data for
-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 4/8] drm/i915: Adjust internal priority definitions

[Matt Roper]

In preparation for adding cgroup-based priority adjustments, let's
define the driver's priority values a little more clearly.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  1 -
 drivers/gpu/drm/i915/i915_gem_context.c |  4 ++--
 drivers/gpu/drm/i915/i915_request.h     | 13 ++++++++++---
 drivers/gpu/drm/i915/intel_display.c    |  2 +-
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e27ba8fb64e6..6ce7d0662bb6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3150,7 +3150,6 @@ int i915_gem_object_wait(struct drm_i915_gem_object *obj,
 int i915_gem_object_wait_priority(struct drm_i915_gem_object *obj,
 				  unsigned int flags,
 				  int priority);
-#define I915_PRIORITY_DISPLAY I915_PRIORITY_MAX
 
 int __must_check
 i915_gem_object_set_to_wc_domain(struct drm_i915_gem_object *obj, bool write);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5cfac0255758..551c1d75fe24 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -474,7 +474,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
 	ida_init(&dev_priv->contexts.hw_ida);
 
 	/* lowest priority; idle task */
-	ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
+	ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_IDLE);
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context\n");
 		return PTR_ERR(ctx);
@@ -488,7 +488,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
 
 	/* highest priority; preempting task */
 	if (needs_preempt_context(dev_priv)) {
-		ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
+		ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_PREEMPT);
 		if (!IS_ERR(ctx))
 			dev_priv->preempt_context = ctx;
 		else
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 7d6eb82eeb91..72b13fc2b72b 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -78,12 +78,19 @@ struct i915_priotree {
 	int priority;
 };
 
+/*
+ * Userspace can only assign priority values of [-1023,1023] via context param,
+ * but the effective priority value can fall in a larger range once we add in
+ * a cgroup-provided offset.
+ */
 enum {
-	I915_PRIORITY_MIN = I915_CONTEXT_MIN_USER_PRIORITY - 1,
 	I915_PRIORITY_NORMAL = I915_CONTEXT_DEFAULT_PRIORITY,
-	I915_PRIORITY_MAX = I915_CONTEXT_MAX_USER_PRIORITY + 1,
+	I915_PRIORITY_DEFAULT_DISPBOOST = I915_CONTEXT_MAX_USER_PRIORITY + 1,
 
-	I915_PRIORITY_INVALID = INT_MIN
+	/* Special case priority values */
+	I915_PRIORITY_INVALID = INT_MIN,
+	I915_PRIORITY_IDLE = INT_MIN + 1,
+	I915_PRIORITY_PREEMPT = INT_MAX,
 };
 
 struct i915_capture_list {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3e7ab75e1b41..b053a21f682b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12783,7 +12783,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 
 	ret = intel_plane_pin_fb(to_intel_plane_state(new_state));
 
-	i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
+	i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DEFAULT_DISPBOOST);
 
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 	i915_gem_object_unpin_pages(obj);
-- 
2.14.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 5/8] drm/i915: cgroup integration (v3)

[Matt Roper]

Introduce a new DRM_IOCTL_I915_CGROUP_SETPARAM ioctl that will allow
userspace to set i915-specific parameters for individual cgroups.  i915
cgroup data will be registered and later looked up via the new
cgroup_priv infrastructure.

v2:
 - Large rebase/rewrite for new cgroup_priv interface
v3:
 - Another rebase/rewrite for ida-based keys and kref-based storage
 - Access control no longer follows cgroup filesystem permissions;
   instead we restrict access to either DRM master or
   capable(CAP_SYS_RESOURCE).

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/Makefile      |   1 +
 drivers/gpu/drm/i915/i915_cgroup.c | 140 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.c    |   7 ++
 drivers/gpu/drm/i915/i915_drv.h    |  25 +++++++
 include/uapi/drm/i915_drm.h        |  12 ++++
 5 files changed, 185 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/i915_cgroup.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 552e43e9663f..26031185cf0e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -48,6 +48,7 @@ i915-y := i915_drv.o \
 i915-$(CONFIG_COMPAT)   += i915_ioc32.o
 i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o intel_pipe_crc.o
 i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o
+i915-$(CONFIG_CGROUPS) += i915_cgroup.o
 
 # GEM code
 i915-y += i915_cmd_parser.o \
diff --git a/drivers/gpu/drm/i915/i915_cgroup.c b/drivers/gpu/drm/i915/i915_cgroup.c
new file mode 100644
index 000000000000..eaa540b65efd
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_cgroup.c
@@ -0,0 +1,140 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * i915_cgroup.c - Linux cgroups integration for i915
+ *
+ * Copyright (C) 2018 Intel Corporation
+ */
+
+#include <linux/cgroup.h>
+
+#include "i915_drv.h"
+
+struct i915_cgroup_data {
+	struct kref ref;
+};
+
+static inline struct i915_cgroup_data *
+cgrp_ref_to_i915(struct kref *ref)
+{
+	return container_of(ref, struct i915_cgroup_data, ref);
+}
+
+static void
+i915_cgroup_free(struct kref *ref)
+{
+	struct i915_cgroup_data *priv;
+
+	priv = cgrp_ref_to_i915(ref);
+	kfree(priv);
+}
+
+int
+i915_cgroup_init(struct drm_i915_private *dev_priv)
+{
+	int ret = 0;
+
+	dev_priv->cgroup_priv_key = cgroup_priv_getkey(i915_cgroup_free);
+	if (dev_priv->cgroup_priv_key < 0) {
+		DRM_DEBUG_DRIVER("Failed to get a cgroup private data key\n");
+		ret = dev_priv->cgroup_priv_key;
+	}
+
+	mutex_init(&dev_priv->cgroup_lock);
+
+	return ret;
+}
+
+void
+i915_cgroup_shutdown(struct drm_i915_private *dev_priv)
+{
+	cgroup_priv_destroykey(dev_priv->cgroup_priv_key);
+}
+
+/*
+ * Return i915 cgroup private data, creating and registering it if one doesn't
+ * already exist for this cgroup.
+ */
+__maybe_unused
+static struct i915_cgroup_data *
+get_or_create_cgroup_data(struct drm_i915_private *dev_priv,
+			  struct cgroup *cgrp)
+{
+	struct kref *ref;
+	struct i915_cgroup_data *priv;
+
+	mutex_lock(&dev_priv->cgroup_lock);
+
+	ref = cgroup_priv_get(cgrp, dev_priv->cgroup_priv_key);
+	if (ref) {
+		priv = cgrp_ref_to_i915(ref);
+	} else {
+		priv = kzalloc(sizeof *priv, GFP_KERNEL);
+		if (!priv) {
+			priv = ERR_PTR(-ENOMEM);
+			goto out;
+		}
+
+		kref_init(&priv->ref);
+		cgroup_priv_install(cgrp, dev_priv->cgroup_priv_key,
+				    &priv->ref);
+	}
+
+out:
+	mutex_unlock(&dev_priv->cgroup_lock);
+
+	return priv;
+}
+
+/**
+ * i915_cgroup_setparam_ioctl - ioctl to alter i915 settings for a cgroup
+ * @dev: DRM device
+ * @data: data pointer for the ioctl
+ * @file_priv: DRM file handle for the ioctl call
+ *
+ * Allows i915-specific parameters to be set for a Linux cgroup.
+ */
+int
+i915_cgroup_setparam_ioctl(struct drm_device *dev,
+			   void *data,
+			   struct drm_file *file)
+{
+	struct drm_i915_cgroup_param *req = data;
+	struct cgroup *cgrp;
+	int ret;
+
+	/* We don't actually support any flags yet. */
+	if (req->flags) {
+		DRM_DEBUG_DRIVER("Invalid flags\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Make sure the file descriptor really is a cgroup fd and is on the
+	 * v2 hierarchy.
+	 */
+	cgrp = cgroup_get_from_fd(req->cgroup_fd);
+	if (IS_ERR(cgrp)) {
+		DRM_DEBUG_DRIVER("Invalid cgroup file descriptor\n");
+		return PTR_ERR(cgrp);
+	}
+
+	/*
+	 * Access control:  For now we grant access via CAP_SYS_RESOURCE _or_
+	 * DRM master status.
+	 */
+	if (!capable(CAP_SYS_RESOURCE) && !drm_is_current_master(file)) {
+		DRM_DEBUG_DRIVER("Insufficient permissions to adjust i915 cgroup settings\n");
+		goto out;
+	}
+
+	switch (req->param) {
+	default:
+		DRM_DEBUG_DRIVER("Invalid cgroup parameter %lld\n", req->param);
+		ret = -EINVAL;
+	}
+
+out:
+	cgroup_put(cgrp);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3df5193487f3..99b571cb7ef8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1407,6 +1407,10 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	intel_runtime_pm_put(dev_priv);
 
+	ret = i915_cgroup_init(dev_priv);
+	if (ret < 0)
+		goto out_cleanup_hw;
+
 	i915_welcome_messages(dev_priv);
 
 	return 0;
@@ -1433,6 +1437,8 @@ void i915_driver_unload(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 
+	i915_cgroup_shutdown(dev_priv);
+
 	i915_driver_unregister(dev_priv);
 
 	if (i915_gem_suspend(dev_priv))
@@ -2834,6 +2840,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_CGROUP_SETPARAM, i915_cgroup_setparam_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6ce7d0662bb6..b1862fe5e1a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1738,6 +1738,10 @@ struct drm_i915_private {
 
 	struct intel_ppat ppat;
 
+	/* cgroup private data */
+	int cgroup_priv_key;
+	struct mutex cgroup_lock;
+
 	/* Kernel Modesetting */
 
 	struct intel_crtc *plane_to_crtc_mapping[I915_MAX_PIPES];
@@ -2678,6 +2682,27 @@ intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *dev_priv)
 int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
 				int enable_ppgtt);
 
+/* i915_cgroup.c */
+#ifdef CONFIG_CGROUPS
+int i915_cgroup_init(struct drm_i915_private *dev_priv);
+int i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
+void i915_cgroup_shutdown(struct drm_i915_private *dev_priv);
+#else
+static inline int
+i915_cgroup_init(struct drm_i915_private *dev_priv)
+{
+	return 0;
+}
+static inline void i915_cgroup_shutdown(struct drm_i915_private *dev_priv) {}
+static inline int
+i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data,
+			   struct drm_file *file)
+{
+	return -EINVAL;
+}
+#endif
+
 /* i915_drv.c */
 void __printf(3, 4)
 __i915_printk(struct drm_i915_private *dev_priv, const char *level,
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7f5634ce8e88..735128fa61de 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -319,6 +319,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_PERF_ADD_CONFIG	0x37
 #define DRM_I915_PERF_REMOVE_CONFIG	0x38
 #define DRM_I915_QUERY			0x39
+#define DRM_I915_CGROUP_SETPARAM	0x3a
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -377,6 +378,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_PERF_ADD_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
 #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64)
 #define DRM_IOCTL_I915_QUERY			DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query)
+#define DRM_IOCTL_I915_CGROUP_SETPARAM		DRM_IOW(DRM_COMMAND_BASE + DRM_I915_CGROUP_SETPARAM, struct drm_i915_cgroup_param)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1717,6 +1719,16 @@ struct drm_i915_query_topology_info {
 	__u8 data[];
 };
 
+/**
+ * Structure to set i915 parameter on a cgroup.
+ */
+struct drm_i915_cgroup_param {
+	__s32 cgroup_fd;
+	__u32 flags;
+	__u64 param;
+	__s64 value;
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.14.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 6/8] drm/i915: Introduce 'priority offset' for GPU contexts (v3)

[Matt Roper]

There are cases where a system integrator may wish to raise/lower the
priority of GPU workloads being submitted by specific OS process(es),
independently of how the software self-classifies its own priority.
Exposing "priority offset" as an i915-specific cgroup parameter will
enable such system-level configuration.

Normally GPU contexts start with a priority value of 0
(I915_CONTEXT_DEFAULT_PRIORITY) and then may be adjusted up/down from
there via other mechanisms.  We'd like to provide a system-level input
to the priority decision that will be taken into consideration, even
when userspace later attempts to set an absolute priority value via
I915_CONTEXT_PARAM_PRIORITY.  The priority offset introduced here
provides a base value that will always be added to (or subtracted from)
the software's self-assigned priority value.

This patch makes priority offset a cgroup-specific value; contexts will
be created with a priority offset based on the cgroup membership of the
process creating the context at the time the context is created.  Note
that priority offset is assigned at context creation time; migrating a
process to a different cgroup or changing the offset associated with a
cgroup will only affect new context creation and will not alter the
behavior of existing contexts previously created by the process.

v2:
 - Rebase onto new cgroup_priv API
 - Use current instead of drm_file->pid to determine which process to
   lookup priority for. (Chris)
 - Don't forget to subtract priority offset in context_getparam ioctl to
   make it match setparam behavior. (Chris)

v3:
 - Rebase again onto new idr/kref-based cgroup_priv API
 - Bound priority offset such that effective priority from settings
   on context + cgroup fall within [-0x7fffff, 0x7fffff]. (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_cgroup.c      | 52 +++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h         | 15 +++++-----
 drivers/gpu/drm/i915/i915_gem_context.c |  7 +++--
 drivers/gpu/drm/i915/i915_gem_context.h |  9 ++++++
 drivers/gpu/drm/i915/i915_request.h     |  4 +++
 include/uapi/drm/i915_drm.h             |  1 +
 6 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cgroup.c b/drivers/gpu/drm/i915/i915_cgroup.c
index eaa540b65efd..b58b243aef20 100644
--- a/drivers/gpu/drm/i915/i915_cgroup.c
+++ b/drivers/gpu/drm/i915/i915_cgroup.c
@@ -10,6 +10,8 @@
 #include "i915_drv.h"
 
 struct i915_cgroup_data {
+	int priority_offset;
+
 	struct kref ref;
 };
 
@@ -54,7 +56,6 @@ i915_cgroup_shutdown(struct drm_i915_private *dev_priv)
  * Return i915 cgroup private data, creating and registering it if one doesn't
  * already exist for this cgroup.
  */
-__maybe_unused
 static struct i915_cgroup_data *
 get_or_create_cgroup_data(struct drm_i915_private *dev_priv,
 			  struct cgroup *cgrp)
@@ -98,9 +99,11 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev,
 			   void *data,
 			   struct drm_file *file)
 {
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_cgroup_param *req = data;
 	struct cgroup *cgrp;
-	int ret;
+	struct i915_cgroup_data *cgrpdata;
+	int ret = 0;
 
 	/* We don't actually support any flags yet. */
 	if (req->flags) {
@@ -127,14 +130,59 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
+	cgrpdata = get_or_create_cgroup_data(dev_priv, cgrp);
+	if (IS_ERR(cgrpdata)) {
+		ret = PTR_ERR(cgrpdata);
+		goto out;
+	}
+
 	switch (req->param) {
+	case I915_CGROUP_PARAM_PRIORITY_OFFSET:
+		if (req->value + I915_CONTEXT_MAX_USER_PRIORITY <= I915_PRIORITY_MAX &&
+		    req->value - I915_CONTEXT_MIN_USER_PRIORITY >= I915_PRIORITY_MIN) {
+			DRM_DEBUG_DRIVER("Setting cgroup priority offset to %lld\n",
+					 req->value);
+			cgrpdata->priority_offset = req->value;
+		} else {
+			DRM_DEBUG_DRIVER("Invalid cgroup priority offset %lld\n",
+					 req->value);
+			ret = -EINVAL;
+		}
+		break;
+
 	default:
 		DRM_DEBUG_DRIVER("Invalid cgroup parameter %lld\n", req->param);
 		ret = -EINVAL;
 	}
 
+	kref_put(&cgrpdata->ref, i915_cgroup_free);
+
 out:
 	cgroup_put(cgrp);
 
 	return ret;
 }
+
+/*
+ * Generator for simple getter functions that look up a cgroup private data
+ * field for the current task's cgroup.  It's safe to call these before
+ * a cgroup private data key has been registered; they'll just return the
+ * default value in that case.
+ */
+#define CGROUP_GET(name, field, def) \
+int i915_cgroup_get_current_##name(struct drm_i915_private *dev_priv)	\
+{									\
+	struct kref *ref;						\
+	int val = def;							\
+	if (!dev_priv->cgroup_priv_key) return def;			\
+	ref = cgroup_priv_get_current(dev_priv->cgroup_priv_key);	\
+	if (ref) {							\
+		val = cgrp_ref_to_i915(ref)->field;			\
+		kref_put(ref, i915_cgroup_free);			\
+	}								\
+	return val;							\
+}
+
+CGROUP_GET(prio_offset, priority_offset, 0)
+
+#undef CGROUP_GET
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b1862fe5e1a6..fd5629593f4b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2688,18 +2688,19 @@ int i915_cgroup_init(struct drm_i915_private *dev_priv);
 int i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data,
 			       struct drm_file *file);
 void i915_cgroup_shutdown(struct drm_i915_private *dev_priv);
+int i915_cgroup_get_current_prio_offset(struct drm_i915_private *dev_priv);
 #else
-static inline int
-i915_cgroup_init(struct drm_i915_private *dev_priv)
+static inline void i915_cgroup_init(struct drm_i915_private *dev_priv) {}
+static inline void i915_cgroup_shutdown(struct drm_i915_private *dev_priv) {}
+static inline int i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data,
+					     struct drm_file *file)
 {
-	return 0;
+	return -EINVAL;
 }
-static inline void i915_cgroup_shutdown(struct drm_i915_private *dev_priv) {}
 static inline int
-i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data,
-			   struct drm_file *file)
+i915_cgroup_get_current_prio_offset(struct drm_i915_private *dev_priv)
 {
-	return -EINVAL;
+	return 0;
 }
 #endif
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 551c1d75fe24..306886c1add8 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -280,7 +280,8 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 	kref_init(&ctx->ref);
 	list_add_tail(&ctx->link, &dev_priv->contexts.list);
 	ctx->i915 = dev_priv;
-	ctx->priority = I915_PRIORITY_NORMAL;
+	ctx->priority_offset = i915_cgroup_get_current_prio_offset(dev_priv);
+	ctx->priority = I915_PRIORITY_NORMAL + ctx->priority_offset;
 
 	INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
 	INIT_LIST_HEAD(&ctx->handles_list);
@@ -747,7 +748,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 		args->value = i915_gem_context_is_bannable(ctx);
 		break;
 	case I915_CONTEXT_PARAM_PRIORITY:
-		args->value = ctx->priority;
+		args->value = ctx->priority - ctx->priority_offset;
 		break;
 	default:
 		ret = -EINVAL;
@@ -820,7 +821,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 				 !capable(CAP_SYS_NICE))
 				ret = -EPERM;
 			else
-				ctx->priority = priority;
+				ctx->priority = priority + ctx->priority_offset;
 		}
 		break;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 7854262ddfd9..c3b4fb54fbb6 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -150,6 +150,15 @@ struct i915_gem_context {
 	 */
 	int priority;
 
+	/**
+	 * @priority_offset: priority offset
+	 *
+	 * A value, configured via cgroup, that sets the starting priority
+	 * of the context.  Any priority set explicitly via context parameter
+	 * will be added to the priority offset.
+	 */
+	int priority_offset;
+
 	/** ggtt_offset_bias: placement restriction for context objects */
 	u32 ggtt_offset_bias;
 
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 72b13fc2b72b..cf7a7147daf3 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -87,6 +87,10 @@ enum {
 	I915_PRIORITY_NORMAL = I915_CONTEXT_DEFAULT_PRIORITY,
 	I915_PRIORITY_DEFAULT_DISPBOOST = I915_CONTEXT_MAX_USER_PRIORITY + 1,
 
+	/* Range reachable by combining user priority + cgroup offset */
+	I915_PRIORITY_MAX = 0x7fffff,
+	I915_PRIORITY_MIN = -I915_PRIORITY_MAX,
+
 	/* Special case priority values */
 	I915_PRIORITY_INVALID = INT_MIN,
 	I915_PRIORITY_IDLE = INT_MIN + 1,
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 735128fa61de..6b70f46d224e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1726,6 +1726,7 @@ struct drm_i915_cgroup_param {
 	__s32 cgroup_fd;
 	__u32 flags;
 	__u64 param;
+#define I915_CGROUP_PARAM_PRIORITY_OFFSET	0x1
 	__s64 value;
 };
 
-- 
2.14.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 7/8] drm/i915: Introduce per-cgroup display boost setting

[Matt Roper]

Usually display-boosted contexts get treated as
I915_CONTEXT_MAX_USER_PRIORITY+1, which prioritizes them above regular
GPU contexts.  Now that we allow a much larger range of effective
priority values via per-cgroup priority offsets, a system administrator
may want more detailed control over how much a process should be boosted
by display operations (i.e., can a context from a cgroup with a low
priority offset still be display boosted above contexts from a cgroup
with a much higher priority offset?  or are come cgroups more important
than maintaining display rate?).

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_cgroup.c   | 17 +++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h      |  6 ++++++
 drivers/gpu/drm/i915/i915_request.h  |  1 +
 drivers/gpu/drm/i915/intel_display.c |  5 +++--
 include/uapi/drm/i915_drm.h          |  1 +
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cgroup.c b/drivers/gpu/drm/i915/i915_cgroup.c
index b58b243aef20..868d455ffeb6 100644
--- a/drivers/gpu/drm/i915/i915_cgroup.c
+++ b/drivers/gpu/drm/i915/i915_cgroup.c
@@ -11,6 +11,7 @@
 
 struct i915_cgroup_data {
 	int priority_offset;
+	int display_boost;
 
 	struct kref ref;
 };
@@ -75,6 +76,8 @@ get_or_create_cgroup_data(struct drm_i915_private *dev_priv,
 			goto out;
 		}
 
+		priv->display_boost = I915_PRIORITY_DEFAULT_DISPBOOST;
+
 		kref_init(&priv->ref);
 		cgroup_priv_install(cgrp, dev_priv->cgroup_priv_key,
 				    &priv->ref);
@@ -150,6 +153,19 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev,
 		}
 		break;
 
+	case I915_CGROUP_PARAM_DISPBOOST_PRIORITY:
+		if (req->value < I915_PRIORITY_MAX_DISPBOOST &&
+		    req->value > I915_PRIORITY_MIN) {
+			DRM_DEBUG_DRIVER("Setting cgroup display boost priority to %lld\n",
+					 req->value);
+			cgrpdata->display_boost = req->value;
+		} else {
+			DRM_DEBUG_DRIVER("Invalid cgroup display boost priority %lld\n",
+					 req->value);
+			ret = -EINVAL;
+		}
+		break;
+
 	default:
 		DRM_DEBUG_DRIVER("Invalid cgroup parameter %lld\n", req->param);
 		ret = -EINVAL;
@@ -184,5 +200,6 @@ int i915_cgroup_get_current_##name(struct drm_i915_private *dev_priv)	\
 }
 
 CGROUP_GET(prio_offset, priority_offset, 0)
+CGROUP_GET(dispboost, display_boost, I915_PRIORITY_DEFAULT_DISPBOOST);
 
 #undef CGROUP_GET
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fd5629593f4b..fe787cfcef72 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2689,6 +2689,7 @@ int i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data,
 			       struct drm_file *file);
 void i915_cgroup_shutdown(struct drm_i915_private *dev_priv);
 int i915_cgroup_get_current_prio_offset(struct drm_i915_private *dev_priv);
+int i915_cgroup_get_current_dispboost(struct drm_i915_private *dev_priv);
 #else
 static inline void i915_cgroup_init(struct drm_i915_private *dev_priv) {}
 static inline void i915_cgroup_shutdown(struct drm_i915_private *dev_priv) {}
@@ -2702,6 +2703,11 @@ i915_cgroup_get_current_prio_offset(struct drm_i915_private *dev_priv)
 {
 	return 0;
 }
+static inline int
+i915_cgroup_get_current_dispboost(struct drm_i915_private *dev_priv)
+{
+	return I915_PRIORITY_DEFAULT_DISPBOOST;
+}
 #endif
 
 /* i915_drv.c */
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index cf7a7147daf3..db300d93fd08 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -90,6 +90,7 @@ enum {
 	/* Range reachable by combining user priority + cgroup offset */
 	I915_PRIORITY_MAX = 0x7fffff,
 	I915_PRIORITY_MIN = -I915_PRIORITY_MAX,
+	I915_PRIORITY_MAX_DISPBOOST = I915_PRIORITY_MAX + 1,
 
 	/* Special case priority values */
 	I915_PRIORITY_INVALID = INT_MIN,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b053a21f682b..1d0245e2fd75 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12731,7 +12731,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	struct drm_framebuffer *fb = new_state->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
-	int ret;
+	int boost, ret;
 
 	if (old_obj) {
 		struct drm_crtc_state *crtc_state =
@@ -12783,7 +12783,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 
 	ret = intel_plane_pin_fb(to_intel_plane_state(new_state));
 
-	i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DEFAULT_DISPBOOST);
+	boost = i915_cgroup_get_current_dispboost(dev_priv);
+	i915_gem_object_wait_priority(obj, 0, boost);
 
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 	i915_gem_object_unpin_pages(obj);
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 6b70f46d224e..ad454f121884 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1727,6 +1727,7 @@ struct drm_i915_cgroup_param {
 	__u32 flags;
 	__u64 param;
 #define I915_CGROUP_PARAM_PRIORITY_OFFSET	0x1
+#define I915_CGROUP_PARAM_DISPBOOST_PRIORITY    0x2
 	__s64 value;
 };
 
-- 
2.14.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 8/8] drm/i915: Add context priority & priority offset to debugfs (v2)

[Matt Roper]

Update i915_context_status to include priority information.

v2:
 - Clarify that the offset is based on cgroup (Chris)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5378863e3238..02d1983b8581 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1959,6 +1959,9 @@ static int i915_context_status(struct seq_file *m, void *unused)
 		seq_putc(m, ctx->remap_slice ? 'R' : 'r');
 		seq_putc(m, '\n');
 
+		seq_printf(m, "Priority %d (cgroup offset = %d)\n",
+			   ctx->priority, ctx->priority_offset);
+
 		for_each_engine(engine, dev_priv, id) {
 			struct intel_context *ce = &ctx->engine[engine->id];
 
-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH i-g-t] tests: Introduce drv_cgroup (v2)

[Matt Roper]

drv_cgroup exercises both valid and invalid usage of the i915 cgroup
parameter ioctl.

v2:
 - Add support for display boost
 - Drop cgroup permissions test (the kernel no longer bases access
   control on fs permissions)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 tests/Makefile.sources |   1 +
 tests/drv_cgroup.c     | 262 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 263 insertions(+)
 create mode 100644 tests/drv_cgroup.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 4e6f5319..ba4b00bb 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -37,6 +37,7 @@ TESTS_progs = \
 	drm_vma_limiter_cached \
 	drm_vma_limiter_cpu \
 	drm_vma_limiter_gtt \
+	drv_cgroup \
 	drv_getparams_basic \
 	drv_hangman \
 	drv_missed_irq \
diff --git a/tests/drv_cgroup.c b/tests/drv_cgroup.c
new file mode 100644
index 00000000..457daf7b
--- /dev/null
+++ b/tests/drv_cgroup.c
@@ -0,0 +1,262 @@
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+#include "igt.h"
+#include "igt_debugfs.h"
+#include "igt_aux.h"
+#include "igt_kmod.h"
+#include "igt_sysfs.h"
+#include "igt_core.h"
+
+#include <dirent.h>
+#include <fcntl.h>
+#include <libgen.h>
+#include <signal.h>
+#include <stdio.h>
+#include <linux/limits.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/utsname.h>
+
+/* libdrm support is not yet upstream, so duplicate defs here for now */
+
+struct INTERNAL_drm_i915_cgroup_param {
+	__s32 cgroup_fd;
+	__u32 flags;
+	__u64 param;
+#define I915_CGROUP_PARAM_PRIORITY_OFFSET       0x1
+#define I915_CGROUP_PARAM_DISPBOOST_PRIORITY    0x2
+	__s64 value;
+};
+
+#define INTERNAL_IOCTL_I915_CGROUP_SETPARAM  \
+	DRM_IOW(DRM_COMMAND_BASE + 0x3a, struct INTERNAL_drm_i915_cgroup_param)
+
+static char tmp[PATH_MAX], *cgrp2dir = NULL;
+static int cgrp2fd = -1;
+static int drm_fd = -1;
+
+/* Figure out where (if) cgroups-v2 is mounted on this machine. */
+static void
+find_cgroup2(void)
+{
+	FILE *f;
+	char *from, *path, *type;
+
+	f = fopen("/proc/mounts", "r");
+	igt_assert(f);
+
+	while (fgets(tmp, sizeof(tmp), f)) {
+		from = strtok(tmp, " ");
+		if (!from) continue;
+
+		path = strtok(NULL, " ");
+		if (!path) continue;
+
+		type = strtok(NULL, " ");
+		if (strcmp(type, "cgroup2") == 0) {
+			cgrp2dir = path;
+			cgrp2fd = open(cgrp2dir, O_DIRECTORY|O_RDONLY);
+			break;
+		}
+	}
+
+	fclose(f);
+
+	igt_skip_on_f(!cgrp2dir, "cgroups-v2 is not mounted\n");
+	igt_skip_on_f(!cgrp2fd, "Insufficient fs permissions on cgroup2 dir\n");
+}
+
+static char tmp_cgroup[PATH_MAX];
+
+static int
+create_tmp_cgroup(void)
+{
+	char *dirname;
+	int fd;
+
+	snprintf(tmp_cgroup, sizeof(tmp_cgroup), "%s/igt_cgroup_XXXXXX",
+		 cgrp2dir);
+	dirname = mkdtemp(tmp_cgroup);
+	igt_assert(dirname);
+
+	fd = open(dirname, O_DIRECTORY|O_RDONLY);
+	igt_assert(fd >= 0);
+
+	return fd;
+}
+
+static void
+rm_tmp_cgroup(int fd)
+{
+	close(fd);
+	rmdir(tmp_cgroup);
+}
+
+static int
+set_prio(int fd, int prio)
+{
+	struct INTERNAL_drm_i915_cgroup_param req;
+
+	req.cgroup_fd = fd;
+	req.flags = 0;
+	req.param = I915_CGROUP_PARAM_PRIORITY_OFFSET;
+	req.value = prio;
+
+	return drmIoctl(drm_fd, INTERNAL_IOCTL_I915_CGROUP_SETPARAM, &req);
+}
+
+static int
+set_boost(int fd, int boost)
+{
+	struct INTERNAL_drm_i915_cgroup_param req;
+
+	req.cgroup_fd = fd;
+	req.flags = 0;
+	req.param = I915_CGROUP_PARAM_DISPBOOST_PRIORITY;
+	req.value = boost;
+
+	return drmIoctl(drm_fd, INTERNAL_IOCTL_I915_CGROUP_SETPARAM, &req);
+}
+
+igt_main
+{
+	char other_file[PATH_MAX];
+	int ret, fd;
+
+	igt_fixture {
+		find_cgroup2();
+		drm_fd = drm_open_driver_master(DRIVER_INTEL);
+	}
+
+	/* Standard request to set priority.  Should succeed */
+	igt_subtest_f("set-prio") {
+		fd = create_tmp_cgroup();
+		ret = set_prio(fd, 123);
+		igt_fail_on(ret);
+		rm_tmp_cgroup(fd);
+	}
+
+	/* Set priority outside valid range.  Should fail */
+	igt_subtest_f("prio-out-of-range") {
+		fd = create_tmp_cgroup();
+		ret = set_prio(fd, 0x800000);
+		igt_assert(ret < 0 && errno == EINVAL);
+		rm_tmp_cgroup(fd);
+	}
+
+	/* Set display boost to valid value.  Should succeed */
+	igt_subtest_f("set-dispboost") {
+		fd = create_tmp_cgroup();
+		ret = set_boost(fd, 0x3000);
+		igt_fail_on(ret);
+		rm_tmp_cgroup(fd);
+	}
+
+	/* Set display boost outside valid range.  Should fail */
+	igt_subtest_f("dispboost-out-of-range") {
+		fd = create_tmp_cgroup();
+		ret = set_boost(fd, 0x800001);
+		igt_assert(ret < 0 && errno == EINVAL);
+		rm_tmp_cgroup(fd);
+	}
+
+	/* Use an invalid parameter ID.  Should fail. */
+	igt_subtest_f("bad-param") {
+		struct INTERNAL_drm_i915_cgroup_param req;
+
+		fd = create_tmp_cgroup();
+		req.cgroup_fd = fd;
+		req.flags = 0;
+		req.param = 0xDEADBEEF;
+		req.value = 123;
+
+		ret = drmIoctl(drm_fd, INTERNAL_IOCTL_I915_CGROUP_SETPARAM, &req);
+		igt_assert(ret < 0 && errno == EINVAL);
+
+		rm_tmp_cgroup(fd);
+	}
+
+	/*
+	 * Pass fd for a cgroup control file instead of cgroup directory itself.
+	 * Should fail.
+	 */
+	igt_subtest_f("control-file-fd") {
+		int fd2;
+
+		fd = create_tmp_cgroup();
+		snprintf(other_file, sizeof(other_file),
+			 "%s/cgroup.procs", tmp_cgroup);
+		fd2 = open(other_file, O_RDONLY);
+		igt_assert(fd2 >= 0);
+
+		ret = set_prio(fd2, 123);
+		igt_assert(ret < 0 && errno == EBADF);
+
+		close(fd2);
+		rm_tmp_cgroup(fd);
+	}
+
+	/*
+	 * Pass an fd for a non-cgroup directory.  Should fail.
+	 * Note that we rely on /tmp being available and writable.
+	 */
+	igt_subtest_f("non-cgroup-fd") {
+		char *dirname;
+
+		strcpy(other_file, "/tmp/igt_XXXXXX");
+		dirname = mkdtemp(other_file);
+		igt_assert(dirname);
+
+		fd = open(dirname, O_DIRECTORY|O_RDONLY);
+		igt_assert(fd >= 0);
+
+		ret = set_prio(fd, 123);
+		igt_assert(ret < 0 && errno == EBADF);
+
+		close(fd);
+		rmdir(dirname);
+	}
+
+	/*
+	 * Reload driver after setting priority.  Should pass.
+	 *
+	 * The priority data will be lost across reload, but the goal is to
+	 * make sure we don't panic during driver removal or subsequent
+	 * assignment of a new priority to the existing cgroup.
+	 */
+	igt_subtest_f("cgroup-reload") {
+		fd = create_tmp_cgroup();
+		ret = set_prio(fd, 123);
+		igt_fail_on(ret);
+
+		ret = igt_i915_driver_unload();
+		igt_fail_on(ret);
+		ret = igt_i915_driver_load(NULL);
+		igt_fail_on(ret);
+
+		ret = set_prio(fd, 456);
+		igt_fail_on(ret);
+
+		rm_tmp_cgroup(fd);
+	}
+}
-- 
2.14.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for cgroup private data and DRM/i915 integration

[Patchwork]

== Series Details ==

Series: cgroup private data and DRM/i915 integration
URL   : https://patchwork.freedesktop.org/series/40142/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
ed1f8853712d cgroup: Allow registration and lookup of cgroup private data (v2)
-:52: CHECK:UNCOMMENTED_DEFINITION: spinlock_t definition without comment
#52: FILE: include/linux/cgroup-defs.h:436:
+	spinlock_t privdata_lock;

-:276: WARNING:FUNCTION_ARGUMENTS: function definition argument 'struct kref *' should also have an identifier name
#276: FILE: kernel/cgroup/cgroup.c:6095:
+	void (*free)(struct kref *);

total: 0 errors, 1 warnings, 1 checks, 238 lines checked
a2cf2e58be10 cgroup: Introduce task_get_dfl_cgroup() (v2)
7837b0190516 cgroup: Introduce cgroup_priv_get_current
cd9b81bd5505 drm/i915: Adjust internal priority definitions
d4e5c99d4496 drm/i915: cgroup integration (v3)
-:34: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#34: 
new file mode 100644

-:109: WARNING:SIZEOF_PARENTHESIS: sizeof *priv should be sizeof(*priv)
#109: FILE: drivers/gpu/drm/i915/i915_cgroup.c:71:
+		priv = kzalloc(sizeof *priv, GFP_KERNEL);

-:207: WARNING:LONG_LINE: line over 100 characters
#207: FILE: drivers/gpu/drm/i915/i915_drv.c:2843:
+	DRM_IOCTL_DEF_DRV(I915_CGROUP_SETPARAM, i915_cgroup_setparam_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),

-:207: CHECK:SPACING: spaces preferred around that '|' (ctx:VxV)
#207: FILE: drivers/gpu/drm/i915/i915_drv.c:2843:
+	DRM_IOCTL_DEF_DRV(I915_CGROUP_SETPARAM, i915_cgroup_setparam_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	                                                                                ^

-:221: CHECK:UNCOMMENTED_DEFINITION: struct mutex definition without comment
#221: FILE: drivers/gpu/drm/i915/i915_drv.h:1743:
+	struct mutex cgroup_lock;

-:242: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
#242: FILE: drivers/gpu/drm/i915/i915_drv.h:2697:
+}
+static inline void i915_cgroup_shutdown(struct drm_i915_private *dev_priv) {}

-:270: WARNING:LONG_LINE: line over 100 characters
#270: FILE: include/uapi/drm/i915_drm.h:381:
+#define DRM_IOCTL_I915_CGROUP_SETPARAM		DRM_IOW(DRM_COMMAND_BASE + DRM_I915_CGROUP_SETPARAM, struct drm_i915_cgroup_param)

total: 0 errors, 4 warnings, 3 checks, 239 lines checked
6d7ce25f32de drm/i915: Introduce 'priority offset' for GPU contexts (v3)
-:121: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'def' - possible side-effects?
#121: FILE: drivers/gpu/drm/i915/i915_cgroup.c:172:
+#define CGROUP_GET(name, field, def) \
+int i915_cgroup_get_current_##name(struct drm_i915_private *dev_priv)	\
+{									\
+	struct kref *ref;						\
+	int val = def;							\
+	if (!dev_priv->cgroup_priv_key) return def;			\
+	ref = cgroup_priv_get_current(dev_priv->cgroup_priv_key);	\
+	if (ref) {							\
+		val = cgrp_ref_to_i915(ref)->field;			\
+		kref_put(ref, i915_cgroup_free);			\
+	}								\
+	return val;							\
+}

-:126: ERROR:TRAILING_STATEMENTS: trailing statements should be on next line
#126: FILE: drivers/gpu/drm/i915/i915_cgroup.c:177:
+	if (!dev_priv->cgroup_priv_key) return def;			\

total: 1 errors, 0 warnings, 1 checks, 169 lines checked
dac3bdd93cb9 drm/i915: Introduce per-cgroup display boost setting
-:83: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
#83: FILE: drivers/gpu/drm/i915/i915_drv.h:2706:
 }
+static inline int

total: 0 errors, 0 warnings, 1 checks, 89 lines checked
fa175d947839 drm/i915: Add context priority & priority offset to debugfs (v2)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.CHECKPATCH: warning for cgroup private data and DRM/i915 integration

[Jani Nikula]

On Sat, 17 Mar 2018, Patchwork <patchwork@emeril.freedesktop.org> wrote:
> == Series Details ==
>
> Series: cgroup private data and DRM/i915 integration
> URL   : https://patchwork.freedesktop.org/series/40142/
> State : warning
>
> == Summary ==

Matt, please do look into these, I think most of these are valid.

Thanks,
Jani.


>
> $ dim checkpatch origin/drm-tip
> ed1f8853712d cgroup: Allow registration and lookup of cgroup private data (v2)
> -:52: CHECK:UNCOMMENTED_DEFINITION: spinlock_t definition without comment
> #52: FILE: include/linux/cgroup-defs.h:436:
> +	spinlock_t privdata_lock;
>
> -:276: WARNING:FUNCTION_ARGUMENTS: function definition argument 'struct kref *' should also have an identifier name
> #276: FILE: kernel/cgroup/cgroup.c:6095:
> +	void (*free)(struct kref *);
>
> total: 0 errors, 1 warnings, 1 checks, 238 lines checked
> a2cf2e58be10 cgroup: Introduce task_get_dfl_cgroup() (v2)
> 7837b0190516 cgroup: Introduce cgroup_priv_get_current
> cd9b81bd5505 drm/i915: Adjust internal priority definitions
> d4e5c99d4496 drm/i915: cgroup integration (v3)
> -:34: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
> #34: 
> new file mode 100644
>
> -:109: WARNING:SIZEOF_PARENTHESIS: sizeof *priv should be sizeof(*priv)
> #109: FILE: drivers/gpu/drm/i915/i915_cgroup.c:71:
> +		priv = kzalloc(sizeof *priv, GFP_KERNEL);
>
> -:207: WARNING:LONG_LINE: line over 100 characters
> #207: FILE: drivers/gpu/drm/i915/i915_drv.c:2843:
> +	DRM_IOCTL_DEF_DRV(I915_CGROUP_SETPARAM, i915_cgroup_setparam_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>
> -:207: CHECK:SPACING: spaces preferred around that '|' (ctx:VxV)
> #207: FILE: drivers/gpu/drm/i915/i915_drv.c:2843:
> +	DRM_IOCTL_DEF_DRV(I915_CGROUP_SETPARAM, i915_cgroup_setparam_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>  	                                                                                ^
>
> -:221: CHECK:UNCOMMENTED_DEFINITION: struct mutex definition without comment
> #221: FILE: drivers/gpu/drm/i915/i915_drv.h:1743:
> +	struct mutex cgroup_lock;
>
> -:242: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
> #242: FILE: drivers/gpu/drm/i915/i915_drv.h:2697:
> +}
> +static inline void i915_cgroup_shutdown(struct drm_i915_private *dev_priv) {}
>
> -:270: WARNING:LONG_LINE: line over 100 characters
> #270: FILE: include/uapi/drm/i915_drm.h:381:
> +#define DRM_IOCTL_I915_CGROUP_SETPARAM		DRM_IOW(DRM_COMMAND_BASE + DRM_I915_CGROUP_SETPARAM, struct drm_i915_cgroup_param)
>
> total: 0 errors, 4 warnings, 3 checks, 239 lines checked
> 6d7ce25f32de drm/i915: Introduce 'priority offset' for GPU contexts (v3)
> -:121: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'def' - possible side-effects?
> #121: FILE: drivers/gpu/drm/i915/i915_cgroup.c:172:
> +#define CGROUP_GET(name, field, def) \
> +int i915_cgroup_get_current_##name(struct drm_i915_private *dev_priv)	\
> +{									\
> +	struct kref *ref;						\
> +	int val = def;							\
> +	if (!dev_priv->cgroup_priv_key) return def;			\
> +	ref = cgroup_priv_get_current(dev_priv->cgroup_priv_key);	\
> +	if (ref) {							\
> +		val = cgrp_ref_to_i915(ref)->field;			\
> +		kref_put(ref, i915_cgroup_free);			\
> +	}								\
> +	return val;							\
> +}
>
> -:126: ERROR:TRAILING_STATEMENTS: trailing statements should be on next line
> #126: FILE: drivers/gpu/drm/i915/i915_cgroup.c:177:
> +	if (!dev_priv->cgroup_priv_key) return def;			\
>
> total: 1 errors, 0 warnings, 1 checks, 169 lines checked
> dac3bdd93cb9 drm/i915: Introduce per-cgroup display boost setting
> -:83: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
> #83: FILE: drivers/gpu/drm/i915/i915_drv.h:2706:
>  }
> +static inline int
>
> total: 0 errors, 0 warnings, 1 checks, 89 lines checked
> fa175d947839 drm/i915: Add context priority & priority offset to debugfs (v2)
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for cgroup private data and DRM/i915 integration

[Patchwork]

== Series Details ==

Series: cgroup private data and DRM/i915 integration
URL   : https://patchwork.freedesktop.org/series/40142/
State : success

== Summary ==

Series 40142v1 cgroup private data and DRM/i915 integration
https://patchwork.freedesktop.org/api/1.0/series/40142/revisions/1/mbox/

---- Known issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:432s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:442s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:383s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:538s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:297s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:516s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:513s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:517s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:515s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:417s
fi-cfl-s2        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:590s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:511s
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:527s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:587s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:429s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:318s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:408s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:421s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:474s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:428s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:471s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:466s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:518s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:657s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:542s
fi-skl-6700hq    total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:541s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:504s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:492s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:429s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:587s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:399s

639c78d2805bec2022435a8184db27a3b9767fe7 drm-tip: 2018y-03m-16d-23h-05m-00s UTC integration manifest
fa175d947839 drm/i915: Add context priority & priority offset to debugfs (v2)
dac3bdd93cb9 drm/i915: Introduce per-cgroup display boost setting
6d7ce25f32de drm/i915: Introduce 'priority offset' for GPU contexts (v3)
d4e5c99d4496 drm/i915: cgroup integration (v3)
cd9b81bd5505 drm/i915: Adjust internal priority definitions
7837b0190516 cgroup: Introduce cgroup_priv_get_current
a2cf2e58be10 cgroup: Introduce task_get_dfl_cgroup() (v2)
ed1f8853712d cgroup: Allow registration and lookup of cgroup private data (v2)

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8386/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for tests: Introduce drv_cgroup (v2)

[Patchwork]

== Series Details ==

Series: tests: Introduce drv_cgroup (v2)
URL   : https://patchwork.freedesktop.org/series/40143/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
98f7614bd725afaae48f7b70d18329149075661b lib: Parse plane IN_FORMATS blobifiers into a nicer form

with latest DRM-Tip kernel build CI_DRM_3944
639c78d2805b drm-tip: 2018y-03m-16d-23h-05m-00s UTC integration manifest

No testlist changes.

---- Known issues:

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-bdw-5557u) fdo#104944
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104944 https://bugs.freedesktop.org/show_bug.cgi?id=104944
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:241  pass:223  dwarn:0   dfail:0   fail:0   skip:17 
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:444s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:383s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:545s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:296s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:512s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:513s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:518s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:502s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:410s
fi-cfl-s2        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:582s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:511s
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:521s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:584s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:429s
fi-gdg-551       total:285  pass:177  dwarn:0   dfail:0   fail:0   skip:108 time:318s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:536s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:406s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:426s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:474s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:434s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:476s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:469s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:512s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:660s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:444s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:563s
fi-skl-6700hq    total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:539s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:509s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:489s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:427s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:595s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:400s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1158/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration

[Joonas Lahtinen]

Quoting Matt Roper (2018-03-17 02:08:57)
> This is the fourth iteration of the work previously posted here:
>   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
>   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
>   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> 
> The high level goal of this work is to allow non-cgroup-controller parts
> of the kernel (e.g., device drivers) to register their own private
> policy data for specific cgroups.  That mechanism is then made use of in
> the i915 graphics driver to allow GPU priority to be assigned according
> to the cgroup membership of the owning process.  Please see the v1 cover
> letter linked above for a more in-depth explanation and justification.

Hi Matt,

For cross-subsystem changes such as this, it makes sense to Cc all
relevant maintainers, especially if there have been previous comments to
earlier revisions.

Please, do include and keep a reference to the userspace portion of the
changes when you suggest new uAPI to be added. At least I have some trouble
trying to track down the relevant interface consumer here.

I'm unsure how much sense it makes to commence with detailed i915 review
if we will be blocked by lack of userspace after that? I'm assuming
you've read through [1] already.

Regards, Joonas

[1] https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration

[Matt Roper]

On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
> Quoting Matt Roper (2018-03-17 02:08:57)
> > This is the fourth iteration of the work previously posted here:
> >   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
> >   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
> >   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> > 
> > The high level goal of this work is to allow non-cgroup-controller parts
> > of the kernel (e.g., device drivers) to register their own private
> > policy data for specific cgroups.  That mechanism is then made use of in
> > the i915 graphics driver to allow GPU priority to be assigned according
> > to the cgroup membership of the owning process.  Please see the v1 cover
> > letter linked above for a more in-depth explanation and justification.
> 
> Hi Matt,
> 
> For cross-subsystem changes such as this, it makes sense to Cc all
> relevant maintainers, especially if there have been previous comments to
> earlier revisions.
> 
> Please, do include and keep a reference to the userspace portion of the
> changes when you suggest new uAPI to be added. At least I have some trouble
> trying to track down the relevant interface consumer here.
> 
> I'm unsure how much sense it makes to commence with detailed i915 review
> if we will be blocked by lack of userspace after that? I'm assuming
> you've read through [1] already.

Hi Joonas,

I've sent the userspace code out a few times, but it looks like I forgot
to include a copy with v4/v4.5.  Here's the version I provided with v3:
  https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html

Usually we don't consider things like i-g-t to be sufficient userspace
consumers because we need a real-world consumer rather than a "toy"
userspace.  However in this case, the i-g-t tool, although very simple,
is really the only userspace consumer I expect there to ever be.
Ultimately the true consumer of this cgroups work are bash scripts, sysv
init scripts, systemd recipes, etc.  that just need a very simple tool
to assign the specific values that make sense on a given system.
There's no expectation that graphics clients or display servers would
ever need to make use of these interfaces.



Matt

> 
> Regards, Joonas
> 
> [1] https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration

[Joonas Lahtinen]

Quoting Matt Roper (2018-03-23 17:46:16)
> On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
> > Quoting Matt Roper (2018-03-17 02:08:57)
> > > This is the fourth iteration of the work previously posted here:
> > >   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
> > >   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
> > >   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> > > 
> > > The high level goal of this work is to allow non-cgroup-controller parts
> > > of the kernel (e.g., device drivers) to register their own private
> > > policy data for specific cgroups.  That mechanism is then made use of in
> > > the i915 graphics driver to allow GPU priority to be assigned according
> > > to the cgroup membership of the owning process.  Please see the v1 cover
> > > letter linked above for a more in-depth explanation and justification.
> > 
> > Hi Matt,
> > 
> > For cross-subsystem changes such as this, it makes sense to Cc all
> > relevant maintainers, especially if there have been previous comments to
> > earlier revisions.
> > 
> > Please, do include and keep a reference to the userspace portion of the
> > changes when you suggest new uAPI to be added. At least I have some trouble
> > trying to track down the relevant interface consumer here.
> > 
> > I'm unsure how much sense it makes to commence with detailed i915 review
> > if we will be blocked by lack of userspace after that? I'm assuming
> > you've read through [1] already.
> 
> Hi Joonas,
> 
> I've sent the userspace code out a few times, but it looks like I forgot
> to include a copy with v4/v4.5.  Here's the version I provided with v3:
>   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html

Thanks. Keeping that in the relevant commit message of the patch that
introduces the new uAPI will make it harder to forget and easiest for
git blame, too.

> 
> Usually we don't consider things like i-g-t to be sufficient userspace
> consumers because we need a real-world consumer rather than a "toy"
> userspace.  However in this case, the i-g-t tool, although very simple,
> is really the only userspace consumer I expect there to ever be.
> Ultimately the true consumer of this cgroups work are bash scripts, sysv
> init scripts, systemd recipes, etc.  that just need a very simple tool
> to assign the specific values that make sense on a given system.
> There's no expectation that graphics clients or display servers would
> ever need to make use of these interfaces.

I was under the impression that a bit more generic GPU cgroups support
was receiving a lot of support in the early discussion? A dedicated
intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
for user adoption :)

Also, I might not be up-to-date about all things cgroups, but the way
intel_cgroup works, feels bit forced. We create a userspace context just
to communicate with the driver and the IOCTL will still have global
effects. I can't but think that i915 reading from the cgroups subsystem
for the current process would feel more intuitive to me.

Does the implementation mimic some existing cgroups tool or de-facto way
of doing things in cgroups world?

Regards, Joonas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration

[Matt Roper]

On Mon, Mar 26, 2018 at 10:30:23AM +0300, Joonas Lahtinen wrote:
> Quoting Matt Roper (2018-03-23 17:46:16)
> > On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
> > > Quoting Matt Roper (2018-03-17 02:08:57)
> > > > This is the fourth iteration of the work previously posted here:
> > > >   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
> > > >   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
> > > >   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> > > > 
> > > > The high level goal of this work is to allow non-cgroup-controller parts
> > > > of the kernel (e.g., device drivers) to register their own private
> > > > policy data for specific cgroups.  That mechanism is then made use of in
> > > > the i915 graphics driver to allow GPU priority to be assigned according
> > > > to the cgroup membership of the owning process.  Please see the v1 cover
> > > > letter linked above for a more in-depth explanation and justification.
> > > 
> > > Hi Matt,
> > > 
> > > For cross-subsystem changes such as this, it makes sense to Cc all
> > > relevant maintainers, especially if there have been previous comments to
> > > earlier revisions.
> > > 
> > > Please, do include and keep a reference to the userspace portion of the
> > > changes when you suggest new uAPI to be added. At least I have some trouble
> > > trying to track down the relevant interface consumer here.
> > > 
> > > I'm unsure how much sense it makes to commence with detailed i915 review
> > > if we will be blocked by lack of userspace after that? I'm assuming
> > > you've read through [1] already.
> > 
> > Hi Joonas,
> > 
> > I've sent the userspace code out a few times, but it looks like I forgot
> > to include a copy with v4/v4.5.  Here's the version I provided with v3:
> >   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html
> 
> Thanks. Keeping that in the relevant commit message of the patch that
> introduces the new uAPI will make it harder to forget and easiest for
> git blame, too.
> 
> > 
> > Usually we don't consider things like i-g-t to be sufficient userspace
> > consumers because we need a real-world consumer rather than a "toy"
> > userspace.  However in this case, the i-g-t tool, although very simple,
> > is really the only userspace consumer I expect there to ever be.
> > Ultimately the true consumer of this cgroups work are bash scripts, sysv
> > init scripts, systemd recipes, etc.  that just need a very simple tool
> > to assign the specific values that make sense on a given system.
> > There's no expectation that graphics clients or display servers would
> > ever need to make use of these interfaces.
> 
> I was under the impression that a bit more generic GPU cgroups support
> was receiving a lot of support in the early discussion? A dedicated
> intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
> for user adoption :)

I'm open to moving the cgroup_priv registration/lookup to the DRM core
if other drivers are interested in using this mechanism and if we can
come to an agreement on a standard priority offset range to support, how
display boost should work for all drivers, etc.  There might be some
challenges mapping a DRM-defined priority range down to a different
range that makes sense for individual driver schedulers, especially
since some drivers already expose a different priority scheme to
userspace via other interfaces like i915 does with GEM context priority.

So far I haven't really heard any interest outside the Intel camp, but
hopefully other driver teams can speak up if they're for/against this.
I don't want to try to artificially standardize this if other drivers
want to go a different direction with priority/scheduling that's too
different from the current Intel-driven design.

> Also, I might not be up-to-date about all things cgroups, but the way
> intel_cgroup works, feels bit forced. We create a userspace context just
> to communicate with the driver and the IOCTL will still have global
> effects. I can't but think that i915 reading from the cgroups subsystem
> for the current process would feel more intuitive to me.

I think you're referring to the earlier discussion about exposing
priority directly via the cgroups filesystem?  That would certainly be
simpler from a userspace perspective, but it's not the direction that
the cgroups maintainer wants to see things go.  Adding files directly to
the cgroups filesystem is supposed to be something that's reserved for
official cgroups controllers.  The GPU priority concept we're trying to
add here doesn't align with the requirements for creating a controller,
so the preferred approach is to create a custom interface (syscall or
ioctl) that simply takes a cgroup as a parameter.  There's precendent
with similar interfaces in areas like BPF (where the bpf() system call
can accept a cgroup as a parameter and then perform its own private
policy changes as it sees fit).

Using a true cgroups controller and exposing settings via the filesystem
is likely still the way we'll want to go for some other types of
cgroups-based policy in the future (e.g., limiting GPU memory usage); it
just isn't the appropriate direction for priority.

> Does the implementation mimic some existing cgroups tool or de-facto way
> of doing things in cgroups world?

The ioctl approach I took is similar to syscall approach that the BPF
guys use to attach BPF programs to a cgroup.  I'm not very familiar with
BPF or how it gets used from userspace, so I'm not sure whether the
interface is intended for one specific tool (like ours is), or whether
there's more variety for userspace consumers.


Matt

> 
> Regards, Joonas
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

DRM cgroups integration (Was: Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration)

[Joonas Lahtinen]

+ Dave for commenting from DRM subsystem perspective. I strongly believe
there would be benefit from agreeing on some foundation of DRM subsystem
level program GPU niceness [-20,19] and memory limit [0,N] pages.

Quoting Matt Roper (2018-03-30 03:43:13)
> On Mon, Mar 26, 2018 at 10:30:23AM +0300, Joonas Lahtinen wrote:
> > Quoting Matt Roper (2018-03-23 17:46:16)
> > > On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
> > > > Quoting Matt Roper (2018-03-17 02:08:57)
> > > > > This is the fourth iteration of the work previously posted here:
> > > > >   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
> > > > >   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
> > > > >   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> > > > > 
> > > > > The high level goal of this work is to allow non-cgroup-controller parts
> > > > > of the kernel (e.g., device drivers) to register their own private
> > > > > policy data for specific cgroups.  That mechanism is then made use of in
> > > > > the i915 graphics driver to allow GPU priority to be assigned according
> > > > > to the cgroup membership of the owning process.  Please see the v1 cover
> > > > > letter linked above for a more in-depth explanation and justification.
> > > > 
> > > > Hi Matt,
> > > > 
> > > > For cross-subsystem changes such as this, it makes sense to Cc all
> > > > relevant maintainers, especially if there have been previous comments to
> > > > earlier revisions.
> > > > 
> > > > Please, do include and keep a reference to the userspace portion of the
> > > > changes when you suggest new uAPI to be added. At least I have some trouble
> > > > trying to track down the relevant interface consumer here.
> > > > 
> > > > I'm unsure how much sense it makes to commence with detailed i915 review
> > > > if we will be blocked by lack of userspace after that? I'm assuming
> > > > you've read through [1] already.
> > > 
> > > Hi Joonas,
> > > 
> > > I've sent the userspace code out a few times, but it looks like I forgot
> > > to include a copy with v4/v4.5.  Here's the version I provided with v3:
> > >   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html
> > 
> > Thanks. Keeping that in the relevant commit message of the patch that
> > introduces the new uAPI will make it harder to forget and easiest for
> > git blame, too.
> > 
> > > 
> > > Usually we don't consider things like i-g-t to be sufficient userspace
> > > consumers because we need a real-world consumer rather than a "toy"
> > > userspace.  However in this case, the i-g-t tool, although very simple,
> > > is really the only userspace consumer I expect there to ever be.
> > > Ultimately the true consumer of this cgroups work are bash scripts, sysv
> > > init scripts, systemd recipes, etc.  that just need a very simple tool
> > > to assign the specific values that make sense on a given system.
> > > There's no expectation that graphics clients or display servers would
> > > ever need to make use of these interfaces.
> > 
> > I was under the impression that a bit more generic GPU cgroups support
> > was receiving a lot of support in the early discussion? A dedicated
> > intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
> > for user adoption :)
> 
> I'm open to moving the cgroup_priv registration/lookup to the DRM core
> if other drivers are interested in using this mechanism and if we can
> come to an agreement on a standard priority offset range to support, how
> display boost should work for all drivers, etc.  There might be some
> challenges mapping a DRM-defined priority range down to a different
> range that makes sense for individual driver schedulers, especially
> since some drivers already expose a different priority scheme to
> userspace via other interfaces like i915 does with GEM context priority.
> 
> So far I haven't really heard any interest outside the Intel camp, but
> hopefully other driver teams can speak up if they're for/against this.
> I don't want to try to artificially standardize this if other drivers
> want to go a different direction with priority/scheduling that's too
> different from the current Intel-driven design.

I don't think there are that many directions to go about GPU context
priority, considering we have the EGL_IMG_context_priority extension, so
it'll only be about granularity of the scale.

I would suggest to go with the nice like scale for easy user adoption,
then just apply that as the N most significant bits.

The contexts could then of course further adjust their priority from what
is set by the "gpu_nice" application with the remaining bits.

I'm strongly feeling this should be a DRM level "gpu_nice". And the
binding to cgroups should come through DRM core. If it doesn't, limiting
the amount of memory used becomes awkward as the allocation is
centralized to DRM core.

> > Also, I might not be up-to-date about all things cgroups, but the way
> > intel_cgroup works, feels bit forced. We create a userspace context just
> > to communicate with the driver and the IOCTL will still have global
> > effects. I can't but think that i915 reading from the cgroups subsystem
> > for the current process would feel more intuitive to me.
> 
> I think you're referring to the earlier discussion about exposing
> priority directly via the cgroups filesystem?  That would certainly be
> simpler from a userspace perspective, but it's not the direction that
> the cgroups maintainer wants to see things go.  Adding files directly to
> the cgroups filesystem is supposed to be something that's reserved for
> official cgroups controllers.  The GPU priority concept we're trying to
> add here doesn't align with the requirements for creating a controller,
> so the preferred approach is to create a custom interface (syscall or
> ioctl) that simply takes a cgroup as a parameter.  There's precendent
> with similar interfaces in areas like BPF (where the bpf() system call
> can accept a cgroup as a parameter and then perform its own private
> policy changes as it sees fit).
> 
> Using a true cgroups controller and exposing settings via the filesystem
> is likely still the way we'll want to go for some other types of
> cgroups-based policy in the future (e.g., limiting GPU memory usage); it
> just isn't the appropriate direction for priority.

Might be just me but feels bit crazy to be setting GPU memory usage
through another interface and then doing i915 specific IOCTLs to control
the priority of that same cgroup.

I don't feel comfortable adding custom cgroups dependent IOCTLs to i915
where cgroups is only working as the variable carrier in background. We
should really just be consuming a variable from cgroups and it should be
set outside of of the i915 IOCTL interface.

I'm still seeing that we should have a DRM cgroups controller and a DRM
subsystem wide application to control the priority and memory usage
to be fed to the drivers.

If we end up just supporting i915 apps, we could as well use LD_PRELOAD
wrapper and alter the context priority at creation time for exactly the
same effect and no extra interfaces to maintain.

> > Does the implementation mimic some existing cgroups tool or de-facto way
> > of doing things in cgroups world?
> 
> The ioctl approach I took is similar to syscall approach that the BPF
> guys use to attach BPF programs to a cgroup.  I'm not very familiar with
> BPF or how it gets used from userspace, so I'm not sure whether the
> interface is intended for one specific tool (like ours is), or whether
> there's more variety for userspace consumers.

Is the proposal to set the memory usage from similar interface, or is
that still not implemented?

I'm seeing a very close relation between time-slicing GPU time and
allowed GPU buffer allocations, so having two completely different
interfaces does just feel very hackish way of implementing this.

Regards, Joonas

> 
> 
> Matt
> 
> > 
> > Regards, Joonas
> > --
> > To unsubscribe from this list: send the line "unsubscribe cgroups" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: DRM cgroups integration (Was: Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration)

[Joonas Lahtinen]

+ Some more Cc's based on IRC discussion

Quoting Joonas Lahtinen (2018-04-05 16:46:51)
> + Dave for commenting from DRM subsystem perspective. I strongly believe
> there would be benefit from agreeing on some foundation of DRM subsystem
> level program GPU niceness [-20,19] and memory limit [0,N] pages.
> 
> Quoting Matt Roper (2018-03-30 03:43:13)
> > On Mon, Mar 26, 2018 at 10:30:23AM +0300, Joonas Lahtinen wrote:
> > > Quoting Matt Roper (2018-03-23 17:46:16)
> > > > On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
> > > > > Quoting Matt Roper (2018-03-17 02:08:57)
> > > > > > This is the fourth iteration of the work previously posted here:
> > > > > >   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
> > > > > >   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
> > > > > >   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> > > > > > 
> > > > > > The high level goal of this work is to allow non-cgroup-controller parts
> > > > > > of the kernel (e.g., device drivers) to register their own private
> > > > > > policy data for specific cgroups.  That mechanism is then made use of in
> > > > > > the i915 graphics driver to allow GPU priority to be assigned according
> > > > > > to the cgroup membership of the owning process.  Please see the v1 cover
> > > > > > letter linked above for a more in-depth explanation and justification.
> > > > > 
> > > > > Hi Matt,
> > > > > 
> > > > > For cross-subsystem changes such as this, it makes sense to Cc all
> > > > > relevant maintainers, especially if there have been previous comments to
> > > > > earlier revisions.
> > > > > 
> > > > > Please, do include and keep a reference to the userspace portion of the
> > > > > changes when you suggest new uAPI to be added. At least I have some trouble
> > > > > trying to track down the relevant interface consumer here.
> > > > > 
> > > > > I'm unsure how much sense it makes to commence with detailed i915 review
> > > > > if we will be blocked by lack of userspace after that? I'm assuming
> > > > > you've read through [1] already.
> > > > 
> > > > Hi Joonas,
> > > > 
> > > > I've sent the userspace code out a few times, but it looks like I forgot
> > > > to include a copy with v4/v4.5.  Here's the version I provided with v3:
> > > >   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html
> > > 
> > > Thanks. Keeping that in the relevant commit message of the patch that
> > > introduces the new uAPI will make it harder to forget and easiest for
> > > git blame, too.
> > > 
> > > > 
> > > > Usually we don't consider things like i-g-t to be sufficient userspace
> > > > consumers because we need a real-world consumer rather than a "toy"
> > > > userspace.  However in this case, the i-g-t tool, although very simple,
> > > > is really the only userspace consumer I expect there to ever be.
> > > > Ultimately the true consumer of this cgroups work are bash scripts, sysv
> > > > init scripts, systemd recipes, etc.  that just need a very simple tool
> > > > to assign the specific values that make sense on a given system.
> > > > There's no expectation that graphics clients or display servers would
> > > > ever need to make use of these interfaces.
> > > 
> > > I was under the impression that a bit more generic GPU cgroups support
> > > was receiving a lot of support in the early discussion? A dedicated
> > > intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
> > > for user adoption :)
> > 
> > I'm open to moving the cgroup_priv registration/lookup to the DRM core
> > if other drivers are interested in using this mechanism and if we can
> > come to an agreement on a standard priority offset range to support, how
> > display boost should work for all drivers, etc.  There might be some
> > challenges mapping a DRM-defined priority range down to a different
> > range that makes sense for individual driver schedulers, especially
> > since some drivers already expose a different priority scheme to
> > userspace via other interfaces like i915 does with GEM context priority.
> > 
> > So far I haven't really heard any interest outside the Intel camp, but
> > hopefully other driver teams can speak up if they're for/against this.
> > I don't want to try to artificially standardize this if other drivers
> > want to go a different direction with priority/scheduling that's too
> > different from the current Intel-driven design.
> 
> I don't think there are that many directions to go about GPU context
> priority, considering we have the EGL_IMG_context_priority extension, so
> it'll only be about granularity of the scale.
> 
> I would suggest to go with the nice like scale for easy user adoption,
> then just apply that as the N most significant bits.
> 
> The contexts could then of course further adjust their priority from what
> is set by the "gpu_nice" application with the remaining bits.
> 
> I'm strongly feeling this should be a DRM level "gpu_nice". And the
> binding to cgroups should come through DRM core. If it doesn't, limiting
> the amount of memory used becomes awkward as the allocation is
> centralized to DRM core.
> 
> > > Also, I might not be up-to-date about all things cgroups, but the way
> > > intel_cgroup works, feels bit forced. We create a userspace context just
> > > to communicate with the driver and the IOCTL will still have global
> > > effects. I can't but think that i915 reading from the cgroups subsystem
> > > for the current process would feel more intuitive to me.
> > 
> > I think you're referring to the earlier discussion about exposing
> > priority directly via the cgroups filesystem?  That would certainly be
> > simpler from a userspace perspective, but it's not the direction that
> > the cgroups maintainer wants to see things go.  Adding files directly to
> > the cgroups filesystem is supposed to be something that's reserved for
> > official cgroups controllers.  The GPU priority concept we're trying to
> > add here doesn't align with the requirements for creating a controller,
> > so the preferred approach is to create a custom interface (syscall or
> > ioctl) that simply takes a cgroup as a parameter.  There's precendent
> > with similar interfaces in areas like BPF (where the bpf() system call
> > can accept a cgroup as a parameter and then perform its own private
> > policy changes as it sees fit).
> > 
> > Using a true cgroups controller and exposing settings via the filesystem
> > is likely still the way we'll want to go for some other types of
> > cgroups-based policy in the future (e.g., limiting GPU memory usage); it
> > just isn't the appropriate direction for priority.
> 
> Might be just me but feels bit crazy to be setting GPU memory usage
> through another interface and then doing i915 specific IOCTLs to control
> the priority of that same cgroup.
> 
> I don't feel comfortable adding custom cgroups dependent IOCTLs to i915
> where cgroups is only working as the variable carrier in background. We
> should really just be consuming a variable from cgroups and it should be
> set outside of of the i915 IOCTL interface.
> 
> I'm still seeing that we should have a DRM cgroups controller and a DRM
> subsystem wide application to control the priority and memory usage
> to be fed to the drivers.
> 
> If we end up just supporting i915 apps, we could as well use LD_PRELOAD
> wrapper and alter the context priority at creation time for exactly the
> same effect and no extra interfaces to maintain.
> 
> > > Does the implementation mimic some existing cgroups tool or de-facto way
> > > of doing things in cgroups world?
> > 
> > The ioctl approach I took is similar to syscall approach that the BPF
> > guys use to attach BPF programs to a cgroup.  I'm not very familiar with
> > BPF or how it gets used from userspace, so I'm not sure whether the
> > interface is intended for one specific tool (like ours is), or whether
> > there's more variety for userspace consumers.
> 
> Is the proposal to set the memory usage from similar interface, or is
> that still not implemented?
> 
> I'm seeing a very close relation between time-slicing GPU time and
> allowed GPU buffer allocations, so having two completely different
> interfaces does just feel very hackish way of implementing this.
> 
> Regards, Joonas
> 
> > 
> > 
> > Matt
> > 
> > > 
> > > Regards, Joonas
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe cgroups" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM cgroups integration (Was: Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration)

[Matt Roper]

On Thu, Apr 05, 2018 at 05:15:13PM +0300, Joonas Lahtinen wrote:
> + Some more Cc's based on IRC discussion
> 
> Quoting Joonas Lahtinen (2018-04-05 16:46:51)
> > + Dave for commenting from DRM subsystem perspective. I strongly believe
> > there would be benefit from agreeing on some foundation of DRM subsystem
> > level program GPU niceness [-20,19] and memory limit [0,N] pages.

+Chris Wilson

If we ignore backward compatibility and ABI issues for now and assume
all drivers can move to [-20, 19] for application-accessible priority
ranges (i.e., self opt-in via existing context parameter ioctls and
such), I think we still need a much larger range for the priority offset
assigned via cgroup.  One of the goals with the cgroup work is to give
the system integrator the ability to define classes of applications with
non-overlapping ranges (i.e., in some systems an app in the "very
important" cgroup that self-lowers itself as far as possible should
still be prioritized higher than an app in the "best effort" cgroup that
self-boosts itself as far as possible).  Chris Wilson and I discussed
that a bit on this thread if you want to see the context:

https://lists.freedesktop.org/archives/intel-gfx/2018-March/158204.html

That discussion was based on i915's current application-accessible range
of [-1023,1023].  If we shift to a much smaller [-20,19] range for
applications to use directly, then we might want to allow cgroup offset
to be something like [-1000,1000] to ensure the system integrator has
enough flexibility?

Also note that "display boost" (i.e., a priority boost for contexts that
are blocking a flip) may vary depending on system setup, so setting
being able to define the display boost's effective priority via cgroup
is important as well.


Matt



> > 
> > Quoting Matt Roper (2018-03-30 03:43:13)
> > > On Mon, Mar 26, 2018 at 10:30:23AM +0300, Joonas Lahtinen wrote:
> > > > Quoting Matt Roper (2018-03-23 17:46:16)
> > > > > On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
> > > > > > Quoting Matt Roper (2018-03-17 02:08:57)
> > > > > > > This is the fourth iteration of the work previously posted here:
> > > > > > >   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
> > > > > > >   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
> > > > > > >   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> > > > > > > 
> > > > > > > The high level goal of this work is to allow non-cgroup-controller parts
> > > > > > > of the kernel (e.g., device drivers) to register their own private
> > > > > > > policy data for specific cgroups.  That mechanism is then made use of in
> > > > > > > the i915 graphics driver to allow GPU priority to be assigned according
> > > > > > > to the cgroup membership of the owning process.  Please see the v1 cover
> > > > > > > letter linked above for a more in-depth explanation and justification.
> > > > > > 
> > > > > > Hi Matt,
> > > > > > 
> > > > > > For cross-subsystem changes such as this, it makes sense to Cc all
> > > > > > relevant maintainers, especially if there have been previous comments to
> > > > > > earlier revisions.
> > > > > > 
> > > > > > Please, do include and keep a reference to the userspace portion of the
> > > > > > changes when you suggest new uAPI to be added. At least I have some trouble
> > > > > > trying to track down the relevant interface consumer here.
> > > > > > 
> > > > > > I'm unsure how much sense it makes to commence with detailed i915 review
> > > > > > if we will be blocked by lack of userspace after that? I'm assuming
> > > > > > you've read through [1] already.
> > > > > 
> > > > > Hi Joonas,
> > > > > 
> > > > > I've sent the userspace code out a few times, but it looks like I forgot
> > > > > to include a copy with v4/v4.5.  Here's the version I provided with v3:
> > > > >   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html
> > > > 
> > > > Thanks. Keeping that in the relevant commit message of the patch that
> > > > introduces the new uAPI will make it harder to forget and easiest for
> > > > git blame, too.
> > > > 
> > > > > 
> > > > > Usually we don't consider things like i-g-t to be sufficient userspace
> > > > > consumers because we need a real-world consumer rather than a "toy"
> > > > > userspace.  However in this case, the i-g-t tool, although very simple,
> > > > > is really the only userspace consumer I expect there to ever be.
> > > > > Ultimately the true consumer of this cgroups work are bash scripts, sysv
> > > > > init scripts, systemd recipes, etc.  that just need a very simple tool
> > > > > to assign the specific values that make sense on a given system.
> > > > > There's no expectation that graphics clients or display servers would
> > > > > ever need to make use of these interfaces.
> > > > 
> > > > I was under the impression that a bit more generic GPU cgroups support
> > > > was receiving a lot of support in the early discussion? A dedicated
> > > > intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
> > > > for user adoption :)
> > > 
> > > I'm open to moving the cgroup_priv registration/lookup to the DRM core
> > > if other drivers are interested in using this mechanism and if we can
> > > come to an agreement on a standard priority offset range to support, how
> > > display boost should work for all drivers, etc.  There might be some
> > > challenges mapping a DRM-defined priority range down to a different
> > > range that makes sense for individual driver schedulers, especially
> > > since some drivers already expose a different priority scheme to
> > > userspace via other interfaces like i915 does with GEM context priority.
> > > 
> > > So far I haven't really heard any interest outside the Intel camp, but
> > > hopefully other driver teams can speak up if they're for/against this.
> > > I don't want to try to artificially standardize this if other drivers
> > > want to go a different direction with priority/scheduling that's too
> > > different from the current Intel-driven design.
> > 
> > I don't think there are that many directions to go about GPU context
> > priority, considering we have the EGL_IMG_context_priority extension, so
> > it'll only be about granularity of the scale.
> > 
> > I would suggest to go with the nice like scale for easy user adoption,
> > then just apply that as the N most significant bits.
> > 
> > The contexts could then of course further adjust their priority from what
> > is set by the "gpu_nice" application with the remaining bits.
> > 
> > I'm strongly feeling this should be a DRM level "gpu_nice". And the
> > binding to cgroups should come through DRM core. If it doesn't, limiting
> > the amount of memory used becomes awkward as the allocation is
> > centralized to DRM core.
> > 
> > > > Also, I might not be up-to-date about all things cgroups, but the way
> > > > intel_cgroup works, feels bit forced. We create a userspace context just
> > > > to communicate with the driver and the IOCTL will still have global
> > > > effects. I can't but think that i915 reading from the cgroups subsystem
> > > > for the current process would feel more intuitive to me.
> > > 
> > > I think you're referring to the earlier discussion about exposing
> > > priority directly via the cgroups filesystem?  That would certainly be
> > > simpler from a userspace perspective, but it's not the direction that
> > > the cgroups maintainer wants to see things go.  Adding files directly to
> > > the cgroups filesystem is supposed to be something that's reserved for
> > > official cgroups controllers.  The GPU priority concept we're trying to
> > > add here doesn't align with the requirements for creating a controller,
> > > so the preferred approach is to create a custom interface (syscall or
> > > ioctl) that simply takes a cgroup as a parameter.  There's precendent
> > > with similar interfaces in areas like BPF (where the bpf() system call
> > > can accept a cgroup as a parameter and then perform its own private
> > > policy changes as it sees fit).
> > > 
> > > Using a true cgroups controller and exposing settings via the filesystem
> > > is likely still the way we'll want to go for some other types of
> > > cgroups-based policy in the future (e.g., limiting GPU memory usage); it
> > > just isn't the appropriate direction for priority.
> > 
> > Might be just me but feels bit crazy to be setting GPU memory usage
> > through another interface and then doing i915 specific IOCTLs to control
> > the priority of that same cgroup.
> > 
> > I don't feel comfortable adding custom cgroups dependent IOCTLs to i915
> > where cgroups is only working as the variable carrier in background. We
> > should really just be consuming a variable from cgroups and it should be
> > set outside of of the i915 IOCTL interface.
> > 
> > I'm still seeing that we should have a DRM cgroups controller and a DRM
> > subsystem wide application to control the priority and memory usage
> > to be fed to the drivers.
> > 
> > If we end up just supporting i915 apps, we could as well use LD_PRELOAD
> > wrapper and alter the context priority at creation time for exactly the
> > same effect and no extra interfaces to maintain.
> > 
> > > > Does the implementation mimic some existing cgroups tool or de-facto way
> > > > of doing things in cgroups world?
> > > 
> > > The ioctl approach I took is similar to syscall approach that the BPF
> > > guys use to attach BPF programs to a cgroup.  I'm not very familiar with
> > > BPF or how it gets used from userspace, so I'm not sure whether the
> > > interface is intended for one specific tool (like ours is), or whether
> > > there's more variety for userspace consumers.
> > 
> > Is the proposal to set the memory usage from similar interface, or is
> > that still not implemented?
> > 
> > I'm seeing a very close relation between time-slicing GPU time and
> > allowed GPU buffer allocations, so having two completely different
> > interfaces does just feel very hackish way of implementing this.
> > 
> > Regards, Joonas
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > 
> > > > Regards, Joonas
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe cgroups" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development
> > > Intel Corporation
> > > (916) 356-2795
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM cgroups integration (Was: Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration)

[Matt Roper]

On Thu, Apr 05, 2018 at 07:49:44AM -0700, Matt Roper wrote:
> On Thu, Apr 05, 2018 at 05:15:13PM +0300, Joonas Lahtinen wrote:
> > + Some more Cc's based on IRC discussion
> > 
> > Quoting Joonas Lahtinen (2018-04-05 16:46:51)
> > > + Dave for commenting from DRM subsystem perspective. I strongly believe
> > > there would be benefit from agreeing on some foundation of DRM subsystem
> > > level program GPU niceness [-20,19] and memory limit [0,N] pages.
> 
> +Chris Wilson

+more Cc's based on IRC discussion.


Matt

> 
> If we ignore backward compatibility and ABI issues for now and assume
> all drivers can move to [-20, 19] for application-accessible priority
> ranges (i.e., self opt-in via existing context parameter ioctls and
> such), I think we still need a much larger range for the priority offset
> assigned via cgroup.  One of the goals with the cgroup work is to give
> the system integrator the ability to define classes of applications with
> non-overlapping ranges (i.e., in some systems an app in the "very
> important" cgroup that self-lowers itself as far as possible should
> still be prioritized higher than an app in the "best effort" cgroup that
> self-boosts itself as far as possible).  Chris Wilson and I discussed
> that a bit on this thread if you want to see the context:
> 
> https://lists.freedesktop.org/archives/intel-gfx/2018-March/158204.html
> 
> That discussion was based on i915's current application-accessible range
> of [-1023,1023].  If we shift to a much smaller [-20,19] range for
> applications to use directly, then we might want to allow cgroup offset
> to be something like [-1000,1000] to ensure the system integrator has
> enough flexibility?
> 
> Also note that "display boost" (i.e., a priority boost for contexts that
> are blocking a flip) may vary depending on system setup, so setting
> being able to define the display boost's effective priority via cgroup
> is important as well.
> 
> 
> Matt
> 
> 
> 
> > > 
> > > Quoting Matt Roper (2018-03-30 03:43:13)
> > > > On Mon, Mar 26, 2018 at 10:30:23AM +0300, Joonas Lahtinen wrote:
> > > > > Quoting Matt Roper (2018-03-23 17:46:16)
> > > > > > On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
> > > > > > > Quoting Matt Roper (2018-03-17 02:08:57)
> > > > > > > > This is the fourth iteration of the work previously posted here:
> > > > > > > >   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
> > > > > > > >   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
> > > > > > > >   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> > > > > > > > 
> > > > > > > > The high level goal of this work is to allow non-cgroup-controller parts
> > > > > > > > of the kernel (e.g., device drivers) to register their own private
> > > > > > > > policy data for specific cgroups.  That mechanism is then made use of in
> > > > > > > > the i915 graphics driver to allow GPU priority to be assigned according
> > > > > > > > to the cgroup membership of the owning process.  Please see the v1 cover
> > > > > > > > letter linked above for a more in-depth explanation and justification.
> > > > > > > 
> > > > > > > Hi Matt,
> > > > > > > 
> > > > > > > For cross-subsystem changes such as this, it makes sense to Cc all
> > > > > > > relevant maintainers, especially if there have been previous comments to
> > > > > > > earlier revisions.
> > > > > > > 
> > > > > > > Please, do include and keep a reference to the userspace portion of the
> > > > > > > changes when you suggest new uAPI to be added. At least I have some trouble
> > > > > > > trying to track down the relevant interface consumer here.
> > > > > > > 
> > > > > > > I'm unsure how much sense it makes to commence with detailed i915 review
> > > > > > > if we will be blocked by lack of userspace after that? I'm assuming
> > > > > > > you've read through [1] already.
> > > > > > 
> > > > > > Hi Joonas,
> > > > > > 
> > > > > > I've sent the userspace code out a few times, but it looks like I forgot
> > > > > > to include a copy with v4/v4.5.  Here's the version I provided with v3:
> > > > > >   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html
> > > > > 
> > > > > Thanks. Keeping that in the relevant commit message of the patch that
> > > > > introduces the new uAPI will make it harder to forget and easiest for
> > > > > git blame, too.
> > > > > 
> > > > > > 
> > > > > > Usually we don't consider things like i-g-t to be sufficient userspace
> > > > > > consumers because we need a real-world consumer rather than a "toy"
> > > > > > userspace.  However in this case, the i-g-t tool, although very simple,
> > > > > > is really the only userspace consumer I expect there to ever be.
> > > > > > Ultimately the true consumer of this cgroups work are bash scripts, sysv
> > > > > > init scripts, systemd recipes, etc.  that just need a very simple tool
> > > > > > to assign the specific values that make sense on a given system.
> > > > > > There's no expectation that graphics clients or display servers would
> > > > > > ever need to make use of these interfaces.
> > > > > 
> > > > > I was under the impression that a bit more generic GPU cgroups support
> > > > > was receiving a lot of support in the early discussion? A dedicated
> > > > > intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
> > > > > for user adoption :)
> > > > 
> > > > I'm open to moving the cgroup_priv registration/lookup to the DRM core
> > > > if other drivers are interested in using this mechanism and if we can
> > > > come to an agreement on a standard priority offset range to support, how
> > > > display boost should work for all drivers, etc.  There might be some
> > > > challenges mapping a DRM-defined priority range down to a different
> > > > range that makes sense for individual driver schedulers, especially
> > > > since some drivers already expose a different priority scheme to
> > > > userspace via other interfaces like i915 does with GEM context priority.
> > > > 
> > > > So far I haven't really heard any interest outside the Intel camp, but
> > > > hopefully other driver teams can speak up if they're for/against this.
> > > > I don't want to try to artificially standardize this if other drivers
> > > > want to go a different direction with priority/scheduling that's too
> > > > different from the current Intel-driven design.
> > > 
> > > I don't think there are that many directions to go about GPU context
> > > priority, considering we have the EGL_IMG_context_priority extension, so
> > > it'll only be about granularity of the scale.
> > > 
> > > I would suggest to go with the nice like scale for easy user adoption,
> > > then just apply that as the N most significant bits.
> > > 
> > > The contexts could then of course further adjust their priority from what
> > > is set by the "gpu_nice" application with the remaining bits.
> > > 
> > > I'm strongly feeling this should be a DRM level "gpu_nice". And the
> > > binding to cgroups should come through DRM core. If it doesn't, limiting
> > > the amount of memory used becomes awkward as the allocation is
> > > centralized to DRM core.
> > > 
> > > > > Also, I might not be up-to-date about all things cgroups, but the way
> > > > > intel_cgroup works, feels bit forced. We create a userspace context just
> > > > > to communicate with the driver and the IOCTL will still have global
> > > > > effects. I can't but think that i915 reading from the cgroups subsystem
> > > > > for the current process would feel more intuitive to me.
> > > > 
> > > > I think you're referring to the earlier discussion about exposing
> > > > priority directly via the cgroups filesystem?  That would certainly be
> > > > simpler from a userspace perspective, but it's not the direction that
> > > > the cgroups maintainer wants to see things go.  Adding files directly to
> > > > the cgroups filesystem is supposed to be something that's reserved for
> > > > official cgroups controllers.  The GPU priority concept we're trying to
> > > > add here doesn't align with the requirements for creating a controller,
> > > > so the preferred approach is to create a custom interface (syscall or
> > > > ioctl) that simply takes a cgroup as a parameter.  There's precendent
> > > > with similar interfaces in areas like BPF (where the bpf() system call
> > > > can accept a cgroup as a parameter and then perform its own private
> > > > policy changes as it sees fit).
> > > > 
> > > > Using a true cgroups controller and exposing settings via the filesystem
> > > > is likely still the way we'll want to go for some other types of
> > > > cgroups-based policy in the future (e.g., limiting GPU memory usage); it
> > > > just isn't the appropriate direction for priority.
> > > 
> > > Might be just me but feels bit crazy to be setting GPU memory usage
> > > through another interface and then doing i915 specific IOCTLs to control
> > > the priority of that same cgroup.
> > > 
> > > I don't feel comfortable adding custom cgroups dependent IOCTLs to i915
> > > where cgroups is only working as the variable carrier in background. We
> > > should really just be consuming a variable from cgroups and it should be
> > > set outside of of the i915 IOCTL interface.
> > > 
> > > I'm still seeing that we should have a DRM cgroups controller and a DRM
> > > subsystem wide application to control the priority and memory usage
> > > to be fed to the drivers.
> > > 
> > > If we end up just supporting i915 apps, we could as well use LD_PRELOAD
> > > wrapper and alter the context priority at creation time for exactly the
> > > same effect and no extra interfaces to maintain.
> > > 
> > > > > Does the implementation mimic some existing cgroups tool or de-facto way
> > > > > of doing things in cgroups world?
> > > > 
> > > > The ioctl approach I took is similar to syscall approach that the BPF
> > > > guys use to attach BPF programs to a cgroup.  I'm not very familiar with
> > > > BPF or how it gets used from userspace, so I'm not sure whether the
> > > > interface is intended for one specific tool (like ours is), or whether
> > > > there's more variety for userspace consumers.
> > > 
> > > Is the proposal to set the memory usage from similar interface, or is
> > > that still not implemented?
> > > 
> > > I'm seeing a very close relation between time-slicing GPU time and
> > > allowed GPU buffer allocations, so having two completely different
> > > interfaces does just feel very hackish way of implementing this.
> > > 
> > > Regards, Joonas
> > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > > 
> > > > > Regards, Joonas
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe cgroups" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > IoTG Platform Enabling & Development
> > > > Intel Corporation
> > > > (916) 356-2795
> > --
> > To unsubscribe from this list: send the line "unsubscribe cgroups" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: DRM cgroups integration (Was: Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration)

[Matt Roper]

Just realized Joonas had some other comments down at the bottom that I
missed originally...

On Thu, Apr 05, 2018 at 08:06:23AM -0700, Matt Roper wrote:
> On Thu, Apr 05, 2018 at 07:49:44AM -0700, Matt Roper wrote:
> > On Thu, Apr 05, 2018 at 05:15:13PM +0300, Joonas Lahtinen wrote:
> > > + Some more Cc's based on IRC discussion
> > > 
> > > Quoting Joonas Lahtinen (2018-04-05 16:46:51)
> > > > + Dave for commenting from DRM subsystem perspective. I strongly believe
> > > > there would be benefit from agreeing on some foundation of DRM subsystem
> > > > level program GPU niceness [-20,19] and memory limit [0,N] pages.
> > 
> > +Chris Wilson
> 
> +more Cc's based on IRC discussion.
> 
> 
> Matt
> 
> > 
> > If we ignore backward compatibility and ABI issues for now and assume
> > all drivers can move to [-20, 19] for application-accessible priority
> > ranges (i.e., self opt-in via existing context parameter ioctls and
> > such), I think we still need a much larger range for the priority offset
> > assigned via cgroup.  One of the goals with the cgroup work is to give
> > the system integrator the ability to define classes of applications with
> > non-overlapping ranges (i.e., in some systems an app in the "very
> > important" cgroup that self-lowers itself as far as possible should
> > still be prioritized higher than an app in the "best effort" cgroup that
> > self-boosts itself as far as possible).  Chris Wilson and I discussed
> > that a bit on this thread if you want to see the context:
> > 
> > https://lists.freedesktop.org/archives/intel-gfx/2018-March/158204.html
> > 
> > That discussion was based on i915's current application-accessible range
> > of [-1023,1023].  If we shift to a much smaller [-20,19] range for
> > applications to use directly, then we might want to allow cgroup offset
> > to be something like [-1000,1000] to ensure the system integrator has
> > enough flexibility?
> > 
> > Also note that "display boost" (i.e., a priority boost for contexts that
> > are blocking a flip) may vary depending on system setup, so setting
> > being able to define the display boost's effective priority via cgroup
> > is important as well.
> > 
> > 
> > Matt
> > 
> > 
> > 
> > > > 
> > > > Quoting Matt Roper (2018-03-30 03:43:13)
> > > > > On Mon, Mar 26, 2018 at 10:30:23AM +0300, Joonas Lahtinen wrote:
> > > > > > Quoting Matt Roper (2018-03-23 17:46:16)
> > > > > > > On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
> > > > > > > > Quoting Matt Roper (2018-03-17 02:08:57)
> > > > > > > > > This is the fourth iteration of the work previously posted here:
> > > > > > > > >   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
> > > > > > > > >   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
> > > > > > > > >   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
> > > > > > > > > 
> > > > > > > > > The high level goal of this work is to allow non-cgroup-controller parts
> > > > > > > > > of the kernel (e.g., device drivers) to register their own private
> > > > > > > > > policy data for specific cgroups.  That mechanism is then made use of in
> > > > > > > > > the i915 graphics driver to allow GPU priority to be assigned according
> > > > > > > > > to the cgroup membership of the owning process.  Please see the v1 cover
> > > > > > > > > letter linked above for a more in-depth explanation and justification.
> > > > > > > > 
> > > > > > > > Hi Matt,
> > > > > > > > 
> > > > > > > > For cross-subsystem changes such as this, it makes sense to Cc all
> > > > > > > > relevant maintainers, especially if there have been previous comments to
> > > > > > > > earlier revisions.
> > > > > > > > 
> > > > > > > > Please, do include and keep a reference to the userspace portion of the
> > > > > > > > changes when you suggest new uAPI to be added. At least I have some trouble
> > > > > > > > trying to track down the relevant interface consumer here.
> > > > > > > > 
> > > > > > > > I'm unsure how much sense it makes to commence with detailed i915 review
> > > > > > > > if we will be blocked by lack of userspace after that? I'm assuming
> > > > > > > > you've read through [1] already.
> > > > > > > 
> > > > > > > Hi Joonas,
> > > > > > > 
> > > > > > > I've sent the userspace code out a few times, but it looks like I forgot
> > > > > > > to include a copy with v4/v4.5.  Here's the version I provided with v3:
> > > > > > >   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html
> > > > > > 
> > > > > > Thanks. Keeping that in the relevant commit message of the patch that
> > > > > > introduces the new uAPI will make it harder to forget and easiest for
> > > > > > git blame, too.
> > > > > > 
> > > > > > > 
> > > > > > > Usually we don't consider things like i-g-t to be sufficient userspace
> > > > > > > consumers because we need a real-world consumer rather than a "toy"
> > > > > > > userspace.  However in this case, the i-g-t tool, although very simple,
> > > > > > > is really the only userspace consumer I expect there to ever be.
> > > > > > > Ultimately the true consumer of this cgroups work are bash scripts, sysv
> > > > > > > init scripts, systemd recipes, etc.  that just need a very simple tool
> > > > > > > to assign the specific values that make sense on a given system.
> > > > > > > There's no expectation that graphics clients or display servers would
> > > > > > > ever need to make use of these interfaces.
> > > > > > 
> > > > > > I was under the impression that a bit more generic GPU cgroups support
> > > > > > was receiving a lot of support in the early discussion? A dedicated
> > > > > > intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
> > > > > > for user adoption :)
> > > > > 
> > > > > I'm open to moving the cgroup_priv registration/lookup to the DRM core
> > > > > if other drivers are interested in using this mechanism and if we can
> > > > > come to an agreement on a standard priority offset range to support, how
> > > > > display boost should work for all drivers, etc.  There might be some
> > > > > challenges mapping a DRM-defined priority range down to a different
> > > > > range that makes sense for individual driver schedulers, especially
> > > > > since some drivers already expose a different priority scheme to
> > > > > userspace via other interfaces like i915 does with GEM context priority.
> > > > > 
> > > > > So far I haven't really heard any interest outside the Intel camp, but
> > > > > hopefully other driver teams can speak up if they're for/against this.
> > > > > I don't want to try to artificially standardize this if other drivers
> > > > > want to go a different direction with priority/scheduling that's too
> > > > > different from the current Intel-driven design.
> > > > 
> > > > I don't think there are that many directions to go about GPU context
> > > > priority, considering we have the EGL_IMG_context_priority extension, so
> > > > it'll only be about granularity of the scale.
> > > > 
> > > > I would suggest to go with the nice like scale for easy user adoption,
> > > > then just apply that as the N most significant bits.
> > > > 
> > > > The contexts could then of course further adjust their priority from what
> > > > is set by the "gpu_nice" application with the remaining bits.
> > > > 
> > > > I'm strongly feeling this should be a DRM level "gpu_nice". And the
> > > > binding to cgroups should come through DRM core. If it doesn't, limiting
> > > > the amount of memory used becomes awkward as the allocation is
> > > > centralized to DRM core.

My earliest iterations of this work did add the interfaces to set
cgroup-based GPU priority at the DRM core.  I didn't really get the
sense at the time that anyone outside of Intel was interested in the
cgroups work, so I moved the interfaces back into i915 for subequent
updates, but I'm happy to move it back.  Note that even in that case
this will still end up as a DRM core ioctl rather than a true cgroups
controller; see below.

> > > > 
> > > > > > Also, I might not be up-to-date about all things cgroups, but the way
> > > > > > intel_cgroup works, feels bit forced. We create a userspace context just
> > > > > > to communicate with the driver and the IOCTL will still have global
> > > > > > effects. I can't but think that i915 reading from the cgroups subsystem
> > > > > > for the current process would feel more intuitive to me.
> > > > > 
> > > > > I think you're referring to the earlier discussion about exposing
> > > > > priority directly via the cgroups filesystem?  That would certainly be
> > > > > simpler from a userspace perspective, but it's not the direction that
> > > > > the cgroups maintainer wants to see things go.  Adding files directly to
> > > > > the cgroups filesystem is supposed to be something that's reserved for
> > > > > official cgroups controllers.  The GPU priority concept we're trying to
> > > > > add here doesn't align with the requirements for creating a controller,
> > > > > so the preferred approach is to create a custom interface (syscall or
> > > > > ioctl) that simply takes a cgroup as a parameter.  There's precendent
> > > > > with similar interfaces in areas like BPF (where the bpf() system call
> > > > > can accept a cgroup as a parameter and then perform its own private
> > > > > policy changes as it sees fit).
> > > > > 
> > > > > Using a true cgroups controller and exposing settings via the filesystem
> > > > > is likely still the way we'll want to go for some other types of
> > > > > cgroups-based policy in the future (e.g., limiting GPU memory usage); it
> > > > > just isn't the appropriate direction for priority.
> > > > 
> > > > Might be just me but feels bit crazy to be setting GPU memory usage
> > > > through another interface and then doing i915 specific IOCTLs to control
> > > > the priority of that same cgroup.
> > > > 
> > > > I don't feel comfortable adding custom cgroups dependent IOCTLs to i915
> > > > where cgroups is only working as the variable carrier in background. We
> > > > should really just be consuming a variable from cgroups and it should be
> > > > set outside of of the i915 IOCTL interface.
> > > > 
> > > > I'm still seeing that we should have a DRM cgroups controller and a DRM
> > > > subsystem wide application to control the priority and memory usage
> > > > to be fed to the drivers.

A cgroups controller could be used for GPU memory management, but not
for priority adjustment the way we're doing it right now.  Although
there's some preexisting legacy stuff that doesn't quite follow the
rules, the cgroups subsystem will only accept new cgroup controllers if
they take the entire structure of the cgroups hierarchy into account
(rather than just looking at values at a leaf node) and are based on one
of the four resource distribution models described in the
Documentation/cgroup-v2.txt (weights, limits, protections, allocations).
GPU priority is somewhat close to a "weight" but not quite --- a true
weight wouldn't allow unfair scheduling or intentional starvation which
are very important to the use cases I'm focusing on.

The cgroups maintainer has suggested that a separate interface (syscall,
ioctl, etc.) be used for items like this that don't meet the criteria
for a full cgroups controller, and there are other examples of this kind
of design elsewhere in the kernel too.

> > > > 
> > > > If we end up just supporting i915 apps, we could as well use LD_PRELOAD
> > > > wrapper and alter the context priority at creation time for exactly the
> > > > same effect and no extra interfaces to maintain.

I think this is missing the point of using cgroups.  cgroups allow
policy to be set independently by a privileged system integrator or
system administrator, in a way that applications themselves don't have
access to.  LD_PRELOAD would only have access to the interfaces
available to the app itself and the app could simply undo whatever
setting the LD_PRELOAD tried to use.

The goal with cgroups isn't to provide a "gpu nice" where you
self-classify yourself, but rather the ability for the person defining
how the overall system works to classify and control the various types
of applications running in a transparent and global way.

> > > > 
> > > > > > Does the implementation mimic some existing cgroups tool or de-facto way
> > > > > > of doing things in cgroups world?
> > > > > 
> > > > > The ioctl approach I took is similar to syscall approach that the BPF
> > > > > guys use to attach BPF programs to a cgroup.  I'm not very familiar with
> > > > > BPF or how it gets used from userspace, so I'm not sure whether the
> > > > > interface is intended for one specific tool (like ours is), or whether
> > > > > there's more variety for userspace consumers.
> > > > 
> > > > Is the proposal to set the memory usage from similar interface, or is
> > > > that still not implemented?

GPU memory management is a very different resource model, so I'd expect
memory to be managed by a real cgroups controller. I.e., the interface
would be different.  I haven't personally done any work on cgroups-based
memory management; I agree that would be nice to have, but none of the
use cases my team is focused on have that as a requirement.

> > > > 
> > > > I'm seeing a very close relation between time-slicing GPU time and
> > > > allowed GPU buffer allocations, so having two completely different
> > > > interfaces does just feel very hackish way of implementing this.

If we were time slicing GPU time and giving out fractional shares of
time to each client, that would be a very different situation than we
have today; in that case I agree that a cgroups controller would be the
way to go.  While there probably are a lot of cases where time slicing
and fair scheduling would be beneficial (especially stuff like desktop
or mobile settings), I'm mostly focused on embedded use cases that
depend on today's unfair, strict priority scheduling.


Matt

> > > > 
> > > > Regards, Joonas
> > > > 
> > > > > 
> > > > > 
> > > > > Matt
> > > > > 
> > > > > > 
> > > > > > Regards, Joonas
> > > > > > --
> > > > > > To unsubscribe from this list: send the line "unsubscribe cgroups" in
> > > > > > the body of a message to majordomo@vger.kernel.org
> > > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > 
> > > > > -- 
> > > > > Matt Roper
> > > > > Graphics Software Engineer
> > > > > IoTG Platform Enabling & Development
> > > > > Intel Corporation
> > > > > (916) 356-2795
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe cgroups" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DRM cgroups integration (Was: Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration)

[Felix Kuehling]

Adding Kenny, who recently started looking into cgroup support from the
AMD GPU computing side.

Regards,
  Felix


On 2018-04-05 11:48 AM, Matt Roper wrote:
> Just realized Joonas had some other comments down at the bottom that I
> missed originally...
>
> On Thu, Apr 05, 2018 at 08:06:23AM -0700, Matt Roper wrote:
>> On Thu, Apr 05, 2018 at 07:49:44AM -0700, Matt Roper wrote:
>>> On Thu, Apr 05, 2018 at 05:15:13PM +0300, Joonas Lahtinen wrote:
>>>> + Some more Cc's based on IRC discussion
>>>>
>>>> Quoting Joonas Lahtinen (2018-04-05 16:46:51)
>>>>> + Dave for commenting from DRM subsystem perspective. I strongly believe
>>>>> there would be benefit from agreeing on some foundation of DRM subsystem
>>>>> level program GPU niceness [-20,19] and memory limit [0,N] pages.
>>> +Chris Wilson
>> +more Cc's based on IRC discussion.
>>
>>
>> Matt
>>
>>> If we ignore backward compatibility and ABI issues for now and assume
>>> all drivers can move to [-20, 19] for application-accessible priority
>>> ranges (i.e., self opt-in via existing context parameter ioctls and
>>> such), I think we still need a much larger range for the priority offset
>>> assigned via cgroup.  One of the goals with the cgroup work is to give
>>> the system integrator the ability to define classes of applications with
>>> non-overlapping ranges (i.e., in some systems an app in the "very
>>> important" cgroup that self-lowers itself as far as possible should
>>> still be prioritized higher than an app in the "best effort" cgroup that
>>> self-boosts itself as far as possible).  Chris Wilson and I discussed
>>> that a bit on this thread if you want to see the context:
>>>
>>> https://lists.freedesktop.org/archives/intel-gfx/2018-March/158204.html
>>>
>>> That discussion was based on i915's current application-accessible range
>>> of [-1023,1023].  If we shift to a much smaller [-20,19] range for
>>> applications to use directly, then we might want to allow cgroup offset
>>> to be something like [-1000,1000] to ensure the system integrator has
>>> enough flexibility?
>>>
>>> Also note that "display boost" (i.e., a priority boost for contexts that
>>> are blocking a flip) may vary depending on system setup, so setting
>>> being able to define the display boost's effective priority via cgroup
>>> is important as well.
>>>
>>>
>>> Matt
>>>
>>>
>>>
>>>>> Quoting Matt Roper (2018-03-30 03:43:13)
>>>>>> On Mon, Mar 26, 2018 at 10:30:23AM +0300, Joonas Lahtinen wrote:
>>>>>>> Quoting Matt Roper (2018-03-23 17:46:16)
>>>>>>>> On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
>>>>>>>>> Quoting Matt Roper (2018-03-17 02:08:57)
>>>>>>>>>> This is the fourth iteration of the work previously posted here:
>>>>>>>>>>   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
>>>>>>>>>>   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
>>>>>>>>>>   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
>>>>>>>>>>
>>>>>>>>>> The high level goal of this work is to allow non-cgroup-controller parts
>>>>>>>>>> of the kernel (e.g., device drivers) to register their own private
>>>>>>>>>> policy data for specific cgroups.  That mechanism is then made use of in
>>>>>>>>>> the i915 graphics driver to allow GPU priority to be assigned according
>>>>>>>>>> to the cgroup membership of the owning process.  Please see the v1 cover
>>>>>>>>>> letter linked above for a more in-depth explanation and justification.
>>>>>>>>> Hi Matt,
>>>>>>>>>
>>>>>>>>> For cross-subsystem changes such as this, it makes sense to Cc all
>>>>>>>>> relevant maintainers, especially if there have been previous comments to
>>>>>>>>> earlier revisions.
>>>>>>>>>
>>>>>>>>> Please, do include and keep a reference to the userspace portion of the
>>>>>>>>> changes when you suggest new uAPI to be added. At least I have some trouble
>>>>>>>>> trying to track down the relevant interface consumer here.
>>>>>>>>>
>>>>>>>>> I'm unsure how much sense it makes to commence with detailed i915 review
>>>>>>>>> if we will be blocked by lack of userspace after that? I'm assuming
>>>>>>>>> you've read through [1] already.
>>>>>>>> Hi Joonas,
>>>>>>>>
>>>>>>>> I've sent the userspace code out a few times, but it looks like I forgot
>>>>>>>> to include a copy with v4/v4.5.  Here's the version I provided with v3:
>>>>>>>>   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html
>>>>>>> Thanks. Keeping that in the relevant commit message of the patch that
>>>>>>> introduces the new uAPI will make it harder to forget and easiest for
>>>>>>> git blame, too.
>>>>>>>
>>>>>>>> Usually we don't consider things like i-g-t to be sufficient userspace
>>>>>>>> consumers because we need a real-world consumer rather than a "toy"
>>>>>>>> userspace.  However in this case, the i-g-t tool, although very simple,
>>>>>>>> is really the only userspace consumer I expect there to ever be.
>>>>>>>> Ultimately the true consumer of this cgroups work are bash scripts, sysv
>>>>>>>> init scripts, systemd recipes, etc.  that just need a very simple tool
>>>>>>>> to assign the specific values that make sense on a given system.
>>>>>>>> There's no expectation that graphics clients or display servers would
>>>>>>>> ever need to make use of these interfaces.
>>>>>>> I was under the impression that a bit more generic GPU cgroups support
>>>>>>> was receiving a lot of support in the early discussion? A dedicated
>>>>>>> intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
>>>>>>> for user adoption :)
>>>>>> I'm open to moving the cgroup_priv registration/lookup to the DRM core
>>>>>> if other drivers are interested in using this mechanism and if we can
>>>>>> come to an agreement on a standard priority offset range to support, how
>>>>>> display boost should work for all drivers, etc.  There might be some
>>>>>> challenges mapping a DRM-defined priority range down to a different
>>>>>> range that makes sense for individual driver schedulers, especially
>>>>>> since some drivers already expose a different priority scheme to
>>>>>> userspace via other interfaces like i915 does with GEM context priority.
>>>>>>
>>>>>> So far I haven't really heard any interest outside the Intel camp, but
>>>>>> hopefully other driver teams can speak up if they're for/against this.
>>>>>> I don't want to try to artificially standardize this if other drivers
>>>>>> want to go a different direction with priority/scheduling that's too
>>>>>> different from the current Intel-driven design.
>>>>> I don't think there are that many directions to go about GPU context
>>>>> priority, considering we have the EGL_IMG_context_priority extension, so
>>>>> it'll only be about granularity of the scale.
>>>>>
>>>>> I would suggest to go with the nice like scale for easy user adoption,
>>>>> then just apply that as the N most significant bits.
>>>>>
>>>>> The contexts could then of course further adjust their priority from what
>>>>> is set by the "gpu_nice" application with the remaining bits.
>>>>>
>>>>> I'm strongly feeling this should be a DRM level "gpu_nice". And the
>>>>> binding to cgroups should come through DRM core. If it doesn't, limiting
>>>>> the amount of memory used becomes awkward as the allocation is
>>>>> centralized to DRM core.
> My earliest iterations of this work did add the interfaces to set
> cgroup-based GPU priority at the DRM core.  I didn't really get the
> sense at the time that anyone outside of Intel was interested in the
> cgroups work, so I moved the interfaces back into i915 for subequent
> updates, but I'm happy to move it back.  Note that even in that case
> this will still end up as a DRM core ioctl rather than a true cgroups
> controller; see below.
>
>>>>>>> Also, I might not be up-to-date about all things cgroups, but the way
>>>>>>> intel_cgroup works, feels bit forced. We create a userspace context just
>>>>>>> to communicate with the driver and the IOCTL will still have global
>>>>>>> effects. I can't but think that i915 reading from the cgroups subsystem
>>>>>>> for the current process would feel more intuitive to me.
>>>>>> I think you're referring to the earlier discussion about exposing
>>>>>> priority directly via the cgroups filesystem?  That would certainly be
>>>>>> simpler from a userspace perspective, but it's not the direction that
>>>>>> the cgroups maintainer wants to see things go.  Adding files directly to
>>>>>> the cgroups filesystem is supposed to be something that's reserved for
>>>>>> official cgroups controllers.  The GPU priority concept we're trying to
>>>>>> add here doesn't align with the requirements for creating a controller,
>>>>>> so the preferred approach is to create a custom interface (syscall or
>>>>>> ioctl) that simply takes a cgroup as a parameter.  There's precendent
>>>>>> with similar interfaces in areas like BPF (where the bpf() system call
>>>>>> can accept a cgroup as a parameter and then perform its own private
>>>>>> policy changes as it sees fit).
>>>>>>
>>>>>> Using a true cgroups controller and exposing settings via the filesystem
>>>>>> is likely still the way we'll want to go for some other types of
>>>>>> cgroups-based policy in the future (e.g., limiting GPU memory usage); it
>>>>>> just isn't the appropriate direction for priority.
>>>>> Might be just me but feels bit crazy to be setting GPU memory usage
>>>>> through another interface and then doing i915 specific IOCTLs to control
>>>>> the priority of that same cgroup.
>>>>>
>>>>> I don't feel comfortable adding custom cgroups dependent IOCTLs to i915
>>>>> where cgroups is only working as the variable carrier in background. We
>>>>> should really just be consuming a variable from cgroups and it should be
>>>>> set outside of of the i915 IOCTL interface.
>>>>>
>>>>> I'm still seeing that we should have a DRM cgroups controller and a DRM
>>>>> subsystem wide application to control the priority and memory usage
>>>>> to be fed to the drivers.
> A cgroups controller could be used for GPU memory management, but not
> for priority adjustment the way we're doing it right now.  Although
> there's some preexisting legacy stuff that doesn't quite follow the
> rules, the cgroups subsystem will only accept new cgroup controllers if
> they take the entire structure of the cgroups hierarchy into account
> (rather than just looking at values at a leaf node) and are based on one
> of the four resource distribution models described in the
> Documentation/cgroup-v2.txt (weights, limits, protections, allocations).
> GPU priority is somewhat close to a "weight" but not quite --- a true
> weight wouldn't allow unfair scheduling or intentional starvation which
> are very important to the use cases I'm focusing on.
>
> The cgroups maintainer has suggested that a separate interface (syscall,
> ioctl, etc.) be used for items like this that don't meet the criteria
> for a full cgroups controller, and there are other examples of this kind
> of design elsewhere in the kernel too.
>
>>>>> If we end up just supporting i915 apps, we could as well use LD_PRELOAD
>>>>> wrapper and alter the context priority at creation time for exactly the
>>>>> same effect and no extra interfaces to maintain.
> I think this is missing the point of using cgroups.  cgroups allow
> policy to be set independently by a privileged system integrator or
> system administrator, in a way that applications themselves don't have
> access to.  LD_PRELOAD would only have access to the interfaces
> available to the app itself and the app could simply undo whatever
> setting the LD_PRELOAD tried to use.
>
> The goal with cgroups isn't to provide a "gpu nice" where you
> self-classify yourself, but rather the ability for the person defining
> how the overall system works to classify and control the various types
> of applications running in a transparent and global way.
>
>>>>>>> Does the implementation mimic some existing cgroups tool or de-facto way
>>>>>>> of doing things in cgroups world?
>>>>>> The ioctl approach I took is similar to syscall approach that the BPF
>>>>>> guys use to attach BPF programs to a cgroup.  I'm not very familiar with
>>>>>> BPF or how it gets used from userspace, so I'm not sure whether the
>>>>>> interface is intended for one specific tool (like ours is), or whether
>>>>>> there's more variety for userspace consumers.
>>>>> Is the proposal to set the memory usage from similar interface, or is
>>>>> that still not implemented?
> GPU memory management is a very different resource model, so I'd expect
> memory to be managed by a real cgroups controller. I.e., the interface
> would be different.  I haven't personally done any work on cgroups-based
> memory management; I agree that would be nice to have, but none of the
> use cases my team is focused on have that as a requirement.
>
>>>>> I'm seeing a very close relation between time-slicing GPU time and
>>>>> allowed GPU buffer allocations, so having two completely different
>>>>> interfaces does just feel very hackish way of implementing this.
> If we were time slicing GPU time and giving out fractional shares of
> time to each client, that would be a very different situation than we
> have today; in that case I agree that a cgroups controller would be the
> way to go.  While there probably are a lot of cases where time slicing
> and fair scheduling would be beneficial (especially stuff like desktop
> or mobile settings), I'm mostly focused on embedded use cases that
> depend on today's unfair, strict priority scheduling.
>
>
> Matt
>
>>>>> Regards, Joonas
>>>>>
>>>>>>
>>>>>> Matt
>>>>>>
>>>>>>> Regards, Joonas
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe cgroups" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>> -- 
>>>>>> Matt Roper
>>>>>> Graphics Software Engineer
>>>>>> IoTG Platform Enabling & Development
>>>>>> Intel Corporation
>>>>>> (916) 356-2795
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe cgroups" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> -- 
>>> Matt Roper
>>> Graphics Software Engineer
>>> IoTG Platform Enabling & Development
>>> Intel Corporation
>>> (916) 356-2795
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: DRM cgroups integration (Was: Re: [PATCH v4 0/8] cgroup private data and DRM/i915 integration)

[Felix Kuehling]

Adding Kenny, who recently started looking into cgroup support from the
AMD GPU computing side.

Regards,
  Felix


On 2018-04-05 11:48 AM, Matt Roper wrote:
> Just realized Joonas had some other comments down at the bottom that I
> missed originally...
>
> On Thu, Apr 05, 2018 at 08:06:23AM -0700, Matt Roper wrote:
>> On Thu, Apr 05, 2018 at 07:49:44AM -0700, Matt Roper wrote:
>>> On Thu, Apr 05, 2018 at 05:15:13PM +0300, Joonas Lahtinen wrote:
>>>> + Some more Cc's based on IRC discussion
>>>>
>>>> Quoting Joonas Lahtinen (2018-04-05 16:46:51)
>>>>> + Dave for commenting from DRM subsystem perspective. I strongly believe
>>>>> there would be benefit from agreeing on some foundation of DRM subsystem
>>>>> level program GPU niceness [-20,19] and memory limit [0,N] pages.
>>> +Chris Wilson
>> +more Cc's based on IRC discussion.
>>
>>
>> Matt
>>
>>> If we ignore backward compatibility and ABI issues for now and assume
>>> all drivers can move to [-20, 19] for application-accessible priority
>>> ranges (i.e., self opt-in via existing context parameter ioctls and
>>> such), I think we still need a much larger range for the priority offset
>>> assigned via cgroup.  One of the goals with the cgroup work is to give
>>> the system integrator the ability to define classes of applications with
>>> non-overlapping ranges (i.e., in some systems an app in the "very
>>> important" cgroup that self-lowers itself as far as possible should
>>> still be prioritized higher than an app in the "best effort" cgroup that
>>> self-boosts itself as far as possible).  Chris Wilson and I discussed
>>> that a bit on this thread if you want to see the context:
>>>
>>> https://lists.freedesktop.org/archives/intel-gfx/2018-March/158204.html
>>>
>>> That discussion was based on i915's current application-accessible range
>>> of [-1023,1023].  If we shift to a much smaller [-20,19] range for
>>> applications to use directly, then we might want to allow cgroup offset
>>> to be something like [-1000,1000] to ensure the system integrator has
>>> enough flexibility?
>>>
>>> Also note that "display boost" (i.e., a priority boost for contexts that
>>> are blocking a flip) may vary depending on system setup, so setting
>>> being able to define the display boost's effective priority via cgroup
>>> is important as well.
>>>
>>>
>>> Matt
>>>
>>>
>>>
>>>>> Quoting Matt Roper (2018-03-30 03:43:13)
>>>>>> On Mon, Mar 26, 2018 at 10:30:23AM +0300, Joonas Lahtinen wrote:
>>>>>>> Quoting Matt Roper (2018-03-23 17:46:16)
>>>>>>>> On Fri, Mar 23, 2018 at 02:15:38PM +0200, Joonas Lahtinen wrote:
>>>>>>>>> Quoting Matt Roper (2018-03-17 02:08:57)
>>>>>>>>>> This is the fourth iteration of the work previously posted here:
>>>>>>>>>>   (v1) https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html
>>>>>>>>>>   (v2) https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg208170.html
>>>>>>>>>>   (v3) https://lists.freedesktop.org/archives/intel-gfx/2018-March/157928.html
>>>>>>>>>>
>>>>>>>>>> The high level goal of this work is to allow non-cgroup-controller parts
>>>>>>>>>> of the kernel (e.g., device drivers) to register their own private
>>>>>>>>>> policy data for specific cgroups.  That mechanism is then made use of in
>>>>>>>>>> the i915 graphics driver to allow GPU priority to be assigned according
>>>>>>>>>> to the cgroup membership of the owning process.  Please see the v1 cover
>>>>>>>>>> letter linked above for a more in-depth explanation and justification.
>>>>>>>>> Hi Matt,
>>>>>>>>>
>>>>>>>>> For cross-subsystem changes such as this, it makes sense to Cc all
>>>>>>>>> relevant maintainers, especially if there have been previous comments to
>>>>>>>>> earlier revisions.
>>>>>>>>>
>>>>>>>>> Please, do include and keep a reference to the userspace portion of the
>>>>>>>>> changes when you suggest new uAPI to be added. At least I have some trouble
>>>>>>>>> trying to track down the relevant interface consumer here.
>>>>>>>>>
>>>>>>>>> I'm unsure how much sense it makes to commence with detailed i915 review
>>>>>>>>> if we will be blocked by lack of userspace after that? I'm assuming
>>>>>>>>> you've read through [1] already.
>>>>>>>> Hi Joonas,
>>>>>>>>
>>>>>>>> I've sent the userspace code out a few times, but it looks like I forgot
>>>>>>>> to include a copy with v4/v4.5.  Here's the version I provided with v3:
>>>>>>>>   https://lists.freedesktop.org/archives/intel-gfx/2018-March/157935.html
>>>>>>> Thanks. Keeping that in the relevant commit message of the patch that
>>>>>>> introduces the new uAPI will make it harder to forget and easiest for
>>>>>>> git blame, too.
>>>>>>>
>>>>>>>> Usually we don't consider things like i-g-t to be sufficient userspace
>>>>>>>> consumers because we need a real-world consumer rather than a "toy"
>>>>>>>> userspace.  However in this case, the i-g-t tool, although very simple,
>>>>>>>> is really the only userspace consumer I expect there to ever be.
>>>>>>>> Ultimately the true consumer of this cgroups work are bash scripts, sysv
>>>>>>>> init scripts, systemd recipes, etc.  that just need a very simple tool
>>>>>>>> to assign the specific values that make sense on a given system.
>>>>>>>> There's no expectation that graphics clients or display servers would
>>>>>>>> ever need to make use of these interfaces.
>>>>>>> I was under the impression that a bit more generic GPU cgroups support
>>>>>>> was receiving a lot of support in the early discussion? A dedicated
>>>>>>> intel_cgroup sounds underwhelming, when comparing to idea of "gpu_nice",
>>>>>>> for user adoption :)
>>>>>> I'm open to moving the cgroup_priv registration/lookup to the DRM core
>>>>>> if other drivers are interested in using this mechanism and if we can
>>>>>> come to an agreement on a standard priority offset range to support, how
>>>>>> display boost should work for all drivers, etc.  There might be some
>>>>>> challenges mapping a DRM-defined priority range down to a different
>>>>>> range that makes sense for individual driver schedulers, especially
>>>>>> since some drivers already expose a different priority scheme to
>>>>>> userspace via other interfaces like i915 does with GEM context priority.
>>>>>>
>>>>>> So far I haven't really heard any interest outside the Intel camp, but
>>>>>> hopefully other driver teams can speak up if they're for/against this.
>>>>>> I don't want to try to artificially standardize this if other drivers
>>>>>> want to go a different direction with priority/scheduling that's too
>>>>>> different from the current Intel-driven design.
>>>>> I don't think there are that many directions to go about GPU context
>>>>> priority, considering we have the EGL_IMG_context_priority extension, so
>>>>> it'll only be about granularity of the scale.
>>>>>
>>>>> I would suggest to go with the nice like scale for easy user adoption,
>>>>> then just apply that as the N most significant bits.
>>>>>
>>>>> The contexts could then of course further adjust their priority from what
>>>>> is set by the "gpu_nice" application with the remaining bits.
>>>>>
>>>>> I'm strongly feeling this should be a DRM level "gpu_nice". And the
>>>>> binding to cgroups should come through DRM core. If it doesn't, limiting
>>>>> the amount of memory used becomes awkward as the allocation is
>>>>> centralized to DRM core.
> My earliest iterations of this work did add the interfaces to set
> cgroup-based GPU priority at the DRM core.  I didn't really get the
> sense at the time that anyone outside of Intel was interested in the
> cgroups work, so I moved the interfaces back into i915 for subequent
> updates, but I'm happy to move it back.  Note that even in that case
> this will still end up as a DRM core ioctl rather than a true cgroups
> controller; see below.
>
>>>>>>> Also, I might not be up-to-date about all things cgroups, but the way
>>>>>>> intel_cgroup works, feels bit forced. We create a userspace context just
>>>>>>> to communicate with the driver and the IOCTL will still have global
>>>>>>> effects. I can't but think that i915 reading from the cgroups subsystem
>>>>>>> for the current process would feel more intuitive to me.
>>>>>> I think you're referring to the earlier discussion about exposing
>>>>>> priority directly via the cgroups filesystem?  That would certainly be
>>>>>> simpler from a userspace perspective, but it's not the direction that
>>>>>> the cgroups maintainer wants to see things go.  Adding files directly to
>>>>>> the cgroups filesystem is supposed to be something that's reserved for
>>>>>> official cgroups controllers.  The GPU priority concept we're trying to
>>>>>> add here doesn't align with the requirements for creating a controller,
>>>>>> so the preferred approach is to create a custom interface (syscall or
>>>>>> ioctl) that simply takes a cgroup as a parameter.  There's precendent
>>>>>> with similar interfaces in areas like BPF (where the bpf() system call
>>>>>> can accept a cgroup as a parameter and then perform its own private
>>>>>> policy changes as it sees fit).
>>>>>>
>>>>>> Using a true cgroups controller and exposing settings via the filesystem
>>>>>> is likely still the way we'll want to go for some other types of
>>>>>> cgroups-based policy in the future (e.g., limiting GPU memory usage); it
>>>>>> just isn't the appropriate direction for priority.
>>>>> Might be just me but feels bit crazy to be setting GPU memory usage
>>>>> through another interface and then doing i915 specific IOCTLs to control
>>>>> the priority of that same cgroup.
>>>>>
>>>>> I don't feel comfortable adding custom cgroups dependent IOCTLs to i915
>>>>> where cgroups is only working as the variable carrier in background. We
>>>>> should really just be consuming a variable from cgroups and it should be
>>>>> set outside of of the i915 IOCTL interface.
>>>>>
>>>>> I'm still seeing that we should have a DRM cgroups controller and a DRM
>>>>> subsystem wide application to control the priority and memory usage
>>>>> to be fed to the drivers.
> A cgroups controller could be used for GPU memory management, but not
> for priority adjustment the way we're doing it right now.  Although
> there's some preexisting legacy stuff that doesn't quite follow the
> rules, the cgroups subsystem will only accept new cgroup controllers if
> they take the entire structure of the cgroups hierarchy into account
> (rather than just looking at values at a leaf node) and are based on one
> of the four resource distribution models described in the
> Documentation/cgroup-v2.txt (weights, limits, protections, allocations).
> GPU priority is somewhat close to a "weight" but not quite --- a true
> weight wouldn't allow unfair scheduling or intentional starvation which
> are very important to the use cases I'm focusing on.
>
> The cgroups maintainer has suggested that a separate interface (syscall,
> ioctl, etc.) be used for items like this that don't meet the criteria
> for a full cgroups controller, and there are other examples of this kind
> of design elsewhere in the kernel too.
>
>>>>> If we end up just supporting i915 apps, we could as well use LD_PRELOAD
>>>>> wrapper and alter the context priority at creation time for exactly the
>>>>> same effect and no extra interfaces to maintain.
> I think this is missing the point of using cgroups.  cgroups allow
> policy to be set independently by a privileged system integrator or
> system administrator, in a way that applications themselves don't have
> access to.  LD_PRELOAD would only have access to the interfaces
> available to the app itself and the app could simply undo whatever
> setting the LD_PRELOAD tried to use.
>
> The goal with cgroups isn't to provide a "gpu nice" where you
> self-classify yourself, but rather the ability for the person defining
> how the overall system works to classify and control the various types
> of applications running in a transparent and global way.
>
>>>>>>> Does the implementation mimic some existing cgroups tool or de-facto way
>>>>>>> of doing things in cgroups world?
>>>>>> The ioctl approach I took is similar to syscall approach that the BPF
>>>>>> guys use to attach BPF programs to a cgroup.  I'm not very familiar with
>>>>>> BPF or how it gets used from userspace, so I'm not sure whether the
>>>>>> interface is intended for one specific tool (like ours is), or whether
>>>>>> there's more variety for userspace consumers.
>>>>> Is the proposal to set the memory usage from similar interface, or is
>>>>> that still not implemented?
> GPU memory management is a very different resource model, so I'd expect
> memory to be managed by a real cgroups controller. I.e., the interface
> would be different.  I haven't personally done any work on cgroups-based
> memory management; I agree that would be nice to have, but none of the
> use cases my team is focused on have that as a requirement.
>
>>>>> I'm seeing a very close relation between time-slicing GPU time and
>>>>> allowed GPU buffer allocations, so having two completely different
>>>>> interfaces does just feel very hackish way of implementing this.
> If we were time slicing GPU time and giving out fractional shares of
> time to each client, that would be a very different situation than we
> have today; in that case I agree that a cgroups controller would be the
> way to go.  While there probably are a lot of cases where time slicing
> and fair scheduling would be beneficial (especially stuff like desktop
> or mobile settings), I'm mostly focused on embedded use cases that
> depend on today's unfair, strict priority scheduling.
>
>
> Matt
>
>>>>> Regards, Joonas
>>>>>
>>>>>>
>>>>>> Matt
>>>>>>
>>>>>>> Regards, Joonas
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe cgroups" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>> -- 
>>>>>> Matt Roper
>>>>>> Graphics Software Engineer
>>>>>> IoTG Platform Enabling & Development
>>>>>> Intel Corporation
>>>>>> (916) 356-2795
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe cgroups" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> -- 
>>> Matt Roper
>>> Graphics Software Engineer
>>> IoTG Platform Enabling & Development
>>> Intel Corporation
>>> (916) 356-2795
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx