Re: [PATCH 3/4] cpuset: Fix unchecked calls to NODEMASK_ALLOC()

[David Rientjes <rientjes@google.com>]

On Tue, 22 Feb 2011, Li Zefan wrote:

> [PATCH 3/4] cpuset: Fix unchecked calls to NODEMASK_ALLOC()
> 
> Those functions that use NODEMASK_ALLOC() can't propogate errno
> to users, so might fail silently.
> 
> Fix it by using one static nodemask_t variable for each function, and
> those variables are protected by cgroup_mutex.
> 

I think there would also be incentive to do the same thing for 
update_nodemask() even though its caller can handle -ENOMEM.  Imagine 
current being out of memory and the NODEMASK_ALLOC() subsequently failing 
because it is oom yet it may be trying to give itself more memory.  It's 
also protected by cgroup_mutex so the only thing we're sacrificing is 8 
bytes on the defconfig and 256 bytes even with CONFIG_NODES_SHIFT == 10.  
On machines that large, this seems like an acceptable cost to ensure we 
can give ourselves more memory :)

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cpuset.c |   50 ++++++++++++++++----------------------------------
>  1 files changed, 16 insertions(+), 34 deletions(-)
> 
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index 8fef8c6..073ce91 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -1015,17 +1015,12 @@ static void cpuset_change_nodemask(struct task_struct *p,
>  	struct cpuset *cs;
>  	int migrate;
>  	const nodemask_t *oldmem = scan->data;
> -	NODEMASK_ALLOC(nodemask_t, newmems, GFP_KERNEL);
> -
> -	if (!newmems)
> -		return;
> +	static nodemask_t newmems;	/* protected by cgroup_mutex */
>  
>  	cs = cgroup_cs(scan->cg);
> -	guarantee_online_mems(cs, newmems);
> -
> -	cpuset_change_task_nodemask(p, newmems);
> +	guarantee_online_mems(cs, &newmems);

The newmems nodemask is going to be persistant across calls since it is 
static, so we have to be careful that nothing depends on it being 
NODE_MASK_NONE.  Indeed, NODEMASK_ALLOC() with just GFP_KERNEL doesn't 
guarantee anything different.  guarantee_online_mems() sets the nodemask, 
so this looks good.

>  
> -	NODEMASK_FREE(newmems);
> +	cpuset_change_task_nodemask(p, &newmems);
>  
>  	mm = get_task_mm(p);
>  	if (!mm)
> @@ -1438,41 +1433,35 @@ static void cpuset_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>  	struct mm_struct *mm;
>  	struct cpuset *cs = cgroup_cs(cont);
>  	struct cpuset *oldcs = cgroup_cs(oldcont);
> -	NODEMASK_ALLOC(nodemask_t, to, GFP_KERNEL);
> -
> -	if (to == NULL)
> -		goto alloc_fail;
> +	static nodemask_t to;		/* protected by cgroup_mutex */
>  
>  	if (cs == &top_cpuset) {
>  		cpumask_copy(cpus_attach, cpu_possible_mask);
>  	} else {
>  		guarantee_online_cpus(cs, cpus_attach);
>  	}
> -	guarantee_online_mems(cs, to);
> +	guarantee_online_mems(cs, &to);
>  
>  	/* do per-task migration stuff possibly for each in the threadgroup */
> -	cpuset_attach_task(tsk, to, cs);
> +	cpuset_attach_task(tsk, &to, cs);
>  	if (threadgroup) {
>  		struct task_struct *c;
>  		rcu_read_lock();
>  		list_for_each_entry_rcu(c, &tsk->thread_group, thread_group) {
> -			cpuset_attach_task(c, to, cs);
> +			cpuset_attach_task(c, &to, cs);
>  		}
>  		rcu_read_unlock();
>  	}
>  
>  	/* change mm; only needs to be done once even if threadgroup */
> -	*to = cs->mems_allowed;
> +	to = cs->mems_allowed;
>  	mm = get_task_mm(tsk);
>  	if (mm) {
> -		mpol_rebind_mm(mm, to);
> +		mpol_rebind_mm(mm, &to);
>  		if (is_memory_migrate(cs))
> -			cpuset_migrate_mm(mm, &oldcs->mems_allowed, to);
> +			cpuset_migrate_mm(mm, &oldcs->mems_allowed, &to);
>  		mmput(mm);
>  	}
> -
> -alloc_fail:
> -	NODEMASK_FREE(to);
>  }
>  
>  /* The various types of files and directories in a cpuset file system */
> @@ -2051,10 +2040,7 @@ static void scan_for_empty_cpusets(struct cpuset *root)
>  	struct cpuset *cp;	/* scans cpusets being updated */
>  	struct cpuset *child;	/* scans child cpusets of cp */
>  	struct cgroup *cont;
> -	NODEMASK_ALLOC(nodemask_t, oldmems, GFP_KERNEL);
> -
> -	if (oldmems == NULL)
> -		return;
> +	static nodemask_t oldmems;	/* protected by cgroup_mutex */
>  
>  	list_add_tail((struct list_head *)&root->stack_list, &queue);
>  
> @@ -2071,7 +2057,7 @@ static void scan_for_empty_cpusets(struct cpuset *root)
>  		    nodes_subset(cp->mems_allowed, node_states[N_HIGH_MEMORY]))
>  			continue;
>  
> -		*oldmems = cp->mems_allowed;
> +		oldmems = cp->mems_allowed;
>  
>  		/* Remove offline cpus and mems from this cpuset. */
>  		mutex_lock(&callback_mutex);
> @@ -2087,10 +2073,9 @@ static void scan_for_empty_cpusets(struct cpuset *root)
>  			remove_tasks_in_empty_cpuset(cp);
>  		else {
>  			update_tasks_cpumask(cp, NULL);
> -			update_tasks_nodemask(cp, oldmems, NULL);
> +			update_tasks_nodemask(cp, &oldmems, NULL);
>  		}
>  	}
> -	NODEMASK_FREE(oldmems);
>  }
>  
>  /*
> @@ -2132,19 +2117,16 @@ void cpuset_update_active_cpus(void)
>  static int cpuset_track_online_nodes(struct notifier_block *self,
>  				unsigned long action, void *arg)
>  {
> -	NODEMASK_ALLOC(nodemask_t, oldmems, GFP_KERNEL);
> -
> -	if (oldmems == NULL)
> -		return NOTIFY_DONE;
> +	static nodemask_t oldmems;	/* protected by cgroup_mutex */
>  
>  	cgroup_lock();
>  	switch (action) {
>  	case MEM_ONLINE:
> -		*oldmems = top_cpuset.mems_allowed;
> +		oldmems = top_cpuset.mems_allowed;
>  		mutex_lock(&callback_mutex);
>  		top_cpuset.mems_allowed = node_states[N_HIGH_MEMORY];
>  		mutex_unlock(&callback_mutex);
> -		update_tasks_nodemask(&top_cpuset, oldmems, NULL);
> +		update_tasks_nodemask(&top_cpuset, &oldmems, NULL);
>  		break;
>  	case MEM_OFFLINE:
>  		/*

The NODEMASK_FREE() wasn't removed from cpuset_track_online_nodes().  
After that's fixed:

Acked-by: David Rientjes <rientjes@google.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>