[PATCH] mm/vmscan: fix highidx argument type

[PATCH] mm/vmscan: fix highidx argument type

[Michael S. Tsirkin]

for_each_zone_zonelist_nodemask wants an enum zone_type
argument, but is passed gfp_t:

mm/vmscan.c:2658:9:    expected int enum zone_type [signed] highest_zoneidx
mm/vmscan.c:2658:9:    got restricted gfp_t [usertype] gfp_mask
mm/vmscan.c:2658:9: warning: incorrect type in argument 2 (different base types)
mm/vmscan.c:2658:9:    expected int enum zone_type [signed] highest_zoneidx
mm/vmscan.c:2658:9:    got restricted gfp_t [usertype] gfp_mask

convert argument to the correct type.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

In case this was already fixed, pls ignore - seems to still be
there in latest master.

 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index ab2505c..dcd90c8 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2656,7 +2656,7 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	 * should make reasonable progress.
 	 */
 	for_each_zone_zonelist_nodemask(zone, z, zonelist,
-					gfp_mask, nodemask) {
+					gfp_zone(gfp_mask), nodemask) {
 		if (zone_idx(zone) > ZONE_NORMAL)
 			continue;
 
-- 
MST

Re: [PATCH] mm/vmscan: fix highidx argument type

[Andrew Morton]

On Fri, 16 Jan 2015 00:18:12 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:

> for_each_zone_zonelist_nodemask wants an enum zone_type
> argument, but is passed gfp_t:
> 
> mm/vmscan.c:2658:9:    expected int enum zone_type [signed] highest_zoneidx
> mm/vmscan.c:2658:9:    got restricted gfp_t [usertype] gfp_mask
> mm/vmscan.c:2658:9: warning: incorrect type in argument 2 (different base types)
> mm/vmscan.c:2658:9:    expected int enum zone_type [signed] highest_zoneidx
> mm/vmscan.c:2658:9:    got restricted gfp_t [usertype] gfp_mask

Which tool emitted these warnings?

> convert argument to the correct type.
> 
> ...
>
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2656,7 +2656,7 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
>  	 * should make reasonable progress.
>  	 */
>  	for_each_zone_zonelist_nodemask(zone, z, zonelist,
> -					gfp_mask, nodemask) {
> +					gfp_zone(gfp_mask), nodemask) {
>  		if (zone_idx(zone) > ZONE_NORMAL)
>  			continue;

hm, I wonder what the runtime effects are.

The throttle_direct_reclaim() comment isn't really accurate, is it? 
"Throttle direct reclaimers if backing storage is backed by the
network".  The code is applicable to all types of backing, but was
added to address problems which are mainly observed with network
backing?

Re: [PATCH] mm/vmscan: fix highidx argument type

[Michael S. Tsirkin]

On Thu, Jan 15, 2015 at 02:49:20PM -0800, Andrew Morton wrote:
> On Fri, 16 Jan 2015 00:18:12 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > for_each_zone_zonelist_nodemask wants an enum zone_type
> > argument, but is passed gfp_t:
> > 
> > mm/vmscan.c:2658:9:    expected int enum zone_type [signed] highest_zoneidx
> > mm/vmscan.c:2658:9:    got restricted gfp_t [usertype] gfp_mask
> > mm/vmscan.c:2658:9: warning: incorrect type in argument 2 (different base types)
> > mm/vmscan.c:2658:9:    expected int enum zone_type [signed] highest_zoneidx
> > mm/vmscan.c:2658:9:    got restricted gfp_t [usertype] gfp_mask
> 
> Which tool emitted these warnings?

Oh, sorry.
It's sparce.

> > convert argument to the correct type.
> > 
> > ...
> >
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -2656,7 +2656,7 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
> >  	 * should make reasonable progress.
> >  	 */
> >  	for_each_zone_zonelist_nodemask(zone, z, zonelist,
> > -					gfp_mask, nodemask) {
> > +					gfp_zone(gfp_mask), nodemask) {
> >  		if (zone_idx(zone) > ZONE_NORMAL)
> >  			continue;
> 
> hm, I wonder what the runtime effects are.
> 
> The throttle_direct_reclaim() comment isn't really accurate, is it? 
> "Throttle direct reclaimers if backing storage is backed by the
> network".  The code is applicable to all types of backing, but was
> added to address problems which are mainly observed with network
> backing?


As far as I can tell, yes. It would seem that it can cause
deadlocks in theory.  Cc stable on the grounds that it's obvious?

-- 
MST

Re: [PATCH] mm/vmscan: fix highidx argument type

[Vlastimil Babka]

On 01/16/2015 08:07 AM, Michael S. Tsirkin wrote:
> On Thu, Jan 15, 2015 at 02:49:20PM -0800, Andrew Morton wrote:
>> On Fri, 16 Jan 2015 00:18:12 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > for_each_zone_zonelist_nodemask wants an enum zone_type
>> > argument, but is passed gfp_t:
>> > 
>> > mm/vmscan.c:2658:9:    expected int enum zone_type [signed] highest_zoneidx
>> > mm/vmscan.c:2658:9:    got restricted gfp_t [usertype] gfp_mask
>> > mm/vmscan.c:2658:9: warning: incorrect type in argument 2 (different base types)
>> > mm/vmscan.c:2658:9:    expected int enum zone_type [signed] highest_zoneidx
>> > mm/vmscan.c:2658:9:    got restricted gfp_t [usertype] gfp_mask
>> 
>> Which tool emitted these warnings?
> 
> Oh, sorry.
> It's sparce.
> 
>> > convert argument to the correct type.
>> > 
>> > ...
>> >
>> > --- a/mm/vmscan.c
>> > +++ b/mm/vmscan.c
>> > @@ -2656,7 +2656,7 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
>> >  	 * should make reasonable progress.
>> >  	 */
>> >  	for_each_zone_zonelist_nodemask(zone, z, zonelist,
>> > -					gfp_mask, nodemask) {
>> > +					gfp_zone(gfp_mask), nodemask) {
>> >  		if (zone_idx(zone) > ZONE_NORMAL)
>> >  			continue;
>> 
>> hm, I wonder what the runtime effects are.

So this was introduced by 675becce15f "mm: vmscan: do not throttle based on
pfmemalloc reserves if node has no ZONE_NORMAL" in 3.15. AFAICS gfp_mask >=
gfp_zone(gfp_mask), so the high_zoneidx will be higher than it should, and
next_zones_zonelist() won't filter the higher-than-wanted zones as it should.

I guess the runtime effects is that allocations for zone_type < NORMAL, i.e.
DMA32 or DMA, can now wrongly choose a numa node without such zones, for
checking pfmemalloc reserves and throttling. Which means the throttling can be
ineffective, or it could also throttle without actually needing to, if the wrong
zone has lower reserves? Mel?

>> The throttle_direct_reclaim() comment isn't really accurate, is it? 
>> "Throttle direct reclaimers if backing storage is backed by the
>> network".  The code is applicable to all types of backing, but was
>> added to address problems which are mainly observed with network
>> backing?

I guess. I also don't see any code restricting this just for network.

> 
> 
> As far as I can tell, yes. It would seem that it can cause
> deadlocks in theory.  Cc stable on the grounds that it's obvious?

I don't think this mistake can introduce deadlocks on its own, but it also won't
prevent any problems that the throttling was suppsoed to prevent.
I agree it should go stable.

BTW, I wonder if the whole code couldn't be much simpler by capping high_zoneidx
by ZONE_NORMAL before traversing the zonelist, like this:

int high_zoneidx = min(gfp_zone(gfp_mask), ZONE_NORMAL);

first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
pgdat = zone->zone_pgdat;

if (!pgdat || pfmemalloc_watermark_ok(pgdat))
	goto out;

Re: [PATCH] mm/vmscan: fix highidx argument type

[Vlastimil Babka]

On 01/16/2015 07:53 PM, Vlastimil Babka wrote:
> BTW, I wonder if the whole code couldn't be much simpler by capping high_zoneidx
> by ZONE_NORMAL before traversing the zonelist, like this:
> 
> int high_zoneidx = min(gfp_zone(gfp_mask), ZONE_NORMAL);
> 
> first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);

Erm, s/NULL/nodemask/ here. I copy/pasted this from before 675becce15f32, where
it didn't actually use the nodemask parameter of throttle_direct_reclaim(),
Wonder why, looks like another bug to me, that the commit has silently fixed.

> pgdat = zone->zone_pgdat;
> 
> if (!pgdat || pfmemalloc_watermark_ok(pgdat))
> 	goto out;
> 
> 
> --
> 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/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>