linux-kernel - Re: [PATCH v3 1/5] cpusets, hotplug: Implement cpuset tree traversal in a helper function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.00.1205141700480.25235@chino.kir.corp.google.com>
Date:	Mon, 14 May 2012 17:03:47 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
cc:	a.p.zijlstra@...llo.nl, mingo@...nel.org, pjt@...gle.com,
	paul@...lmenage.org, akpm@...ux-foundation.org, rjw@...k.pl,
	nacc@...ibm.com, paulmck@...ux.vnet.ibm.com, tglx@...utronix.de,
	seto.hidetoshi@...fujitsu.com, tj@...nel.org, mschmidt@...hat.com,
	berrange@...hat.com, nikunj@...ux.vnet.ibm.com,
	vatsa@...ux.vnet.ibm.com, liuj97@...il.com,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 1/5] cpusets, hotplug: Implement cpuset tree traversal
 in a helper function

On Mon, 14 May 2012, Srivatsa S. Bhat wrote:

> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 14f7070..23e5da6 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -2000,6 +2000,23 @@ static void remove_tasks_in_empty_cpuset(struct cpuset *cs)
>  	move_member_tasks_to_cpuset(cs, parent);
>  }
>  
> +static struct cpuset *traverse_cpusets(struct list_head *queue)

I suggest a different name for this, traverse_cpusets() doesn't imply that 
it will be returning struct cpuset *.

> +{
> +	struct cpuset *cp;
> +	struct cpuset *child;	/* scans child cpusets of cp */
> +	struct cgroup *cont;
> +
> +	cp = list_first_entry(queue, struct cpuset, stack_list);
> +	list_del(queue->next);
> +	list_for_each_entry(cont, &cp->css.cgroup->children, sibling) {
> +		child = cgroup_cs(cont);
> +		list_add_tail(&child->stack_list, queue);
> +	}
> +
> +	return cp;

Eek, what happens if queue is empty?  It can't happen with only this 
patch applied, but since you're doing this to be used in other places then 
you'd need to check for list_empty().

> +}
> +
> +
>  /*
>   * Walk the specified cpuset subtree and look for empty cpusets.
>   * The tasks of such cpuset must be moved to a parent cpuset.
> @@ -2019,19 +2036,12 @@ static void scan_for_empty_cpusets(struct cpuset *root)
>  {
>  	LIST_HEAD(queue);
>  	struct cpuset *cp;	/* scans cpusets being updated */
> -	struct cpuset *child;	/* scans child cpusets of cp */
> -	struct cgroup *cont;
>  	static nodemask_t oldmems;	/* protected by cgroup_mutex */
>  
>  	list_add_tail((struct list_head *)&root->stack_list, &queue);
>  
>  	while (!list_empty(&queue)) {
> -		cp = list_first_entry(&queue, struct cpuset, stack_list);
> -		list_del(queue.next);
> -		list_for_each_entry(cont, &cp->css.cgroup->children, sibling) {
> -			child = cgroup_cs(cont);
> -			list_add_tail(&child->stack_list, &queue);
> -		}
> +		cp = traverse_cpusets(&queue);
>  
>  		/* Continue past cpusets with all cpus, mems online */
>  		if (cpumask_subset(cp->cpus_allowed, cpu_active_mask) &&

Otherwise, looks good.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/