mm, page_alloc: reset the zone->watermark_boost early

PATCH Superseded Charan Teja Reddy 2 participants 5 messages May 11, 2020 – May 13, 2020

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Charan Teja Reddy

author

2020-05-11 13:40 UTC #0 lore raw

PATCH 1/1 mm, page_alloc: reset the zone->watermark_boost early

Updating the zone watermarks by any means, like extra_free_kbytes, min_free_kbytes, water_mark_scale_factor e.t.c, when watermark_boost is set will result into the higher low and high watermarks than the user asks. This can be avoided by resetting the zone->watermark_boost to zero early.

Signed-off-by Charan Teja Reddy <[email protected]>

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

mm/page_alloc.c +1 -1

index 1b265b09..822e262 100644
@@ -7746,9 +7746,9 @@ static void __setup_per_zone_wmarks(void)
 			    mult_frac(zone_managed_pages(zone),
 				      watermark_scale_factor, 10000));
 
+		zone->watermark_boost = 0;
 		zone->_watermark[WMARK_LOW]  = min_wmark_pages(zone) + tmp;
 		zone->_watermark[WMARK_HIGH] = min_wmark_pages(zone) + tmp * 2;
-		zone->watermark_boost = 0;
 
 		spin_unlock_irqrestore(&zone->lock, flags);
 	}

Signature · 2 lines

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

Andrew Morton

maintainer

2020-05-11 20:11 UTC #1 lore raw

Quoted 5 lines · On Mon, 11 May 2020 19:10:08 +0530 Charan Teja Reddy <[email protected]>…

Updating the zone watermarks by any means, like extra_free_kbytes, min_free_kbytes, water_mark_scale_factor e.t.c, when watermark_boost is set will result into the higher low and high watermarks than the user asks. This can be avoided by resetting the zone->watermark_boost to zero early.

Does this solve some problem which has been observed in testing?

Quoted 5 lines

...

mm/page_alloc.c static void __setup_per_zone_wmarks(void) · line 7754

Quoted 10 lines

mm/page_alloc.c +1 -1

@@ -7746,9 +7746,9 @@ static void __setup_per_zone_wmarks(void)
 			    mult_frac(zone_managed_pages(zone),
 				      watermark_scale_factor, 10000));
 
+		zone->watermark_boost = 0;
 		zone->_watermark[WMARK_LOW]  = min_wmark_pages(zone) + tmp;
 		zone->_watermark[WMARK_HIGH] = min_wmark_pages(zone) + tmp * 2;
-		zone->watermark_boost = 0;
 
 		spin_unlock_irqrestore(&zone->lock, flags);
 	}

This could only be a problem if code is accessing these things without holding zone->lock. Is that ever the case?

Charan Teja Kalla

author

2020-05-12 13:32 UTC #2 lore raw

Thank you Andrew for the reply.

Quoted 9 lines · Andrew Morton

Quoted 5 lines · On Mon, 11 May 2020 19:10:08 +0530 Charan Teja Reddy <[email protected]>…
Updating the zone watermarks by any means, like extra_free_kbytes, min_free_kbytes, water_mark_scale_factor e.t.c, when watermark_boost is set will result into the higher low and high watermarks than the user asks. This can be avoided by resetting the zone->watermark_boost to zero early.
Does this solve some problem which has been observed in testing?

Sorry, what are those issues observed in testing? It would be helpful If you post them here.

Quoted 14 lines

...

mm/page_alloc.c +1 -1

@@ -7746,9 +7746,9 @@ static void __setup_per_zone_wmarks(void)
 			    mult_frac(zone_managed_pages(zone),
 				      watermark_scale_factor, 10000));
 
+		zone->watermark_boost = 0;
 		zone->_watermark[WMARK_LOW]  = min_wmark_pages(zone) + tmp;
 		zone->_watermark[WMARK_HIGH] = min_wmark_pages(zone) + tmp * 2;
-		zone->watermark_boost = 0;

Quoted 6 lines

mm/page_alloc.c static void __setup_per_zone_wmarks(void) · line 7754
Quoted 2 lines
mm/page_alloc.c +0 -0
 		spin_unlock_irqrestore(&zone->lock, flags);
 	}
This could only be a problem if code is accessing these things without holding zone->lock. Is that ever the case?

This is a problem even when accessing these things with zone->lock held because we are directly using the macro min_wmark_pages(zone) which leads to the issue. Pasting macro here for reference.

#define min_wmark_pages(z) (z->_watermark[WMARK_MIN] + z->watermark_boost)

