From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Haitao Huang" <haitao.huang@linux.intel.com>,
<dave.hansen@linux.intel.com>, <tj@kernel.org>,
<mkoutny@suse.com>, <linux-kernel@vger.kernel.org>,
<linux-sgx@vger.kernel.org>, <x86@kernel.org>,
<cgroups@vger.kernel.org>, <tglx@linutronix.de>,
<mingo@redhat.com>, <bp@alien8.de>, <hpa@zytor.com>,
<sohil.mehta@intel.com>
Cc: <zhiquan1.li@intel.com>, <kristen@linux.intel.com>,
<seanjc@google.com>, <zhanb@microsoft.com>,
<anakrish@microsoft.com>, <mikko.ylinen@linux.intel.com>,
<yangjie@microsoft.com>
Subject: Re: [PATCH v6 01/12] cgroup/misc: Add per resource callbacks for CSS events
Date: Wed, 10 Jan 2024 21:55:24 +0200 [thread overview]
Message-ID: <CYBAGUN08GGW.1WHQOHZ7XUHL9@suppilovahvero> (raw)
In-Reply-To: <op.2g9gcoz5wjvjmi@hhuan26-mobl.amr.corp.intel.com>
On Tue Jan 9, 2024 at 5:37 AM EET, Haitao Huang wrote:
> On Wed, 15 Nov 2023 14:25:59 -0600, Jarkko Sakkinen <jarkko@kernel.org>
> wrote:
>
> > On Mon Oct 30, 2023 at 8:20 PM EET, Haitao Huang wrote:
> >> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> >>
> >> 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@linux.intel.com>
> >> Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
> >> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> >> ---
> >> 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?
> >
>
> With Michal's suggestion to make ops per-resource-type, I'll rename this
> misc_res_ops (I was following vm_operations_struct as example)
>
> >> +
> >> /**
> >> * 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.
> >
>
> Ok
>
> >> + 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
>
> Will combine these with the use of a static operations array suggested by
> Michal.
OK, great, thanks!
BR, Jarkko
next prev parent reply other threads:[~2024-01-10 19:55 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-30 18:20 [PATCH v6 00/12] Add Cgroup support for SGX EPC memory Haitao Huang
2023-10-30 18:20 ` [PATCH v6 01/12] cgroup/misc: Add per resource callbacks for CSS events Haitao Huang
2023-11-15 20:25 ` Jarkko Sakkinen
2024-01-09 3:37 ` Haitao Huang
2024-01-10 19:55 ` Jarkko Sakkinen [this message]
2024-01-05 9:45 ` Michal Koutný
2024-01-06 1:42 ` Haitao Huang
2023-10-30 18:20 ` [PATCH v6 02/12] cgroup/misc: Export APIs for SGX driver Haitao Huang
2023-10-30 18:20 ` [PATCH v6 03/12] cgroup/misc: Add SGX EPC resource type Haitao Huang
2023-10-30 18:20 ` [PATCH v6 04/12] x86/sgx: Implement basic EPC misc cgroup functionality Haitao Huang
2023-11-06 12:09 ` Huang, Kai
2023-11-06 18:59 ` Haitao Huang
2023-11-06 22:18 ` Huang, Kai
2023-11-07 1:16 ` Haitao Huang
2023-11-07 2:08 ` Haitao Huang
2023-11-07 19:07 ` Huang, Kai
2023-11-20 3:16 ` Huang, Kai
2023-11-26 16:01 ` Haitao Huang
2023-11-26 16:32 ` Haitao Huang
2023-11-06 22:23 ` Huang, Kai
2023-11-15 20:48 ` Jarkko Sakkinen
2023-10-30 18:20 ` [PATCH v6 05/12] x86/sgx: Add sgx_epc_lru_list to encapsulate LRU list Haitao Huang
2023-10-30 18:20 ` [PATCH v6 06/12] x86/sgx: Use sgx_epc_lru_list for existing active page list Haitao Huang
2023-10-30 18:20 ` [PATCH v6 07/12] x86/sgx: Introduce EPC page states Haitao Huang
2023-11-15 20:53 ` Jarkko Sakkinen
2024-01-05 17:57 ` Dave Hansen
2024-01-06 1:45 ` Haitao Huang
2023-10-30 18:20 ` [PATCH v6 08/12] x86/sgx: Use a list to track to-be-reclaimed pages Haitao Huang
2023-11-15 20:59 ` Jarkko Sakkinen
2023-10-30 18:20 ` [PATCH v6 09/12] x86/sgx: Restructure top-level EPC reclaim function Haitao Huang
2023-11-20 3:45 ` Huang, Kai
2023-11-26 16:27 ` Haitao Huang
2023-11-27 9:57 ` Huang, Kai
2023-12-12 4:04 ` Haitao Huang
2023-12-13 11:17 ` Huang, Kai
2023-12-15 19:49 ` Haitao Huang
2023-12-18 1:44 ` Huang, Kai
2023-12-18 17:32 ` Mikko Ylinen
2023-12-18 21:24 ` Haitao Huang
2024-01-03 16:37 ` Dave Hansen
2024-01-04 19:11 ` Haitao Huang
2024-01-04 19:19 ` Jarkko Sakkinen
2024-01-04 19:27 ` Dave Hansen
2024-01-04 21:01 ` Haitao Huang
2024-01-05 14:43 ` Mikko Ylinen
2024-01-04 12:38 ` Michal Koutný
2024-01-04 19:20 ` Haitao Huang
2024-01-12 17:07 ` Haitao Huang
2024-01-13 21:04 ` Jarkko Sakkinen
2024-01-13 21:08 ` Jarkko Sakkinen
2023-10-30 18:20 ` [PATCH v6 10/12] x86/sgx: Implement EPC reclamation for cgroup Haitao Huang
2023-11-06 15:58 ` [PATCH] x86/sgx: Charge proper mem_cgroup for usage due to EPC reclamation by cgroups Haitao Huang
2023-11-06 16:10 ` [PATCH v6 10/12] x86/sgx: Implement EPC reclamation for cgroup Haitao Huang
2023-10-30 18:20 ` [PATCH v6 11/12] Docs/x86/sgx: Add description for cgroup support Haitao Huang
2023-10-30 18:20 ` [PATCH v6 12/12] selftests/sgx: Add scripts for EPC cgroup testing Haitao Huang
2023-11-15 21:00 ` Jarkko Sakkinen
2023-11-15 21:22 ` Haitao Huang
2023-11-06 3:26 ` [PATCH v6 00/12] Add Cgroup support for SGX EPC memory Jarkko Sakkinen
2023-11-06 15:48 ` Haitao Huang
2023-11-08 1:00 ` Haitao Huang
2024-01-05 18:29 ` Dave Hansen
2024-01-05 20:13 ` Haitao Huang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CYBAGUN08GGW.1WHQOHZ7XUHL9@suppilovahvero \
--to=jarkko@kernel.org \
--cc=anakrish@microsoft.com \
--cc=bp@alien8.de \
--cc=cgroups@vger.kernel.org \
--cc=dave.hansen@linux.intel.com \
--cc=haitao.huang@linux.intel.com \
--cc=hpa@zytor.com \
--cc=kristen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=mikko.ylinen@linux.intel.com \
--cc=mingo@redhat.com \
--cc=mkoutny@suse.com \
--cc=seanjc@google.com \
--cc=sohil.mehta@intel.com \
--cc=tglx@linutronix.de \
--cc=tj@kernel.org \
--cc=x86@kernel.org \
--cc=yangjie@microsoft.com \
--cc=zhanb@microsoft.com \
--cc=zhiquan1.li@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).