| Message ID | 20240122172048.11953-2-haitao.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Cgroup support for SGX EPC memory | expand |
On Mon Jan 22, 2024 at 7:20 PM EET, Haitao Huang wrote: > 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. > > 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> > --- > 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 | 60 +++++++++++++++++++++++++++++++++++-- > 2 files changed, 68 insertions(+), 3 deletions(-) > > 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..b8c32791334c 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. > @@ -383,23 +416,37 @@ static struct cftype misc_cg_files[] = { > static struct cgroup_subsys_state * > misc_cg_alloc(struct cgroup_subsys_state *parent_css) > { > + struct misc_cg *parent_cg, *cg; > enum misc_res_type i; > - struct misc_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); > + if (misc_res_ops[i]) { > + ret = misc_res_ops[i]->alloc(cg); > + if (ret) > + goto alloc_err; So I'd consider pattern like this to avoid repetition: if (ret) { __misc_cg_free(cg); return PERR_PTR(ret); } and call __misc_cg_free() also in misc_cg_free(). BR, Jarkko
On Mon, 22 Jan 2024 14:14:01 -0600, Jarkko Sakkinen <jarkko@kernel.org> wrote: > On Mon Jan 22, 2024 at 7:20 PM EET, Haitao Huang wrote: >> 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. >> >> 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> >> --- >> 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 | 60 +++++++++++++++++++++++++++++++++++-- >> 2 files changed, 68 insertions(+), 3 deletions(-) >> >> 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..b8c32791334c 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. >> @@ -383,23 +416,37 @@ static struct cftype misc_cg_files[] = { >> static struct cgroup_subsys_state * >> misc_cg_alloc(struct cgroup_subsys_state *parent_css) >> { >> + struct misc_cg *parent_cg, *cg; >> enum misc_res_type i; >> - struct misc_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); >> + if (misc_res_ops[i]) { >> + ret = misc_res_ops[i]->alloc(cg); >> + if (ret) >> + goto alloc_err; > > So I'd consider pattern like this to avoid repetition: > > if (ret) { > __misc_cg_free(cg); > return PERR_PTR(ret); > } > > and call __misc_cg_free() also in misc_cg_free(). > Will change to do that. Thanks Haitao
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..b8c32791334c 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. @@ -383,23 +416,37 @@ static struct cftype misc_cg_files[] = { static struct cgroup_subsys_state * misc_cg_alloc(struct cgroup_subsys_state *parent_css) { + struct misc_cg *parent_cg, *cg; enum misc_res_type i; - struct misc_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); + if (misc_res_ops[i]) { + ret = misc_res_ops[i]->alloc(cg); + if (ret) + goto alloc_err; + } } return &cg->css; + +alloc_err: + for (i = 0; i < MISC_CG_RES_TYPES; i++) + if (misc_res_ops[i]) + misc_res_ops[i]->free(cg); + kfree(cg); + return ERR_PTR(ret); } /** @@ -410,7 +457,14 @@ 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); + 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); + + kfree(cg); } /* Cgroup controller callbacks */