Re: [PATCH 2/6] cgroup: drop module support

[Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

Hello, Li.

On Wed, Jan 29, 2014 at 12:16:38PM +0800, Li Zefan wrote:
> > -#define for_each_builtin_subsys(ss, i)					\
> > -	for ((i) = 0; (i) < CGROUP_BUILTIN_SUBSYS_COUNT &&		\
> > -	     (((ss) = cgroup_subsys[i]) || true); (i)++)
> > +	for ((ssid) = 0; (ssid) < CGROUP_SUBSYS_COUNT &&		\
> > +	     (((ss) = cgroup_subsys[ssid]) || true); (ssid)++)
> 
> Now cgroup_subsys[i] won't be NULL, so we can drop "|| true".

Hmmm... because the macro is kinda complex, I'd like to avoid changing
its implementation in this patch.  Also, the "|| true" tells the
compiler that it doesn't have to generate conditional branch on the
preceding condition.

Now that the array is always consecutive, I'm planning to further
simplify the iterators to not require ssid, so that it just becomes
for_each_subsys(ss).  Let's leave it alone for now.

> > +	if (need_forkexit_callback)
> > +		for_each_subsys(ss, i)
> >  			if (ss->fork)
> >  				ss->fork(child);
> > -	}
> 
> This looks a bit ugly. How about leaving the parentheses for the
> outmost if statement?
> 
> if (need_forkexit_callback) {
> 	for_each_subsys(ss, i)
> 		if (ss->fork)
> 			ss->fork(child);
> }

Maybe, I don't know.  I tend to aim for the minimum necessary as that
usually is the easiest way to achieve consistency.  That said certain
things are a lot easier on the eye with a bit of extra notations -
e.g. "(a & b) && c".  Alright, will add the parentheses.

Thanks!

-- 
tejun