diff mbox series

[v16,02/16] cgroup/misc: Add per resource callbacks for CSS events

Message ID 20240821015404.6038-3-haitao.huang@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Add Cgroup support for SGX EPC memory | expand

Commit Message

Haitao Huang Aug. 21, 2024, 1:53 a.m. UTC 
From: Kristen Carlson Accardi <kristen@linux.intel.com>

The misc cgroup controller (subsystem) currently does not perform
resource type specific action for Cgroups Subsystem State (CSS) events:
the 'css_alloc' event when a cgroup is created and the 'css_free' event
when a cgroup is destroyed.

Define callbacks for those events and allow resource providers to
register the callbacks per resource type as needed. This will be
utilized later by the EPC misc cgroup support implemented in the SGX
driver.

The SGX callbacks will need to access the 'struct misc_cg'.  Pass
'struct misc_cg' to the callbacks but not the 'struct misc_res' because
the latter doesn't have a pointer pointing back to 'struct misc_cg'.

Link: https://lore.kernel.org/lkml/op.2kdw36otwjvjmi@hhuan26-mobl.amr.corp.intel.com/

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Reviewed-by: Tejun Heo <tj@kernel.org>
Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
---
V16:
- Don't call the ops if capacity is zero. (Kai)
- Simplify the last paragraph of the commit message. (Kai)

V15:
- Style fixes (Jarkko, Kai)
- _misc_cg_res_free() takes the last index. Only call ->free() for those res types with ->alloc() successfully called. (Ridong)

V12:
- Add comments in commit to clarify reason to pass in misc_cg, not
misc_res. (Kai)
- Remove unlikely (Kai)

V8:
- Abstract out _misc_cg_res_free() and _misc_cg_res_alloc() (Jarkko)

V7:
- Make ops one per resource type and store them in array (Michal)
- Rename the ops struct to misc_res_ops, and enforce the constraints of required callback
functions (Jarkko)
- Moved addition of priv field to patch 4 where it was used first. (Jarkko)

V6:
- Create ops struct for per resource callbacks (Jarkko)
- Drop max_write callback (Dave, Michal)
- Style fixes (Kai)
---
 include/linux/misc_cgroup.h | 11 +++++
 kernel/cgroup/misc.c        | 88 ++++++++++++++++++++++++++++++++++---
 2 files changed, 93 insertions(+), 6 deletions(-)

Comments