Steps that lead to the issue is like below:
1) On the extfrag event, we try to boost the watermark by storing the value in ->watermark_boost.

2) User changes the value of extra|min_free_kbytes or watermark_scale_factor.

In __setup_perzone_wmarks, we directly store the user asked watermarks in the zones structure. In this step, the value
is always offsets by ->watermark_boost as we use the min_wmark_pages() macro.

3) Later, when kswapd woke up, it resets the zone's watermark_boost to zero.

Step 2 from the above is what resulting into the issue.

Signature · 1 line

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Charan Teja Kalla

author

2020-05-13 09:47 UTC #3 lore raw

Quoted 13 lines · Charan Teja Kalla

Thank you Andrew for the reply.
Quoted 9 lines · Andrew Morton
Quoted 5 lines · On Mon, 11 May 2020 19:10:08 +0530 Charan Teja Reddy <[email protected]>…
Updating the zone watermarks by any means, like extra_free_kbytes, min_free_kbytes, water_mark_scale_factor e.t.c, when watermark_boost is set will result into the higher low and high watermarks than the user asks. This can be avoided by resetting the zone->watermark_boost to zero early.
Does this solve some problem which has been observed in testing?

Sorry that I misunderstood your question. Yes it has solved problem of higher water marks seen in the zone than what I set through min_free_kbytes.

Below are the steps I pursued to reproduce the problem
1) My system setup of Android kernel running on snapdragon hardware have the below settings as default:
#cat /proc/sys/vm/min_free_kbytes = 5162
#cat /proc/zoneinfo | grep -e boost -e low -e "high " -e min -e Node

	Node 0, zone   Normal
        	min      797
	        low      8340
        	high     8539
        	boost    0 // This is the extra print I have added to check the boosting

2) Now I just try to change the zone watermark when the ->watermark_boost is greater than zero. I just write the same value of min_free_kbytes in which case we should have seen the watermarks same as default(I mean of step 1)

#echo 5162 > /proc/sys/vm/min_free_kbytes

But I have seen very high values of watermarks in the system,

  # cat /proc/zoneinfo | grep -e boost -e low -e "high " -e min -e Node
	Node 0, zone   Normal
      	  min      797
      	  low      21148
      	  high     21347
      	  boost   0

So, yes, this problem is got fixed with the changes made in this patch.

Quoted 45 lines

Sorry, what are those issues observed in testing? It would be helpful If you post them here.
Quoted 14 lines
Quoted 14 lines
...
mm/page_alloc.c +1 -1
@@ -7746,9 +7746,9 @@ static void __setup_per_zone_wmarks(void)
 			    mult_frac(zone_managed_pages(zone),
 				      watermark_scale_factor, 10000));
 
+		zone->watermark_boost = 0;
 		zone->_watermark[WMARK_LOW]  = min_wmark_pages(zone) + tmp;
 		zone->_watermark[WMARK_HIGH] = min_wmark_pages(zone) + tmp * 2;
-		zone->watermark_boost = 0;
 
Quoted 6 lines
mm/page_alloc.c static void __setup_per_zone_wmarks(void) · line 7754
Quoted 2 lines
mm/page_alloc.c +0 -0
 		spin_unlock_irqrestore(&zone->lock, flags);
 	}
This could only be a problem if code is accessing these things without holding zone->lock. Is that ever the case?
This is a problem even when accessing these things with zone->lock held because we are directly using the macro min_wmark_pages(zone) which leads to the issue. Pasting macro here for reference.
#define min_wmark_pages(z) (z->_watermark[WMARK_MIN] + z->watermark_boost)
Steps that lead to the issue is like below:
1) On the extfrag event, we try to boost the watermark by storing the value in ->watermark_boost.
2) User changes the value of extra|min_free_kbytes or watermark_scale_factor.
In __setup_perzone_wmarks, we directly store the user asked watermarks in the zones structure. In this step, the value
is always offsets by ->watermark_boost as we use the min_wmark_pages() macro.
3) Later, when kswapd woke up, it resets the zone's watermark_boost to zero.
Step 2 from the above is what resulting into the issue.

Signature · 1 line

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Andrew Morton

maintainer

2020-05-13 22:24 UTC #4 lore raw

Quoted 1 line · On Wed, 13 May 2020 15:16:53 +0530 Charan Teja Kalla <[email protected]>…

So, yes, this problem is got fixed with the changes made in this patch.

OK, thanks.

Could you please prepare a v2 with a changelog which includes the additional info in your two replies?