On 28/08/2024 11:11 am, Huang, Kai wrote:
+static void sgx_cgroup_misc_init(struct misc_cg *cg, struct
sgx_cgroup *sgx_cg)
+{
+ cg->res[MISC_CG_RES_SGX_EPC].priv = sgx_cg;
+ sgx_cg->cg = cg;
+}
+
[...]
+int __init sgx_cgroup_init(void)
+{
+ sgx_cgroup_misc_init(misc_cg_root(), &sgx_cg_root);
+
+ return 0;
+} > +
+/**
+ * Register capacity and ops for SGX cgroup.
+ * Only called at the end of sgx_init() when SGX is ready to handle
the ops
+ * callbacks.
+ */
+void __init sgx_cgroup_register(void)
+{
+ unsigned int nid = first_node(sgx_numa_mask);
+ unsigned int first = nid;
+ u64 capacity = 0;
+
+ misc_cg_set_ops(MISC_CG_RES_SGX_EPC, &sgx_cgroup_ops);
+
+ /* sgx_numa_mask is not empty when this is called */
+ do {
+ capacity += sgx_numa_nodes[nid].size;
+ nid = next_node_in(nid, sgx_numa_mask);
+ } while (nid != first);
+ misc_cg_set_capacity(MISC_CG_RES_SGX_EPC, capacity);
+}
[...]
@@ -930,6 +961,9 @@ static int __init sgx_init(void)
if (ret)
goto err_kthread;
+ ret = sgx_cgroup_init();
+ if (ret)
+ goto err_provision;
/*
* Always try to initialize the native *and* KVM drivers.
* The KVM driver is less picky than the native one and
@@ -943,6 +977,8 @@ static int __init sgx_init(void)
if (sgx_vepc_init() && ret)
goto err_provision;
In sgx_cgroup_init():
sgx_cgroup_misc_init(misc_cg_root(), &sgx_cg_root);
.. also cannot fail.
I think it should be moved to the sgx_cgroup_register(). Otherwise, if
any step after sgx_cgroup_init() fails, there's no unwind for the above
operation.
The consequence is the misc_cg_root()->res[EPC].priv will remain
pointing to the SGX root cgroup.
It shouldn't cause any real issue for now, but it's weird to have that
set, and can potentially cause problem in the future.
+ sgx_cgroup_register();
+
return 0;
err_provision:
So, I think we should do:
1) Rename sgx_cgroup_register() -> sgx_cgroup_init(), and move the
sgx_cgroup_misc_init(misc_cg_root(), &sgx_cg_root);
to it. All operations in the (new) sgx_cgroup_init() won't fail.
2) Remove (existing) sgx_cgroup_init() form this patch, but introduce it
in the patch "x86/sgx: Implement async reclamation for cgroup" and
rename it to sgx_cgroup_prepare() or something. It just allocates
workqueue inside. And sgx_cgroup_deinit() -> sgx_cgroup_cleanup().
Makes sense?
With the above addressed, and the k-doc warning fixed:
Reviewed-by: Kai Huang <kai.huang@xxxxxxxxx>
