On Mon Jan 22, 2024 at 7:20 PM EET, Haitao Huang wrote:
> From: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Co-developed-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> Signed-off-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> ---
> 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
