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

["Huang, Kai" <kai.huang@intel.com>]

> +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?