From: "Haitao Huang" <haitao.huang@linux.intel.com>
To: "Jarkko Sakkinen" <jarkko@kernel.org>,
dave.hansen@linux.intel.com, tj@kernel.org,
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 v5 01/18] cgroup/misc: Add per resource callbacks for CSS events
Date: Wed, 04 Oct 2023 10:45:08 -0500 [thread overview]
Message-ID: <op.2cardibgwjvjmi@hhuan26-mobl.amr.corp.intel.com> (raw)
In-Reply-To: <CVYBO2F1D1NC.1N7LNWPTDRG04@seitikki>
Hi Jarkko
On Mon, 02 Oct 2023 17:55:14 -0500, Jarkko Sakkinen <jarkko@kernel.org>
wrote:
...
>> > >> I noticed this one later:
>> > >>
>> > >> It would better to create a separate ops struct and declare the
>> instance
>> > >> as const at minimum.
>> > >>
>> > >> Then there is no need for dynamic assigment of ops and all that is
>> in
>> > >> rodata. This is improves both security and also allows static
>> analysis
>> > >> bit better.
>> > >>
>> > >> Now you have to dynamically trace the struct instance, e.g. in
>> case of
>> > >> a bug. If this one done, it would be already in the vmlinux.
>> > >I.e. then in the driver you can have static const struct declaration
>> > > with *all* pointers pre-assigned.
>> > >
>> > > Not sure if cgroups follows this or not but it is *objectively*
>> > > better. Previous work is not always best possible work...
>> > >
>> >
>> > IIUC, like vm_ops field in vma structs. Although function pointers in
>> > vm_ops are assigned statically, but you still need dynamically assign
>> > vm_ops for each instance of vma.
>> >
>> > So the code will look like this:
>> >
>> > if (parent_cg->res[i].misc_ops && parent_cg->res[i].misc_ops->alloc)
>> > {
>> > ...
>> > }
>> >
>> > I don't see this is the pattern used in cgroups and no strong opinion
>> > either way.
>> >
>> > TJ, do you have preference on this?
>>
>> I do have strong opinion on this. In the client side we want as much
>> things declared statically as we can because it gives more tools for
>> statical analysis.
>>
>> I don't want to see dynamic assignments in the SGX driver, when they
>> are not actually needed, no matter things are done in cgroups.
>
> I.e. I don't really even care what crazy things cgroups subsystem
> might do or not do. It's not my problem.
>
> All I care is that we *do not* have any use for assigning those
> pointers at run-time. So do whatever you want with cgroups side
> as long as this is not the case.
>
So I will update to something like following. Let me know if that's
correct understanding.
@tj, I'd appreciate for your input on whether this is acceptable from
cgroups side.
--- a/include/linux/misc_cgroup.h
+++ b/include/linux/misc_cgroup.h
@@ -31,22 +31,26 @@ struct misc_cg;
#include <linux/cgroup.h>
+/* per resource callback ops */
+struct misc_operations_struct {
+ int (*alloc)(struct misc_cg *cg);
+ void (*free)(struct misc_cg *cg);
+ void (*max_write)(struct misc_cg *cg);
+};
/**
* 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;
-
- /* per resource callback ops */
- int (*alloc)(struct misc_cg *cg);
- void (*free)(struct misc_cg *cg);
- void (*max_write)(struct misc_cg *cg);
+ const struct misc_operations_struct *misc_ops;
};
...
diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
index 4633b8629e63..500415087643 100644
--- a/kernel/cgroup/misc.c
+++ b/kernel/cgroup/misc.c
@@ -277,8 +277,8 @@ static ssize_t misc_cg_max_write(struct
kernfs_open_file *of, char *buf,
if (READ_ONCE(misc_res_capacity[type])) {
WRITE_ONCE(cg->res[type].max, max);
- if (cg->res[type].max_write)
- cg->res[type].max_write(cg);
+ if (cg->res[type].misc_ops &&
cg->res[type].misc_ops->max_write)
+ cg->res[type].misc_ops->max_write(cg);
[skip other similar changes in misc.c]
And on SGX side, it'll be updated like this:
--- a/arch/x86/kernel/cpu/sgx/epc_cgroup.c
+++ b/arch/x86/kernel/cpu/sgx/epc_cgroup.c
@@ -376,6 +376,14 @@ static void sgx_epc_cgroup_max_write(struct misc_cg
*cg)
queue_work(sgx_epc_cg_wq, &rc.epc_cg->reclaim_work);
}
+static int sgx_epc_cgroup_alloc(struct misc_cg *cg);
+
+const struct misc_operations_struct sgx_epc_cgroup_ops = {
+ .alloc = sgx_epc_cgroup_alloc,
+ .free = sgx_epc_cgroup_free,
+ .max_write = sgx_epc_cgroup_max_write,
+};
+
static int sgx_epc_cgroup_alloc(struct misc_cg *cg)
{
struct sgx_epc_cgroup *epc_cg;
@@ -386,12 +394,7 @@ static int sgx_epc_cgroup_alloc(struct misc_cg *cg)
sgx_lru_init(&epc_cg->lru);
INIT_WORK(&epc_cg->reclaim_work, sgx_epc_cgroup_reclaim_work_func);
- cg->res[MISC_CG_RES_SGX_EPC].alloc = sgx_epc_cgroup_alloc;
- cg->res[MISC_CG_RES_SGX_EPC].free = sgx_epc_cgroup_free;
- cg->res[MISC_CG_RES_SGX_EPC].max_write = sgx_epc_cgroup_max_write;
- cg->res[MISC_CG_RES_SGX_EPC].priv = epc_cg;
- epc_cg->cg = cg;
-
+ cg->res[MISC_CG_RES_SGX_EPC].misc_ops = &sgx_epc_cgroup_ops;
return 0;
}
Thanks again to all of you for feedback.
Haitao
next prev parent reply other threads:[~2023-10-04 15:45 UTC|newest]
Thread overview: 119+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-23 3:06 [PATCH v5 00/18] Add Cgroup support for SGX EPC memory Haitao Huang
2023-09-23 3:06 ` [PATCH v5 01/18] cgroup/misc: Add per resource callbacks for CSS events Haitao Huang
2023-09-25 17:09 ` Jarkko Sakkinen
2023-09-26 3:04 ` Haitao Huang
2023-09-26 13:10 ` Jarkko Sakkinen
2023-09-26 13:13 ` Jarkko Sakkinen
2023-09-27 1:56 ` Haitao Huang
2023-10-02 22:47 ` Jarkko Sakkinen
2023-10-02 22:55 ` Jarkko Sakkinen
2023-10-04 15:45 ` Haitao Huang [this message]
2023-10-04 17:18 ` Tejun Heo
2023-09-27 9:20 ` Huang, Kai
2023-10-03 14:29 ` Haitao Huang
2023-10-17 18:55 ` Michal Koutný
2023-09-23 3:06 ` [PATCH v5 02/18] cgroup/misc: Add SGX EPC resource type and export APIs for SGX driver Haitao Huang
2023-09-25 18:50 ` Tejun Heo
2023-09-28 3:59 ` Huang, Kai
2023-10-03 7:00 ` Haitao Huang
2023-10-03 19:33 ` Huang, Kai
2023-09-23 3:06 ` [PATCH v5 03/18] x86/sgx: Add sgx_epc_lru_lists to encapsulate LRU lists Haitao Huang
2023-09-23 3:06 ` [PATCH v5 04/18] x86/sgx: Use sgx_epc_lru_lists for existing active page list Haitao Huang
2023-09-23 3:06 ` [PATCH v5 05/18] x86/sgx: Store reclaimable EPC pages in sgx_epc_lru_lists Haitao Huang
2023-09-27 10:14 ` Huang, Kai
2023-09-23 3:06 ` [PATCH v5 06/18] x86/sgx: Introduce EPC page states Haitao Huang
2023-09-25 17:11 ` Jarkko Sakkinen
2023-09-27 10:28 ` Huang, Kai
2023-10-03 4:49 ` Haitao Huang
2023-10-03 20:03 ` Huang, Kai
2023-10-04 15:24 ` Haitao Huang
2023-10-04 21:05 ` Huang, Kai
2023-09-23 3:06 ` [PATCH v5 07/18] x86/sgx: Introduce RECLAIM_IN_PROGRESS state Haitao Huang
2023-09-25 17:13 ` Jarkko Sakkinen
2023-09-27 10:42 ` Huang, Kai
2023-09-23 3:06 ` [PATCH v5 08/18] x86/sgx: Use a list to track to-be-reclaimed pages Haitao Huang
2023-09-28 9:28 ` Huang, Kai
2023-10-03 5:09 ` Haitao Huang
2023-09-23 3:06 ` [PATCH v5 09/18] x86/sgx: Store struct sgx_encl when allocating new VA pages Haitao Huang
2023-09-27 11:14 ` Huang, Kai
2023-09-27 15:35 ` Haitao Huang
2023-09-27 21:21 ` Huang, Kai
2023-09-29 15:06 ` Haitao Huang
2023-10-02 11:05 ` Huang, Kai
2023-09-27 11:35 ` Huang, Kai
2023-10-03 6:45 ` Haitao Huang
2023-10-03 20:07 ` Huang, Kai
2023-10-04 15:03 ` Haitao Huang
2023-10-04 21:13 ` Huang, Kai
2023-10-05 4:22 ` Haitao Huang
2023-10-05 6:49 ` Huang, Kai
2023-09-23 3:06 ` [PATCH v5 10/18] x86/sgx: Add EPC page flags to identify owner types Haitao Huang
2023-09-23 3:06 ` [PATCH v5 11/18] x86/sgx: store unreclaimable pages in LRU lists Haitao Huang
2023-09-27 11:57 ` Huang, Kai
2023-10-03 5:42 ` Haitao Huang
2023-09-28 9:41 ` Huang, Kai
2023-10-03 5:15 ` Haitao Huang
2023-10-03 20:12 ` Huang, Kai
2023-09-23 3:06 ` [PATCH v5 12/18] x86/sgx: Add EPC OOM path to forcefully reclaim EPC Haitao Huang
2023-10-09 23:45 ` Huang, Kai
2023-10-10 0:23 ` Sean Christopherson
2023-10-10 0:50 ` Huang, Kai
2023-10-10 1:34 ` Huang, Kai
2023-10-10 16:49 ` Haitao Huang
2023-10-11 0:51 ` Huang, Kai
2023-10-12 13:27 ` Haitao Huang
2023-10-16 10:57 ` Huang, Kai
2023-10-16 19:52 ` Haitao Huang
2023-10-16 21:09 ` Huang, Kai
2023-10-17 0:10 ` Haitao Huang
2023-10-17 1:34 ` Huang, Kai
2023-10-17 12:58 ` Haitao Huang
2023-10-17 18:54 ` Michal Koutný
2023-10-17 19:13 ` Michal Koutný
2023-10-18 4:39 ` Haitao Huang
2023-10-18 4:37 ` Haitao Huang
2023-10-18 13:55 ` Dave Hansen
2023-10-18 15:26 ` Haitao Huang
2023-10-18 15:37 ` Dave Hansen
2023-10-18 15:52 ` Michal Koutný
2023-10-18 16:25 ` Haitao Huang
2023-10-16 21:32 ` Sean Christopherson
2023-10-17 0:09 ` Haitao Huang
2023-10-17 15:43 ` Sean Christopherson
2023-10-17 11:49 ` Mikko Ylinen
2023-10-11 1:14 ` Huang, Kai
2023-10-16 11:02 ` Huang, Kai
2023-10-10 1:42 ` Haitao Huang
2023-10-10 2:23 ` Huang, Kai
2023-10-10 13:26 ` Haitao Huang
2023-10-11 0:01 ` Sean Christopherson
2023-10-11 15:02 ` Haitao Huang
2023-10-10 1:04 ` Haitao Huang
2023-10-10 1:18 ` Huang, Kai
2023-10-10 1:38 ` Haitao Huang
2023-10-10 2:12 ` Huang, Kai
2023-10-10 17:05 ` Haitao Huang
2023-10-11 0:31 ` Huang, Kai
2023-10-11 16:04 ` Haitao Huang
2023-09-23 3:06 ` [PATCH v5 13/18] x86/sgx: Expose sgx_reclaim_pages() for use by EPC cgroup Haitao Huang
2023-10-05 12:24 ` Huang, Kai
2023-10-05 19:23 ` Haitao Huang
2023-10-05 20:25 ` Huang, Kai
2023-09-23 3:06 ` [PATCH v5 14/18] x86/sgx: Add helper to grab pages from an arbitrary EPC LRU Haitao Huang
2023-09-23 3:06 ` [PATCH v5 15/18] x86/sgx: Prepare for multiple LRUs Haitao Huang
2023-10-05 12:30 ` Huang, Kai
2023-10-05 19:33 ` Haitao Huang
2023-10-05 20:38 ` Huang, Kai
2023-09-23 3:06 ` [PATCH v5 16/18] x86/sgx: Limit process EPC usage with misc cgroup controller Haitao Huang
2023-09-25 17:15 ` Jarkko Sakkinen
2023-10-05 21:01 ` Huang, Kai
2023-10-10 0:12 ` Huang, Kai
2023-10-10 0:16 ` Huang, Kai
2023-10-10 0:26 ` Huang, Kai
2023-10-22 18:26 ` Haitao Huang
2023-10-10 9:19 ` Huang, Kai
2023-10-10 9:32 ` Huang, Kai
2023-10-17 18:54 ` Michal Koutný
2023-10-19 16:05 ` Haitao Huang
2023-09-23 3:06 ` [PATCH v5 17/18] Docs/x86/sgx: Add description for cgroup support Haitao Huang
2023-09-23 3:06 ` [PATCH v5 18/18] selftests/sgx: Add scripts for EPC cgroup testing 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=op.2cardibgwjvjmi@hhuan26-mobl.amr.corp.intel.com \
--to=haitao.huang@linux.intel.com \
--cc=anakrish@microsoft.com \
--cc=bp@alien8.de \
--cc=cgroups@vger.kernel.org \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=jarkko@kernel.org \
--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=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).