Re: [PATCH v3 2/5] cpusets, hotplug: Restructure functions that are invoked during hotplug

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 05/15/2012 05:57 AM, David Rientjes wrote:

> On Mon, 14 May 2012, Srivatsa S. Bhat wrote:
> 
>> Separate out the cpuset related handling for CPU/Memory online/offline.
>> This also helps us exploit the most obvious and basic level of optimization
>> that any notification mechanism (CPU/Mem online/offline) has to offer us:
>> "We *know* why we have been invoked. So stop pretending that we are lost,
>> and do only the necessary amount of processing!".
>>
>> And while at it, rename scan_for_empty_cpusets() to
>> scan_cpusets_upon_hotplug(), which will be more appropriate, considering
>> the upcoming changes.
>>
> 
> If it's more appropriate in upcoming changes, then change it in the 
> upcoming changes that make it more appropriate?
> 


Well, I wanted to split out the core of the fix from the rest of the cleanup,
so that the fix patch can be more focussed, thereby easing review.

And I think renaming this function is more of a noise, when compared with the
fix being implemented in the later patches.. So I thought I'll get it out of
the way by doing it here itself.

Moreover, that renaming is justified in this patch itself, IMHO.. It doesn't
really have to wait till the later ones, because considering the restructuring
that this patch does, the renaming is in order too..

>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Cc: stable@vger.kernel.org
>> ---
[...]
>> +
>> +	case CPUSET_MEM_OFFLINE:
>> +		while (!list_empty(&queue)) {
>> +			cp = traverse_cpusets(&queue);
>> +
>> +			/* Continue past cpusets with all mems online */
>> +			if (nodes_subset(cp->mems_allowed,
>> +					node_states[N_HIGH_MEMORY]))
>> +				continue;
>> +
>> +			oldmems = cp->mems_allowed;
>> +
>> +			/* Remove offline mems from this cpuset. */
>> +			mutex_lock(&callback_mutex);
>> +			nodes_and(cp->mems_allowed, cp->mems_allowed,
>>  						node_states[N_HIGH_MEMORY]);
>> -		mutex_unlock(&callback_mutex);
>> +			mutex_unlock(&callback_mutex);
>>  
>> -		/* Move tasks from the empty cpuset to a parent */
>> -		if (cpumask_empty(cp->cpus_allowed) ||
>> -		     nodes_empty(cp->mems_allowed))
>> -			remove_tasks_in_empty_cpuset(cp);
>> -		else {
>> -			update_tasks_cpumask(cp, NULL);
>> -			update_tasks_nodemask(cp, &oldmems, NULL);
>> +			/* Move tasks from the empty cpuset to a parent */
>> +			if (nodes_empty(cp->mems_allowed))
>> +				remove_tasks_in_empty_cpuset(cp);
>> +			else
>> +				update_tasks_nodemask(cp, &oldmems, NULL);
>>  		}
>>  	}
>>  }
> 
> This looks like a good optimization, but the existing comment for 
> scan_for_empty_cpusets() is wrong: we certainly do not lack memory 
> hot-unplug and it will remove nodes from N_HIGH_MEMORY if all present 
> pages from a node are offlined.  I had a patch that emulated node 
> hot-remove on x86 and this worked fine.  So perhaps update that existing 
> comment as well (not shown in this diff)?
> 


Sure, will do.

> Otherwise, looks good.


Thanks a lot!
 
Regards,
Srivatsa S. Bhat