mm: page_alloc: fix watermark check in __zone_watermark_ok()

PATCH In review Tomasz Stanislawski 6 participants 7 messages May 9, 2013 – Jun 4, 2013

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Tomasz Stanislawski

author

2013-05-09 07:51 UTC #0 lore raw

PATCH 1/1 mm: page_alloc: fix watermark check in __zone_watermark_ok()

The watermark check consists of two sub-checks.
The first one is:

	if (free_pages <= min + lowmem_reserve)
		return false;

The check assures that there is minimal amount of RAM in the zone. If CMA is used then the free_pages is reduced by the number of free pages in CMA prior to the over-mentioned check.

	if (!(alloc_flags & ALLOC_CMA))
		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);

This prevents the zone from being drained from pages available for non-movable allocations.

The second check prevents the zone from getting too fragmented.

	for (o = 0; o < order; o++) {
		free_pages -= z->free_area[o].nr_free << o;
		min >>= 1;
		if (free_pages <= min)
			return false;
	}

The field z->free_area[o].nr_free is equal to the number of free pages including free CMA pages. Therefore the CMA pages are subtracted twice. This may cause a false positive fail of __zone_watermark_ok() if the CMA area gets strongly fragmented. In such a case there are many 0-order free pages located in CMA. Those pages are subtracted twice therefore they will quickly drain free_pages during the check against fragmentation. The test fails even though there are many free non-cma pages in the zone.

This patch fixes this issue by subtracting CMA pages only for a purpose of (free_pages <= min + lowmem_reserve) check.

Signed-off-by Tomasz Stanislawski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 mm/page_alloc.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/page_alloc.c +4 -2

