Re: [PATCH v6 01/12] cgroup/misc: Add per resource callbacks for CSS events

[Jarkko Sakkinen]

On Mon Oct 30, 2023 at 8: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.
>
> Also add per resource type private data for those callbacks to store and
> access resource specific data.
>
> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> Co-developed-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> Signed-off-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> ---
> V6:
> - Create ops struct for per resource callbacks (Jarkko)
> - Drop max_write callback (Dave, Michal)
> - Style fixes (Kai)
> ---
>  include/linux/misc_cgroup.h | 14 ++++++++++++++
>  kernel/cgroup/misc.c        | 27 ++++++++++++++++++++++++---
>  2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/misc_cgroup.h b/include/linux/misc_cgroup.h
> index e799b1f8d05b..5dc509c27c3d 100644
> --- a/include/linux/misc_cgroup.h
> +++ b/include/linux/misc_cgroup.h
> @@ -27,16 +27,30 @@ struct misc_cg;
>  
>  #include <linux/cgroup.h>
>  
> +/**
> + * struct misc_operations_struct: per resource callback ops.
> + * @alloc: invoked for resource specific initialization when cgroup is allocated.
> + * @free: invoked for resource specific cleanup when cgroup is deallocated.
> + */
> +struct misc_operations_struct {
> +	int (*alloc)(struct misc_cg *cg);
> +	void (*free)(struct misc_cg *cg);
> +};

Maybe just misc_operations, or even misc_ops?

> +
>  /**
>   * struct misc_res: Per cgroup per misc type resource
>   * @max: Maximum limit on the resource.
>   * @usage: Current usage of the resource.
>   * @events: Number of times, the resource limit exceeded.
> + * @priv: resource specific data.
> + * @misc_ops: resource specific operations.
>   */
>  struct misc_res {
>  	u64 max;
>  	atomic64_t usage;
>  	atomic64_t events;
> +	void *priv;

priv is the wrong patch, it just confuses the overall picture heere.
please move it to 04/12. Let's deal with the callbacks here.

> +	const struct misc_operations_struct *misc_ops;
>  };

misc_ops would be at least consistent with this, as misc_res also has an
acronym.

>  
>  /**
> diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
> index 79a3717a5803..d971ede44ebf 100644
> --- a/kernel/cgroup/misc.c
> +++ b/kernel/cgroup/misc.c
> @@ -383,23 +383,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 (parent_cg->res[i].misc_ops && parent_cg->res[i].misc_ops->alloc) {
> +			ret = parent_cg->res[i].misc_ops->alloc(cg);
> +			if (ret)
> +				goto alloc_err;

The patch set only has a use case for both operations defined - any
partial combinations should never be allowed.

To enforce this invariant you could create a set of operations (written
out of top of my head):

static int misc_res_init(struct misc_res *res, struct misc_ops *ops)
{
	if (!misc_ops->alloc) {
		pr_err("%s: alloc missing\n", __func__);
		return -EINVAL;
	}

	if (!misc_ops->free) {
		pr_err("%s: free missing\n", __func__);
		return -EINVAL;
	}

	res->misc_ops = misc_ops;
	return 0;
}

static inline int misc_res_alloc(struct misc_cg *cg, struct misc_res *res)
{
	int ret;

	if (!res->misc_ops)
		return 0;
	
	return res->misc_ops->alloc(cg);
}

static inline void misc_res_free(struct misc_cg *cg, struct misc_res *res)
{
	int ret;

	if (!res->misc_ops)
		return 0;
	
	return res->misc_ops->alloc(cg);
}

Now if anything has misc_ops, it will also always have *both* callback,
and nothing half-baked gets in.

The above loops would be then:

	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_res_alloc(&parent_cg->res[i]);
		if (ret)
			goto alloc_err;

Cleaner and better guards for state consistency. In 04/12 you need to
then call misc_res_init() instead of direct assignment.

BR, Jarkko