Huang, Kai Aug. 27, 2024, 10:45 a.m. UTC | #1 
On Tue, 2024-08-20 at 18:53 -0700, Haitao Huang wrote:
> +/**
> + * misc_cg_set_ops() - register resource specific operations.
> + * @type: Type of the misc res.
> + * @ops: Operations for the given type.
> + *
> + * The callbacks in @ops will not be invoked if the capacity of @type is 0.
> + *
> + * Context: Any context.
> + * Return:
> + * * %0 - Successfully registered the operations.
> + * * %-EINVAL - If @type is invalid, or the operations missing any required callbacks.
> + */
> +int misc_cg_set_ops(enum misc_res_type type, const struct misc_res_ops *ops)
> +{
> +	if (!valid_type(type))
> +		return -EINVAL;
> +
> +	if (!ops)
> +		return -EINVAL;
> +
> +	if (!ops->alloc) {
> +		pr_err("%s: alloc missing\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!ops->free) {
> +		pr_err("%s: free missing\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	misc_res_ops[type] = ops;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(misc_cg_set_ops);

As mentioned in another reply, the export isn't mandatory for SGX cgroup
support due to SGX driver cannot be a module.

But I understand the intention is to treat this in a similar way to
misc_cg_set_capacity() etc which are currently exported.

I have no hard opinion whether to export this one.  But if we export this, I
don't quite like the fact that it doesn't accept the NULL ops to allow the
caller to unwind.

But I'll leave to cgroup maintainers.

> +
>  /**
>   * misc_cg_cancel_charge() - Cancel the charge from the misc cgroup.
>   * @type: Misc res type in misc cg to cancel the charge from.
> @@ -439,6 +477,36 @@ static struct cftype misc_cg_files[] = {
>  	{}
>  };
>  
> +static inline void _misc_cg_res_free(struct misc_cg *cg, enum misc_res_type last)
> +{
> +	enum misc_res_type i;
> +
> +	for (i = 0; i <= last; i++)
> +		if (misc_res_ops[i] && READ_ONCE(misc_res_capacity[i]))
> +			misc_res_ops[i]->free(cg);
> +}
> +
> +static inline int _misc_cg_res_alloc(struct misc_cg *cg)
> +{
> +	enum misc_res_type i;
> +	int ret;
> +
> +	for (i = 0; i < MISC_CG_RES_TYPES; i++) {
> +		WRITE_ONCE(cg->res[i].max, MAX_NUM);
> +		atomic64_set(&cg->res[i].usage, 0);
> +
> +		if (misc_res_ops[i] && READ_ONCE(misc_res_capacity[i])) {
> +			ret = misc_res_ops[i]->alloc(cg);
> +			if (ret) {
> +				_misc_cg_res_free(cg, i);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +

I don't fully understand why kernel uses READ_ONCE()/WRITE_ONCE() to access
misc_res_capacity[i] and others like 'cg->res[i].max', but it looks weird they
are not used when accessing misc_res_ops[i]?
Huang, Kai Aug. 29, 2024, 9:25 a.m. UTC | #2 
On Tue, 2024-08-20 at 18:53 -0700, Haitao Huang wrote:
>  /**
>   * misc_cg_alloc() - Allocate misc cgroup.
>   * @parent_css: Parent cgroup.
> @@ -451,20 +519,25 @@ static struct cftype misc_cg_files[] = {
>  static struct cgroup_subsys_state *
>  misc_cg_alloc(struct cgroup_subsys_state *parent_css)
>  {
> -	enum misc_res_type i;
> -	struct misc_cg *cg;
> +	struct misc_cg *parent_cg, *cg;
> +	int ret;
>  
>  	if (!parent_css) {
> +		parent_cg = &root_cg;
>  		cg = &root_cg;
>  	} else {
>  		cg = kzalloc(sizeof(*cg), GFP_KERNEL);
>  		if (!cg)
>  			return ERR_PTR(-ENOMEM);
> +		parent_cg = css_misc(parent_css);
>  	}
>  
> -	for (i = 0; i < MISC_CG_RES_TYPES; i++) {
> -		WRITE_ONCE(cg->res[i].max, MAX_NUM);
> -		atomic64_set(&cg->res[i].usage, 0);
> +	ret = _misc_cg_res_alloc(cg);
> +	if (ret) {
> +		if (likely(parent_css))
> +			kfree(cg);
> +
> +		return ERR_PTR(ret);
>  	}
>  
>  	return &cg->css;

What's the purpose of @parent_cg?

# make kernel/cgroup/ W=1
...
kernel/cgroup/misc.c: In function ‘misc_cg_alloc’:
kernel/cgroup/misc.c:522:25: warning: variable ‘parent_cg’ set but not used [-
Wunused-but-set-variable]
  522 |         struct misc_cg *parent_cg, *cg;
      |                         ^~~~~~~~~
diff mbox series

Patch

diff --git a/include/linux/misc_cgroup.h b/include/linux/misc_cgroup.h
index 49eef10c8e59..e5159770a68e 100644
--- a/include/linux/misc_cgroup.h
+++ b/include/linux/misc_cgroup.h
@@ -28,6 +28,16 @@  struct misc_cg;
 
 #include <linux/cgroup.h>
 
+/**
+ * struct misc_res_ops: per resource type callback ops.
+ * @alloc: invoked for resource specific initialization when cgroup is allocated.
+ * @free: invoked for resource specific cleanup when cgroup is deallocated.
+ */
+struct misc_res_ops {
+	int (*alloc)(struct misc_cg *cg);
+	void (*free)(struct misc_cg *cg);
+};
+
 /**
  * struct misc_res: Per cgroup per misc type resource
  * @max: Maximum limit on the resource.
@@ -62,6 +72,7 @@  struct misc_cg {
 
 u64 misc_cg_res_total_usage(enum misc_res_type type);
 int misc_cg_set_capacity(enum misc_res_type type, u64 capacity);
+int misc_cg_set_ops(enum misc_res_type type, const struct misc_res_ops *ops);
 int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg, u64 amount);
 void misc_cg_uncharge(enum misc_res_type type, struct misc_cg *cg, u64 amount);
 
diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
index 0e26068995a6..f3ce896d2c21 100644
--- a/kernel/cgroup/misc.c
+++ b/kernel/cgroup/misc.c
@@ -39,6 +39,9 @@  static struct misc_cg root_cg;
  */
 static u64 misc_res_capacity[MISC_CG_RES_TYPES];
 
+/* Resource type specific operations */
+static const struct misc_res_ops *misc_res_ops[MISC_CG_RES_TYPES];
+
 /**
  * parent_misc() - Get the parent of the passed misc cgroup.
  * @cgroup: cgroup whose parent needs to be fetched.
@@ -105,6 +108,41 @@  int misc_cg_set_capacity(enum misc_res_type type, u64 capacity)
 }
 EXPORT_SYMBOL_GPL(misc_cg_set_capacity);
 
+/**
+ * misc_cg_set_ops() - register resource specific operations.
+ * @type: Type of the misc res.
+ * @ops: Operations for the given type.
+ *
+ * The callbacks in @ops will not be invoked if the capacity of @type is 0.
+ *
+ * Context: Any context.
+ * Return:
+ * * %0 - Successfully registered the operations.
+ * * %-EINVAL - If @type is invalid, or the operations missing any required callbacks.
+ */
+int misc_cg_set_ops(enum misc_res_type type, const struct misc_res_ops *ops)
+{
+	if (!valid_type(type))
+		return -EINVAL;
+
+	if (!ops)
+		return -EINVAL;
+
+	if (!ops->alloc) {
+		pr_err("%s: alloc missing\n", __func__);
+		return -EINVAL;
+	}
+
+	if (!ops->free) {
+		pr_err("%s: free missing\n", __func__);
+		return -EINVAL;
+	}
+
+	misc_res_ops[type] = ops;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(misc_cg_set_ops);
+
 /**
  * misc_cg_cancel_charge() - Cancel the charge from the misc cgroup.
  * @type: Misc res type in misc cg to cancel the charge from.
@@ -439,6 +477,36 @@  static struct cftype misc_cg_files[] = {
 	{}
 };
 
+static inline void _misc_cg_res_free(struct misc_cg *cg, enum misc_res_type last)
+{
+	enum misc_res_type i;
+
+	for (i = 0; i <= last; i++)
+		if (misc_res_ops[i] && READ_ONCE(misc_res_capacity[i]))
+			misc_res_ops[i]->free(cg);
+}
+
+static inline int _misc_cg_res_alloc(struct misc_cg *cg)
+{
+	enum misc_res_type i;
+	int ret;
+
+	for (i = 0; i < MISC_CG_RES_TYPES; i++) {
+		WRITE_ONCE(cg->res[i].max, MAX_NUM);
+		atomic64_set(&cg->res[i].usage, 0);
+
+		if (misc_res_ops[i] && READ_ONCE(misc_res_capacity[i])) {
+			ret = misc_res_ops[i]->alloc(cg);
+			if (ret) {
+				_misc_cg_res_free(cg, i);
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
 /**
  * misc_cg_alloc() - Allocate misc cgroup.
  * @parent_css: Parent cgroup.
@@ -451,20 +519,25 @@  static struct cftype misc_cg_files[] = {
 static struct cgroup_subsys_state *
 misc_cg_alloc(struct cgroup_subsys_state *parent_css)
 {
-	enum misc_res_type i;
-	struct misc_cg *cg;
+	struct misc_cg *parent_cg, *cg;
+	int ret;
 
 	if (!parent_css) {
+		parent_cg = &root_cg;
 		cg = &root_cg;
 	} else {
 		cg = kzalloc(sizeof(*cg), GFP_KERNEL);
 		if (!cg)
 			return ERR_PTR(-ENOMEM);
+		parent_cg = css_misc(parent_css);
 	}
 
-	for (i = 0; i < MISC_CG_RES_TYPES; i++) {
-		WRITE_ONCE(cg->res[i].max, MAX_NUM);
-		atomic64_set(&cg->res[i].usage, 0);
+	ret = _misc_cg_res_alloc(cg);
+	if (ret) {
+		if (likely(parent_css))
+			kfree(cg);
+
+		return ERR_PTR(ret);
 	}
 
 	return &cg->css;
@@ -478,7 +551,10 @@  misc_cg_alloc(struct cgroup_subsys_state *parent_css)
  */
 static void misc_cg_free(struct cgroup_subsys_state *css)
 {
-	kfree(css_misc(css));
+	struct misc_cg *cg = css_misc(css);
+
+	_misc_cg_res_free(cg, MISC_CG_RES_TYPES - 1);
+	kfree(cg);
 }
 
 /* Cgroup controller callbacks */