Re: [PATCH v6 2/3] cgroups: allow a cgroup subsystem to reject a fork

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

Hello, Aleksa.

On Sat, Mar 14, 2015 at 03:37:14PM +1100, Aleksa Sarai wrote:
> +/* Ditto for the can_fork/cancel_fork/reapply_fork callbacks. */
> +static int need_canfork_callback __read_mostly = 0,
> +	   need_reapplyfork_callback __read_mostly = 0;

Please separate the two definitions.  Please consult
Documentation/CodingStyle and in general try not to be creative with
style.

> +int cgroup_can_fork(struct task_struct *child, void **state)
> +{
> +	struct cgroup_subsys *ss;
> +	struct css_set *cset;
> +	int i, j, retval;
> +
> +	cset = task_css_set(current);
> +	get_css_set(cset);

So, you stuck with css_set refcnting.  That's fine, but please do
finish discussing in the original thread instead of throwing out new
version without explaining why you reached a particular conclusion.
Did you explore the idea of passing an opaque pointer from can_fork to
post_fork?  If so, why did you choose this direction instead of that
one?

Also, what pins the cset between task_css_set() and get_css_set()?
task_css_set() is protected by RCU and the above should have triggered
RCU warning if the debug option is enabled.  Please always test with
the debug options enabled, especially the lockdep and RCU ones.

> +	*state = cset;

There's no reason for css_set pointer to be passed around as an opaque
pointer.  The type is fixed.  I suppose this is from me mentioning
void pointer in the previous thread but that was about the cpuset
can_fork() implementation returning its css pointer, not css_set.
Please conclude discussions before working on the following version.

> @@ -5249,6 +5315,24 @@ void cgroup_post_fork(struct task_struct *child)
>  	}
>  
>  	/*
> +	 * Deal with tasks that were migrated mid-fork. If the css_set
> +	 * changed between can_fork() and post_fork() an organisation
> +	 * operation has occurred, and we need to revert/reapply the
> +	 * the can_fork().
> +	 */
> +	for_each_subsys_which(need_canfork_callback, ss, i) {
> +		struct cgroup_subsys_state *css = cset->subsys[i],
> +					   *old_css = old_cset->subsys[i];
> +
> +		/*
> +		 * We only reapply for subsystems whose
> +		 * association changed in the interim.
> +		 */
> +		if (old_css != css && ss->reapply_fork)
> +			ss->reapply_fork(css, old_css, child);
> +	}

Do we really need a separate callback for this?  Can't we just add
@old_css param to ss->fork() which is NULL if it didn't change since
can_fork()?

> @@ -1196,6 +1196,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  {
>  	int retval;
>  	struct task_struct *p;
> +	void *cfs;

If we're gonna do css_set, there's no need to make this an opaque
pointer.

Thanks.

-- 
tejun