index 8fcced7..0d4fef2 100644
@@ -1626,6 +1626,7 @@ static bool __zone_watermark_ok(struct zone *z, int order, unsigned long mark,
 	long min = mark;
 	long lowmem_reserve = z->lowmem_reserve[classzone_idx];
 	int o;
+	long free_cma = 0;
 
 	free_pages -= (1 << order) - 1;
 	if (alloc_flags & ALLOC_HIGH)

@@ -1635,9 +1636,10 @@ static bool __zone_watermark_ok(struct zone *z, int order, unsigned long mark,
 #ifdef CONFIG_CMA
 	/* If allocation can't use CMA areas don't use free CMA pages */
 	if (!(alloc_flags & ALLOC_CMA))
-		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
+		free_cma = zone_page_state(z, NR_FREE_CMA_PAGES);
 #endif
-	if (free_pages <= min + lowmem_reserve)
+
+	if (free_pages - free_cma <= min + lowmem_reserve)
 		return false;
 	for (o = 0; o < order; o++) {
 		/* At the next order, this order's pages become unavailable */

Signature · 7 lines

1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Laura Abbott

Tested-by on v1 1/1

2013-05-24 20:23 UTC #1 lore raw

Quoted 58 lines · Tomasz Stanislawski

The watermark check consists of two sub-checks.
The first one is:
	if (free_pages <= min + lowmem_reserve)
		return false;
The check assures that there is minimal amount of RAM in the zone. If CMA is used then the free_pages is reduced by the number of free pages in CMA prior to the over-mentioned check.
	if (!(alloc_flags & ALLOC_CMA))
		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
This prevents the zone from being drained from pages available for non-movable allocations.
The second check prevents the zone from getting too fragmented.
	for (o = 0; o < order; o++) {
		free_pages -= z->free_area[o].nr_free << o;
		min >>= 1;
		if (free_pages <= min)
			return false;
	}
The field z->free_area[o].nr_free is equal to the number of free pages including free CMA pages. Therefore the CMA pages are subtracted twice. This may cause a false positive fail of __zone_watermark_ok() if the CMA area gets strongly fragmented. In such a case there are many 0-order free pages located in CMA. Those pages are subtracted twice therefore they will quickly drain free_pages during the check against fragmentation. The test fails even though there are many free non-cma pages in the zone.
This patch fixes this issue by subtracting CMA pages only for a purpose of (free_pages <= min + lowmem_reserve) check.
Signed-off-by Tomasz Stanislawski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
  mm/page_alloc.c |    6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)
mm/page_alloc.c +1 -0
index 8fcced7..0d4fef2 100644
@@ -1626,6 +1626,7 @@ static bool __zone_watermark_ok(struct zone *z, int order, unsigned long mark,
  	long min = mark;
  	long lowmem_reserve = z->lowmem_reserve[classzone_idx];
  	int o;
+	long free_cma = 0;
 
  	free_pages -= (1 << order) - 1;
  	if (alloc_flags & ALLOC_HIGH)
@@ -1635,9 +1636,10 @@ static bool __zone_watermark_ok(struct zone *z, int order, unsigned long mark,
  #ifdef CONFIG_CMA
  	/* If allocation can't use CMA areas don't use free CMA pages */

mm/page_alloc.c static bool __zone_watermark_ok(struct zone *z, int order, unsigned long mark, · line 1645

Quoted 10 lines

mm/page_alloc.c +3 -2

  	if (!(alloc_flags & ALLOC_CMA))
-		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
+		free_cma = zone_page_state(z, NR_FREE_CMA_PAGES);
  #endif
-	if (free_pages <= min + lowmem_reserve)
+
+	if (free_pages - free_cma <= min + lowmem_reserve)
  		return false;
  	for (o = 0; o < order; o++) {
  		/* At the next order, this order's pages become unavailable */

I haven't seen any response to this patch but it has been of some benefit to some of our use cases. You're welcome to add

Tested-by Laura Abbott <[email protected]>

if the patch hasn't been picked up yet.

Thanks,
Laura

Signature · 8 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kyungmin Park

2013-05-25 04:32 UTC #2 lore raw

Quoted 84 lines · Laura Abbott

Quoted 76 lines · Tomasz Stanislawski
The watermark check consists of two sub-checks.
The first one is:
if (free_pages <= min + lowmem_reserve)
return false;
The check assures that there is minimal amount of RAM in the zone. If CMA is
used then the free_pages is reduced by the number of free pages in CMA prior
to the over-mentioned check.
        if (!(alloc_flags & ALLOC_CMA))
                free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
This prevents the zone from being drained from pages available for non-movable
allocations.
The second check prevents the zone from getting too fragmented.
        for (o = 0; o < order; o++) {
                free_pages -= z->free_area[o].nr_free << o;
                min >>= 1;
                if (free_pages <= min)
                        return false;
        }
The field z->free_area[o].nr_free is equal to the number of free pages including free CMA pages. Therefore the CMA pages are subtracted twice.
This
may cause a false positive fail of __zone_watermark_ok() if the CMA area gets
strongly fragmented. In such a case there are many 0-order free pages located
in CMA. Those pages are subtracted twice therefore they will quickly drain free_pages during the check against fragmentation. The test fails even though
there are many free non-cma pages in the zone.
This patch fixes this issue by subtracting CMA pages only for a purpose of (free_pages <= min + lowmem_reserve) check.
Signed-off-by Tomasz Stanislawski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
  mm/page_alloc.c |    6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)
mm/page_alloc.c +1 -0
index 8fcced7..0d4fef2 100644
@@ -1626,6 +1626,7 @@ static bool __zone_watermark_ok(struct zone *z, int
 order, unsigned long mark,
        long min = mark;
        long lowmem_reserve = z->lowmem_reserve[classzone_**idx];
        int o;
+       long free_cma = 0;
 
        free_pages -= (1 << order) - 1;
if (alloc_flags & ALLOC_HIGH)
mm/page_alloc.c +3 -2
@@ -1635,9 +1636,10 @@ static bool __zone_watermark_ok(struct zone *z,
 int order, unsigned long mark,
  #ifdef CONFIG_CMA
        /* If allocation can't use CMA areas don't use free CMA pages */
        if (!(alloc_flags & ALLOC_CMA))
-               free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
+               free_cma = zone_page_state(z, NR_FREE_CMA_PAGES);
  #endif
-       if (free_pages <= min + lowmem_reserve)
+
+       if (free_pages - free_cma <= min + lowmem_reserve)
                return false;
        for (o = 0; o < order; o++) {
                /* At the next order, this order's pages become
Quoted 3 lines
unavailable */
I haven't seen any response to this patch but it has been of some benefit to some of our use cases. You're welcome to add

Quoted 2 lines

Tested-by Laura Abbott <[email protected]>

Thanks Laura,
We already got mail from Andrew, it's merged mm tree.

Thank you,
Kyungmin Park

Quoted 4 lines

if the patch hasn't been picked up yet.

Andrew Morton

maintainer

2013-05-29 22:08 UTC #3 lore raw

Quoted 8 lines · On Sat, 25 May 2013 13:32:02 +0900 Kyungmin Park <[email protected]> wrote:

Quoted 5 lines
I haven't seen any response to this patch but it has been of some benefit to some of our use cases. You're welcome to add
Tested-by Laura Abbott <[email protected]>
Thanks Laura,
We already got mail from Andrew, it's merged mm tree.

Yes, but I have it scheduled for 3.11 with no -stable backport.

This is because the patch changelog didn't tell me about the userspace-visible impact of the bug. Judging from Laura's comments, this was a mistake.

So please: details. What problems were observable to Laura and do we think this bug should be fixed in 3.10 and earlier?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Marek Szyprowski

2013-06-03 07:15 UTC #4 lore raw

Hello,

Quoted 11 lines · Andrew Morton

Quoted 8 lines · On Sat, 25 May 2013 13:32:02 +0900 Kyungmin Park <[email protected]> wrote:
Quoted 5 lines
I haven't seen any response to this patch but it has been of some benefit to some of our use cases. You're welcome to add
Tested-by Laura Abbott <[email protected]>
Thanks Laura,
We already got mail from Andrew, it's merged mm tree.

Quoted 8 lines

Yes, but I have it scheduled for 3.11 with no -stable backport.
This is because the patch changelog didn't tell me about the userspace-visible impact of the bug. Judging from Laura's comments, this was a mistake.
So please: details. What problems were observable to Laura and do we think this bug should be fixed in 3.10 and earlier?

This patch fixes issue introduced in commit d95ea5d18e699515468368415c93ed49b1a3221b, so if possible, I suggest to get it backported to v3.7-v3.10 releases as well.

Best regards

Signature · 9 lines

Marek Szyprowski
Samsung R&D Institute Poland


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Laura Abbott

2013-06-04 02:35 UTC #5 lore raw

In reply to Andrew Morton

Quoted 11 lines · Andrew Morton

Quoted 8 lines · On Sat, 25 May 2013 13:32:02 +0900 Kyungmin Park <[email protected]> wrote:
Quoted 5 lines
I haven't seen any response to this patch but it has been of some benefit to some of our use cases. You're welcome to add
Tested-by Laura Abbott <[email protected]>
Thanks Laura,
We already got mail from Andrew, it's merged mm tree.

Quoted 9 lines

Yes, but I have it scheduled for 3.11 with no -stable backport.
This is because the patch changelog didn't tell me about the userspace-visible impact of the bug. Judging from Laura's comments, this was a mistake.
So please: details. What problems were observable to Laura and do we think this bug should be fixed in 3.10 and earlier?

We were observing allocation failures of higher order pages (order 5 = 128K typically) under tight memory conditions resulting in driver failure. The output from the page allocation failure showed plenty of free pages of the appropriate order/type/zone and mostly CMA pages in the lower orders.

For full disclosure, we still observed some page allocation failures even after applying the patch but the number was drastically reduced and those failures were attributed to fragmentation/other system issues.

Thanks,
Laura

Signature · 8 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

Acked-by on v1 1/1

2013-06-04 07:11 UTC #6 lore raw

In reply to Tomasz Stanislawski

Hello,

Quoted 10 lines · Tomasz Stanislawski

The watermark check consists of two sub-checks.
The first one is:
	if (free_pages <= min + lowmem_reserve)
		return false;
The check assures that there is minimal amount of RAM in the zone. If CMA is used then the free_pages is reduced by the number of free pages in CMA prior to the over-mentioned check.

Quoted 5 lines

	if (!(alloc_flags & ALLOC_CMA))
		free_pages -= zone_page_state(z, NR_FREE_CMA_PAGES);
This prevents the zone from being drained from pages available for non-movable allocations.

The description is rather confusing.
It was not to prevent the zone from being drained but for considering a right number of allocatable pages from the zone because !ALLOC_CMA pages couldn't be allocated from CMA migrate type from the zone so if we didn't have such patch, zone_watermark_ok could return true but allocation couldn't allocate a page.

Quoted 10 lines

The second check prevents the zone from getting too fragmented.
	for (o = 0; o < order; o++) {
		free_pages -= z->free_area[o].nr_free << o;
		min >>= 1;
		if (free_pages <= min)
			return false;
	}

Quoted 2 lines

The field z->free_area[o].nr_free is equal to the number of free pages including free CMA pages. Therefore the CMA pages are subtracted twice. This

Right.

Quoted 6 lines

may cause a false positive fail of __zone_watermark_ok() if the CMA area gets strongly fragmented. In such a case there are many 0-order free pages located in CMA. Those pages are subtracted twice therefore they will quickly drain free_pages during the check against fragmentation. The test fails even though
there are many free non-cma pages in the zone.

How about this?

The !ALLOC_CMA higher order allocation fails even though there are many free high order pages in the zone.

This patch should go to stable so I'd like to include description with detail numbers of zone's stat to describe the situation when the higher allocation is failed so stable maintainers and other distro maintainers could parse the problem easily.

Quoted 8 lines

This patch fixes this issue by subtracting CMA pages only for a purpose of (free_pages <= min + lowmem_reserve) check.
Signed-off-by Tomasz Stanislawski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>

Otherwise, looks good to me.

Acked-by Minchan Kim <[email protected]>

Signature · 8 lines

Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>