On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
> From: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
>
> Add SGX EPC memory, MISC_CG_RES_SGX_EPC, to be a valid resource type
> for the misc controller.
>
> Add per resource type private data so that SGX can store additional per
> cgroup data in misc_cg->misc_cg_res[MISC_CG_RES_SGX_EPC].
To be honest I don't quite understand why putting the above two changes in this
patch together with exporting misc_cg_root/parent() below.
Any reason why the above two cannot be done together with patch (" x86/sgx:
Limit process EPC usage with misc cgroup controller"), where these changes are
actually related?
We all already know that a new EPC misc cgroup will be added. There's no need
to actually introduce the new type here only to justify exporting some helper
functions.
>
> Export misc_cg_root() so the SGX driver can initialize and add those
> additional structures to the root misc cgroup as part of initialization
> for EPC cgroup support. This bootstraps the same additional
> initialization for non-root cgroups in the 'alloc()' callback added in the
> previous patch.
>
> The SGX driver, as the EPC memory provider, will have a background
> worker to reclaim EPC pages to make room for new allocations in the same
> cgroup when its usage counter reaches near the limit controlled by the
> cgroup and its ancestors. Therefore it needs to do a walk from the
> current cgroup up to the root. To enable this walk, move parent_misc()
> into misc_cgroup.h and make inline to make this function available to
> SGX, rename it to misc_cg_parent(), and update kernel/cgroup/misc.c to
> use the new name.
Looks too many details in the above two paragraphs. Could we have a more
concise justification for exporting these two functions?
And if it were me, I would put it at a relatively later position (e.g., before
the patch actually implements EPC cgroup) for better review. This also applies
to the first patch.
>
> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> Signed-off-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> ---
> V5:
> - Revised commit message (Jarkko)
>
> V4:
> - Moved this to the second in the series.
> ---
> include/linux/misc_cgroup.h | 29 +++++++++++++++++++++++++++++
> kernel/cgroup/misc.c | 25 ++++++++++++-------------
> 2 files changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/misc_cgroup.h b/include/linux/misc_cgroup.h
> index 96a88822815a..87f29f8597e1 100644
> --- a/include/linux/misc_cgroup.h
> +++ b/include/linux/misc_cgroup.h
> @@ -17,6 +17,10 @@ enum misc_res_type {
> MISC_CG_RES_SEV,
> /* AMD SEV-ES ASIDs resource */
> MISC_CG_RES_SEV_ES,
> +#endif
> +#ifdef CONFIG_CGROUP_SGX_EPC
> + /* SGX EPC memory resource */
> + MISC_CG_RES_SGX_EPC,
> #endif
> MISC_CG_RES_TYPES
> };
> @@ -37,6 +41,7 @@ struct misc_res {
> u64 max;
> atomic64_t usage;
> atomic64_t events;
> + void *priv;
>
> /* per resource callback ops */
> int (*alloc)(struct misc_cg *cg);
> @@ -59,6 +64,7 @@ struct misc_cg {
> struct misc_res res[MISC_CG_RES_TYPES];
> };
>
> +struct misc_cg *misc_cg_root(void);
> 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_try_charge(enum misc_res_type type, struct misc_cg *cg, u64 amount);
> @@ -78,6 +84,20 @@ static inline struct misc_cg *css_misc(struct cgroup_subsys_state *css)
> return css ? container_of(css, struct misc_cg, css) : NULL;
> }
>
> +/**
> + * misc_cg_parent() - Get the parent of the passed misc cgroup.
> + * @cgroup: cgroup whose parent needs to be fetched.
> + *
> + * Context: Any context.
> + * Return:
> + * * struct misc_cg* - Parent of the @cgroup.
> + * * %NULL - If @cgroup is null or the passed cgroup does not have a parent.
> + */
> +static inline struct misc_cg *misc_cg_parent(struct misc_cg *cgroup)
> +{
> + return cgroup ? css_misc(cgroup->css.parent) : NULL;
> +}
> +
> /*
> * get_current_misc_cg() - Find and get the misc cgroup of the current task.
> *
> @@ -102,6 +122,15 @@ static inline void put_misc_cg(struct misc_cg *cg)
> }
>
> #else /* !CONFIG_CGROUP_MISC */
> +static inline struct misc_cg *misc_cg_root(void)
> +{
> + return NULL;
> +}
> +
> +static inline struct misc_cg *misc_cg_parent(struct misc_cg *cg)
> +{
> + return NULL;
> +}
>
> static inline u64 misc_cg_res_total_usage(enum misc_res_type type)
> {
> diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
> index 62c9198dee21..4633b8629e63 100644
> --- a/kernel/cgroup/misc.c
> +++ b/kernel/cgroup/misc.c
> @@ -24,6 +24,10 @@ static const char *const misc_res_name[] = {
> /* AMD SEV-ES ASIDs resource */
> "sev_es",
> #endif
> +#ifdef CONFIG_CGROUP_SGX_EPC
> + /* Intel SGX EPC memory bytes */
> + "sgx_epc",
> +#endif
> };
>
> /* Root misc cgroup */
> @@ -40,18 +44,13 @@ static struct misc_cg root_cg;
> static u64 misc_res_capacity[MISC_CG_RES_TYPES];
>
> /**
> - * parent_misc() - Get the parent of the passed misc cgroup.
> - * @cgroup: cgroup whose parent needs to be fetched.
> - *
> - * Context: Any context.
> - * Return:
> - * * struct misc_cg* - Parent of the @cgroup.
> - * * %NULL - If @cgroup is null or the passed cgroup does not have a parent.
> + * misc_cg_root() - Return the root misc cgroup.
> */
> -static struct misc_cg *parent_misc(struct misc_cg *cgroup)
> +struct misc_cg *misc_cg_root(void)
> {
> - return cgroup ? css_misc(cgroup->css.parent) : NULL;
> + return &root_cg;
> }
> +EXPORT_SYMBOL_GPL(misc_cg_root);
>
> /**
> * valid_type() - Check if @type is valid or not.
> @@ -150,7 +149,7 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg, u64 amount)
> if (!amount)
> return 0;
>
> - for (i = cg; i; i = parent_misc(i)) {
> + for (i = cg; i; i = misc_cg_parent(i)) {
> res = &i->res[type];
>
> new_usage = atomic64_add_return(amount, &res->usage);
> @@ -163,12 +162,12 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg, u64 amount)
> return 0;
>
> err_charge:
> - for (j = i; j; j = parent_misc(j)) {
> + for (j = i; j; j = misc_cg_parent(j)) {
> atomic64_inc(&j->res[type].events);
> cgroup_file_notify(&j->events_file);
> }
>
> - for (j = cg; j != i; j = parent_misc(j))
> + for (j = cg; j != i; j = misc_cg_parent(j))
> misc_cg_cancel_charge(type, j, amount);
> misc_cg_cancel_charge(type, i, amount);
> return ret;
> @@ -190,7 +189,7 @@ void misc_cg_uncharge(enum misc_res_type type, struct misc_cg *cg, u64 amount)
> if (!(amount && valid_type(type) && cg))
> return;
>
> - for (i = cg; i; i = parent_misc(i))
> + for (i = cg; i; i = misc_cg_parent(i))
> misc_cg_cancel_charge(type, i, amount);
> }
> EXPORT_SYMBOL_GPL(misc_cg_uncharge);
