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

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

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@intel.com>