On Fri, 2023-09-22 at 20:06 -0700, 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, or in event of user writing the max value to
> the misc.max file to set the usage limit of a specific resource
> [admin-guide/cgroup-v2.rst, 5-9. Misc].
>
> 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:
> - On css_alloc, allocate and initialize necessary structures for EPC
> reclaiming, e.g., LRU list, work queue, etc.
> - On css_free, cleanup and free those structures created in alloc.
> - On max_write, trigger EPC reclaiming if the new limit is at or below
> current usage.
Nit:
Wondering why we should trigger EPC reclaiming if the new limit is *at* current
usage?
I actually don't quite care about why here, but writing these details in the
changelog may bring unnecessary confusion. I guess you can just remove all the
details about what SGX driver needs to do on these callbacks.
>
> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> Signed-off-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> ---
> V5:
> - Remove prefixes from the callback names (tj)
> - Update commit message (Jarkko)
>
> V4:
> - Moved this to the front of the series.
> - Applies on cgroup/for-6.6 with the overflow fix for misc.
>
> V3:
> - Removed the released() callback
> ---
> include/linux/misc_cgroup.h | 5 +++++
> kernel/cgroup/misc.c | 32 +++++++++++++++++++++++++++++---
> 2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/misc_cgroup.h b/include/linux/misc_cgroup.h
> index e799b1f8d05b..96a88822815a 100644
> --- a/include/linux/misc_cgroup.h
> +++ b/include/linux/misc_cgroup.h
> @@ -37,6 +37,11 @@ struct misc_res {
> u64 max;
> atomic64_t usage;
> atomic64_t events;
> +
> + /* per resource callback ops */
Nit:
This comment isn't quite useful IMHO. And it seems you should just expand the
existing comment for the 'struct misc_res', which already covers the existing
members.
Or as Jarkko suggested, maybe you can introduce another structure 'misc_res_ops'
and comment more details for all these callbacks just like 'struct misc_res'.
Anyway it's cgroup maintainer's call.
> + int (*alloc)(struct misc_cg *cg);
> + void (*free)(struct misc_cg *cg);
> + void (*max_write)(struct misc_cg *cg);
> };
>
> /**
> diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
> index 79a3717a5803..62c9198dee21 100644
> --- a/kernel/cgroup/misc.c
> +++ b/kernel/cgroup/misc.c
> @@ -276,10 +276,13 @@ static ssize_t misc_cg_max_write(struct kernfs_open_file *of, char *buf,
>
> cg = css_misc(of_css(of));
>
> - if (READ_ONCE(misc_res_capacity[type]))
> + if (READ_ONCE(misc_res_capacity[type])) {
> WRITE_ONCE(cg->res[type].max, max);
> - else
> + if (cg->res[type].max_write)
> + cg->res[type].max_write(cg);
> + } else {
> ret = -EINVAL;
> + }
>
> return ret ? ret : nbytes;
> }
> @@ -383,23 +386,39 @@ 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;
Nit:
The below variable '*cg' can be moved here together with 'parent_cg'.
> enum misc_res_type i;
> struct misc_cg *cg;
> + int ret;
>
> if (!parent_css) {
> cg = &root_cg;
> + parent_cg = &root_cg;
Nit:
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 (parent_cg->res[i].alloc) {
> + ret = parent_cg->res[i].alloc(cg);
> + if (ret)
> + goto alloc_err;
> + }
> }
>
> return &cg->css;
> +
> +alloc_err:
> + for (i = 0; i < MISC_CG_RES_TYPES; i++)
> + if (parent_cg->res[i].free)
> + cg->res[i].free(cg);
> + kfree(cg);
> + return ERR_PTR(ret);
> }
>
> /**
> @@ -410,7 +429,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 (cg->res[i].free)
> + cg->res[i].free(cg);
> +
> + kfree(cg);
> }
>
> /* Cgroup controller callbacks */
