Re: [PATCH v16 05/16] x86/sgx: Implement basic EPC misc cgroup functionality

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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>




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

  Powered by Linux