Re: [PATCH v14 14/14] selftests/sgx: Add scripts for EPC cgroup testing

[Huang, Kai]

> > Reorg:
> >
> > void sgx_cgroup_init(void)
> > {
> >     struct workqueue_struct *wq;
> >
> >     /* eagerly allocate the workqueue: */
> >     wq = alloc_workqueue("sgx_cg_wq", wq_unbound | wq_freezable, 
> > wq_unbound_max_active);
> >     if (!wq) {
> >         pr_warn("sgx_cg_wq creation failed\n");
> >         return;
>
> sgx_cgroup_try_charge() expects sgx_cg_wq, so it would break unless we 
> check and return 0 which was the initially implemented in v12. But then 
> Kai had some concern on that we expose all the interface files to allow 
> user to set limits but we don't enforce. To keep it simple we settled 
> down back to BUG_ON(). 

[...]

> This would only happen rarely and user can add 
> command-line to disable SGX if s/he really wants to start kernel in this 
> case, just can't do SGX.

Just to be clear that I don't like BUG_ON() either.  It's inevitable you 
will get attention because of using it.

This is a compromise that you don't want to reset "capacity" to 0 when 
alloc_workqueue() fails.

There are existing code where BUG_ON() is used during the kernel early 
boot code when memory allocation fails (e.g., see cgroup_init_subsys()), 
so it might be acceptable to use BUG_ON() here, but it's up to 
maintainers to decide whether it is OK.

[...]

> > With "--strict" flag I also catched these:
> >
> > CHECK: spinlock_t definition without comment
> > #1308: FILE: arch/x86/kernel/cpu/sgx/sgx.h:122:
> > +    spinlock_t lock;
> Yes I had a comment but Kai thought it was too obvious and I can't think 
> of a better one that's not obvious so I removed:
>
> https://lore.kernel.org/linux-sgx/9ffb02a3344807f2c173fe8c7cb000cd6c7843b6.camel@xxxxxxxxx/
To be clear, my reply was really about the comment itself isn't useful, 
but didn't say we shouldn't use a comment here.

> > CHECK: multiple assignments should be avoided
> > #444: FILE: kernel/cgroup/misc.c:450:
> > +        parent_cg = cg = &root_cg;
>
> This was also suggested by Kai a few versions back:
> https://lore.kernel.org/linux-sgx/8f08f0b0f2b04b90d7cdb7b628f16f9080687c43.camel@xxxxxxxxx/

I didn't know checkpatch complains this.  Feel free to revert back as it 
is trivial to me.