Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1354058 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 02/27] mm, vmscan: Check if cpusets are enabled during  direct reclaim Mel Gorman <mgorman@techsingularity.net> - 2016-03-09 13:00 +0100
    Re: [PATCH 02/27] mm, vmscan: Check if cpusets are enabled during  direct reclaim Vlastimil Babka <vbabka@suse.cz> - 2016-03-09 13:40 +0100

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

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@techsingularity.net>
> > ---
> >  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
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 of that check was that it was lighter than __cpuset_zone_allowed
in the case where no cpuset is configured.

The patches are not equivalent.

-- 
Mel Gorman
SUSE Labs

[toc] | [next] | [standalone]

#1354081

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@techsingularity.net>
>>> ---
>>>  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.
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web