Re: [PATCH 02/27] mm, vmscan: Check if cpusets are enabled during direct reclaim

[Vlastimil Babka]

On 03/09/2016 12:59 PM, Mel Gorman wrote:
> On Thu, Mar 03, 2016 at 12:31:40PM +0100, Vlastimil Babka wrote:
>> On 02/23/2016 04:04 PM, Mel Gorman wrote:
>>> Direct reclaim obeys cpusets but misses the cpusets_enabled() check.
>>> The overhead is unlikely to be measurable in the direct reclaim
>>> path which is expensive but there is no harm is doing it.
>>>
>>> Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>>  mm/vmscan.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index 86eb21491867..de8d6226e026 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -2566,7 +2566,7 @@ static void shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
>>>  		 * to global LRU.
>>>  		 */
>>>  		if (global_reclaim(sc)) {
>>> -			if (!cpuset_zone_allowed(zone,
>>> +			if (cpusets_enabled() && !cpuset_zone_allowed(zone,
>>>  						 GFP_KERNEL | __GFP_HARDWALL))
>>>  				continue;
>>
>> Hmm, wouldn't it be nicer if cpuset_zone_allowed() itself did the right
>> thing, and not each caller?
>>
>> How about the patch below? (+CC)
>>
> 
> The patch appears to be layer upon the entire series but that in itself

It could be also completely separate, witch your 02/27 dropped as it's
not tied to the rework anyway? Or did I miss something else cpuset
related in later patches?

> is ok. This part is a problem
> 
>> An important function for cpusets is cpuset_node_allowed(), which acknowledges
>> that if there's a single root CPU set, it must be trivially allowed. But the
>> check "nr_cpusets() <= 1" doesn't use the cpusets_enabled_key static key in a
>> proper way where static keys can reduce the overhead.
> 
> 
> There is one check for the static key and a second for the count to see
> if it's likely a valid cpuset that matters has been configured.

The point is that these should be equivalent, as the static key becomes
enabled only when there's more than one (root) cpuset. So checking
"nr_cpusets() <= 1" does the same as "!cpusets_enabled()", but without
taking advantage of the static key code patching.

> The
> point of that check was that it was lighter than __cpuset_zone_allowed
> in the case where no cpuset is configured.

But shrink_zones() (which you were patching) uses cpuset_zone_allowed(),
not __cpuset_zone_allowed(). The latter is provided only for
get_page_from_freelist(), which inserts extra fast check between
cpusets_enabled() and the slow cpuset allowed checking.

> The patches are not equivalent.
>