diff mbox series

[v14,02/14] cgroup/misc: Add per resource callbacks for CSS events

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

Commit Message

Haitao Huang May 31, 2024, 10:26 p.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.

This design passes a struct misc_cg into the callbacks. An alternative
to pass only a struct misc_res was considered for encapsulating how
misc_cg stores its misc_res array. However, the SGX driver would still
need to access the misc_res array in the statically defined misc root_cg
during initialization to set resource specific fields. Therefore, some
extra getter/setter APIs to abstract such access to the misc_res array
within the misc_cg struct would be needed. That seems an overkill at the
moment and is deferred to later improvement when there are more users of
these callbacks.

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>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
---
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        | 82 +++++++++++++++++++++++++++++++++----
 2 files changed, 86 insertions(+), 7 deletions(-)

Comments

Haitao Huang June 6, 2024, 2:51 p.m. UTC | #1 
On Thu, 06 Jun 2024 08:37:31 -0500, chenridong <chenridong@huawei.com>  
wrote:

>
>    
> If _misc_cg_res_alloc fails, maybe some types do not call ->alloc(), but  
> all types ->free() callback >will be called, is that ok?
>
Not sure I understand. Are you suggesting we ignore failures from  
->alloc() callback in _misc_cg_res_alloc() as it is per-resource, and have  
->free() callback and resource provider of the failing type to handle the  
failure internally?

IIUC, this failure only happens when a specific subcgroup is created  
(memory running out for allocation) so failing that subcgroup as a whole  
seems fine to me. Note the root node is static and no pre-resource  
callbacks invoked by misc. And resource provider handles its own  
allocations for root. In SGX case we too declare a static object for  
corresponding root sgx_cgroup struct.

Note also misc cgroup (except for setting capacity[res] = 0 at root) is  
all or nothing so no mechanism to tell user "this resource does not work  
but others are fine in this particular cgroup."

Thanks
Haitao
Chen Ridong June 7, 2024, 1:53 a.m. UTC | #2 
I think it is better when _misc_cg_res_alloc fails, it just calls 
_misc_cg_res_free(cg, index)(add index parameter, it means ending of 
iterator), so it can avoid calling ->free() that do not call ->alloc().

And in misc_cg_free, just call _misc_cg_res_free(cg, MISC_CG_RES_TYPES)  
to free all.


Thanks

Ridong


On 2024/6/6 22:51, Haitao Huang wrote:
> On Thu, 06 Jun 2024 08:37:31 -0500, chenridong <chenridong@huawei.com> 
> wrote:
>
>>
>>   If _misc_cg_res_alloc fails, maybe some types do not call 
>> ->alloc(), but all types ->free() callback >will be called, is that ok?
>>
> Not sure I understand. Are you suggesting we ignore failures from 
> ->alloc() callback in _misc_cg_res_alloc() as it is per-resource, and 
> have ->free() callback and resource provider of the failing type to 
> handle the failure internally?
>
> IIUC, this failure only happens when a specific subcgroup is created 
> (memory running out for allocation) so failing that subcgroup as a 
> whole seems fine to me. Note the root node is static and no 
> pre-resource callbacks invoked by misc. And resource provider handles 
> its own allocations for root. In SGX case we too declare a static 
> object for corresponding root sgx_cgroup struct.
>
> Note also misc cgroup (except for setting capacity[res] = 0 at root) 
> is all or nothing so no mechanism to tell user "this resource does not 
> work but others are fine in this particular cgroup."
>
> Thanks
> Haitao
>
Haitao Huang June 7, 2024, 2:23 p.m. UTC | #3 
On Thu, 06 Jun 2024 20:53:11 -0500, chenridong <chenridong@huawei.com>  
wrote:

> I think it is better when _misc_cg_res_alloc fails, it just calls  
> _misc_cg_res_free(cg, index)(add index parameter, it means ending of  
> iterator), so it can avoid calling ->free() that do not call ->alloc().
>
> And in misc_cg_free, just call _misc_cg_res_free(cg, MISC_CG_RES_TYPES)   
> to free all.
>
That makes sense now, Will do that.
(BTW you need comment inline :-)
Thanks
Haitao

>
> On 2024/6/6 22:51, Haitao Huang wrote:
>> On Thu, 06 Jun 2024 08:37:31 -0500, chenridong <chenridong@huawei.com>  
>> wrote:
>>
>>>
>>>   If _misc_cg_res_alloc fails, maybe some types do not call ->alloc(),  
>>> but all types ->free() callback >will be called, is that ok?
>>>
>> Not sure I understand. Are you suggesting we ignore failures from  
>> ->alloc() callback in _misc_cg_res_alloc() as it is per-resource, and  
>> have ->free() callback and resource provider of the failing type to  
>> handle the failure internally?
>>
>> IIUC, this failure only happens when a specific subcgroup is created  
>> (memory running out for allocation) so failing that subcgroup as a  
>> whole seems fine to me. Note the root node is static and no  
>> pre-resource callbacks invoked by misc. And resource provider handles  
>> its own allocations for root. In SGX case we too declare a static  
>> object for corresponding root sgx_cgroup struct.
>>
>> Note also misc cgroup (except for setting capacity[res] = 0 at root) is  
>> all or nothing so no mechanism to tell user "this resource does not  
>> work but others are fine in this particular cgroup."
>>
>> Thanks
>> Haitao
>>
>
diff mbox series

Patch

diff --git a/include/linux/misc_cgroup.h b/include/linux/misc_cgroup.h
index e799b1f8d05b..0806d4436208 100644
--- a/include/linux/misc_cgroup.h
+++ b/include/linux/misc_cgroup.h
@@ -27,6 +27,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.
@@ -56,6 +66,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 79a3717a5803..4a602d68cf7d 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,36 @@  int misc_cg_set_capacity(enum misc_res_type type, u64 capacity)
 }
 EXPORT_SYMBOL_GPL(misc_cg_set_capacity);
 
+/**
+ * misc_cg_set_ops() - set resource specific operations.
+ * @type: Type of the misc res.
+ * @ops: Operations for the given type.
+ *
+ * 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->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.
@@ -371,6 +404,33 @@  static struct cftype misc_cg_files[] = {
 	{}
 };
 
+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]) {
+			ret = misc_res_ops[i]->alloc(cg);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static inline void _misc_cg_res_free(struct misc_cg *cg)
+{
+	enum misc_res_type i;
+
+	for (i = 0; i < MISC_CG_RES_TYPES; i++)
+		if (misc_res_ops[i])
+			misc_res_ops[i]->free(cg);
+}
+
 /**
  * misc_cg_alloc() - Allocate misc cgroup.
  * @parent_css: Parent cgroup.
@@ -383,20 +443,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) {
-		cg = &root_cg;
+		parent_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) {
+		_misc_cg_res_free(cg);
+		if (likely(parent_css))
+			kfree(cg);
+
+		return ERR_PTR(ret);
 	}
 
 	return &cg->css;
@@ -410,7 +475,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);
+	kfree(cg);
 }
 
 /* Cgroup controller callbacks */