Subject: Re: [PATCH v4 4/8] cgroup: Remove CGROUP_BUILTIN_SUBSYS_COUNT

From: Li Zefan <[email-masked]>
Date: Thu, 13 Sep 2012 14:41:03 +0800

> @@ -1321,11 +1321,13 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
>  	 * take duplicate reference counts on a subsystem that's already used,
>  	 * but rebind_subsystems handles this case.
>  	 */
> -	for (i = CGROUP_BUILTIN_SUBSYS_COUNT; i < CGROUP_SUBSYS_COUNT; i++) {
> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>  		unsigned long bit = 1UL << i;
>  
>  		if (!(bit & opts->subsys_mask))
>  			continue;
> +		if (!subsys[i]->module)
> +			continue;

This check is not necessary. If it's builtin, try_module_get() will just return 1, and
we're fine.

>  		if (!try_module_get(subsys[i]->module)) {
>  			module_pin_failed = true;
>  			break;
> @@ -1337,12 +1339,14 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
>  		 * raced with a module_delete call, and to the user this is
>  		 * essentially a "subsystem doesn't exist" case.
>  		 */
> -		for (i--; i >= CGROUP_BUILTIN_SUBSYS_COUNT; i--) {
> +		for (i--; i >= 0; i--) {
>  			/* drop refcounts only on the ones we took */
>  			unsigned long bit = 1UL << i;
>  
>  			if (!(bit & opts->subsys_mask))
>  				continue;
> +			if (!subsys[i]->module)
> +				continue;

ditto

>  			module_put(subsys[i]->module);
>  		}
>  		return -ENOENT;
> @@ -1354,11 +1358,13 @@ static int parse_cgroupfs_options(char *data, struct cgroup_sb_opts *opts)
>  static void drop_parsed_module_refcounts(unsigned long subsys_mask)
>  {
>  	int i;
> -	for (i = CGROUP_BUILTIN_SUBSYS_COUNT; i < CGROUP_SUBSYS_COUNT; i++) {
> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>  		unsigned long bit = 1UL << i;
>  
>  		if (!(bit & subsys_mask))
>  			continue;
> +		if (!subsys[i]->module)
> +			continue;

ditto

>  		module_put(subsys[i]->module);
>  	}
>  }
> @@ -1437,6 +1443,7 @@ static void init_cgroup_housekeeping(struct cgroup *cgrp)
>  	INIT_LIST_HEAD(&cgrp->event_list);
>  	spin_lock_init(&cgrp->event_list_lock);
>  	simple_xattrs_init(&cgrp->xattrs);
> +	memset(cgrp->subsys, 0, sizeof(cgrp->subsys));

This seems an unrelated change, and is redundant. Am I missing something?

>  }