| Message ID | 20230923030657.16148-3-haitao.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Cgroup support for SGX EPC memory | expand |
On Fri, Sep 22, 2023 at 08:06:41PM -0700, Haitao Huang wrote: > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > 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]. > > 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. > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks.
On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote: > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > 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@linux.intel.com> > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> > --- > 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);
On Wed, 27 Sep 2023 22:59:12 -0500, Huang, Kai <kai.huang@intel.com> wrote: > On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote: >> From: Kristen Carlson Accardi <kristen@linux.intel.com> >> >> 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. > I think previous authors intended to separate all prerequisite misc changes from SGX changes. I can combine them if maintainers are fine with it. >> >> 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? > This was added to address Jarkko's question, "why does SGX driver need to do iterative walks?" See: https://lore.kernel.org/all/CVHOU5G1SCUT.RCBVZ3W8G2NJ@suppilovahvero/ > 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. > I was told to move all prerequisites to the front or separate out. https://lore.kernel.org/linux-sgx/CU4H43P3H35X.1BCA3CE4D1250@seitikki/
On Tue, 2023-10-03 at 02:00 -0500, Haitao Huang wrote: > On Wed, 27 Sep 2023 22:59:12 -0500, Huang, Kai <kai.huang@intel.com> wrote: > > > On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote: > > > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > > 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. > > > > I think previous authors intended to separate all prerequisite misc > changes from SGX changes. > I can combine them if maintainers are fine with it. That's fine. But IMHO for this particular one I think you are mixing things together: Adding SGX EPC resource type and exporting APIs don't have dependency on the code. It will be easier to review if you separate this two parts out. For instance, at least it's not super clear whether adding a 'priv' is a right move here w/o looking at the later patches. Also if you take a look at: 7aef27f0b2a8 ("svm/sev: Register SEV and SEV-ES ASIDs to the misc controller") Adding the resource type is added together with the implementation. So I have no problem if you want to split out "adding SGX EPC resource type" out as a separate patch, but this patch looks should be split. > > > > > > > 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? > > > > This was added to address Jarkko's question, "why does SGX driver need to > do iterative walks?" > See: https://lore.kernel.org/all/CVHOU5G1SCUT.RCBVZ3W8G2NJ@suppilovahvero/ Agree with Jarkko we need a justification (that what I said above too). What I am saying is you can make it more concise. I can try to do if you want me to. > > > 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. > > > > I was told to move all prerequisites to the front or separate out. > > https://lore.kernel.org/linux-sgx/CU4H43P3H35X.1BCA3CE4D1250@seitikki/ > > I don't see any conflict. Please see the first reply.
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);