Accounting problem of MIGRATE_ISOLATED freed page

Quoted 48 lines · Minchan Kim

Quoted 44 lines · Aaditya Kumar

Quoted 14 lines · Minchan Kim

Quoted 8 lines

pgdat_balanced() doesn't recognized zone. Therefore kswapd may sleep if node has multiple zones. Hm ok, I realized my descriptions was slightly misleading. priority 0 is not needed. bakance_pddat() calls pgdat_balanced()
every priority. Most easy case is, movable zone has a lot of free pages and normal zone has no reclaimable page.

Quoted 4 lines

btw, current pgdat_balanced() logic seems not correct. kswapd should sleep only if every zones have much free pages than high water mark _and_ 25% of present pages in node are free.

Quoted 4 lines

Sorry. I can't understand your point.
Current kswapd doesn't sleep if relevant zones don't have free pages above high watermark. It seems I am missing your point.
Please anybody correct me.

Since currently direct reclaim is given up based on zone->all_unreclaimable flag,
so for e.g in one of the scenarios:

Lets say system has one node with two zones (NORMAL and MOVABLE) and we hot-remove the all the pages of the MOVABLE zone.

While migrating pages during memory hot-unplugging, the allocation function (for new page to which the page in MOVABLE zone would be moved) can end up looping in direct reclaim path for ever.

This is so because when most of the pages in the MOVABLE zone have been migrated,
the zone now contains lots of free memory (basically above low watermark) BUT all are in MIGRATE_ISOLATE list of the buddy list.

So kswapd() would not balance this zone as free pages are above low watermark (but all are in isolate list). So zone->all_unreclaimable flag would never be set for this zone
and allocation function would end up looping forever. (assuming the zone NORMAL is
left with no reclaimable memory)

Quoted 2 lines

Thanks a lot, Aaditya! Scenario you mentioned makes perfect. But I don't see it's a problem of kswapd.

Hi Kim,

Yes I agree it is not a problem of kswapd.

Quoted 8 lines

a5d76b54 made new migration type 'MIGRATE_ISOLATE' which is very irony type because there are many free pages in free list but we can't allocate it. :(
It doesn't reflect right NR_FREE_PAGES while many places in the kernel use NR_FREE_PAGES to trigger some operation. Kswapd is just one of them confused.
As right fix of this problem, we should fix hot plug code, IMHO which can fix CMA, too.

This patch could make inconsistency between NR_FREE_PAGES and SumOf[free_area[order].nr_free]

I assume that by the inconsistency you mention above, you mean temporary inconsistency.

Sorry, but IMHO as for memory hot plug the main issue with this patch is that the inconsistency you mentioned above would NOT be a temporary inconsistency.

Every time say 'x' number of page frames are off lined, they will introduce a difference of 'x' pages between NR_FREE_PAGES and SumOf[free_area[order].nr_free].
(So for e.g. if we do a frequent offline/online it will make NR_FREE_PAGES negative)

This is so because, unset_migratetype_isolate() is called from offlining code (to set the migrate type of off lined pages again back to MIGRATE_MOVABLE)
after the pages have been off lined and removed from the buddy list. Since the pages for which unset_migratetype_isolate() is called are not buddy pages so move_freepages_block() does not move any page, and thus introducing a permanent inconsistency.

Quoted 5 lines

and it could make __zone_watermark_ok confuse so we might need to fix move_freepages_block itself to reflect free_area[order].nr_free exactly.

Any thought?

As for fixing move_freepages_block(), At least for memory hot plug, the pages stay in MIGRATE_ISOLATE list only for duration offline_pages() function,
I mean only temporarily. Since fixing move_freepages_block() for will introduce some overhead, So I am not very sure whether that overhead is justified
for a temporary condition. What do you think?

Hi Aaditya,

I want to discuss this problem on another thread.

Quoted 49 lines · Aaditya Kumar

Quoted 48 lines · Minchan Kim

Quoted 44 lines · Aaditya Kumar

Quoted 14 lines · Minchan Kim

Quoted 8 lines

pgdat_balanced() doesn't recognized zone. Therefore kswapd may sleep if node has multiple zones. Hm ok, I realized my descriptions was slightly misleading. priority 0 is not needed. bakance_pddat() calls pgdat_balanced()
every priority. Most easy case is, movable zone has a lot of free pages and normal zone has no reclaimable page.

Quoted 4 lines

btw, current pgdat_balanced() logic seems not correct. kswapd should sleep only if every zones have much free pages than high water mark _and_ 25% of present pages in node are free.

Quoted 4 lines

Sorry. I can't understand your point.
Current kswapd doesn't sleep if relevant zones don't have free pages above high watermark. It seems I am missing your point.
Please anybody correct me.

Since currently direct reclaim is given up based on zone->all_unreclaimable flag,
so for e.g in one of the scenarios:

Lets say system has one node with two zones (NORMAL and MOVABLE) and we hot-remove the all the pages of the MOVABLE zone.

While migrating pages during memory hot-unplugging, the allocation function (for new page to which the page in MOVABLE zone would be moved) can end up looping in direct reclaim path for ever.

This is so because when most of the pages in the MOVABLE zone have been migrated,
the zone now contains lots of free memory (basically above low watermark) BUT all are in MIGRATE_ISOLATE list of the buddy list.

So kswapd() would not balance this zone as free pages are above low watermark (but all are in isolate list). So zone->all_unreclaimable flag would never be set for this zone
and allocation function would end up looping forever. (assuming the zone NORMAL is
left with no reclaimable memory)

Quoted 4 lines

Quoted 2 lines

Thanks a lot, Aaditya! Scenario you mentioned makes perfect. But I don't see it's a problem of kswapd.

Hi Kim,

I like called Minchan rather than Kim
Never mind. :)

Quoted 2 lines

Yes I agree it is not a problem of kswapd.

Yeb.

Quoted 23 lines

Quoted 8 lines

a5d76b54 made new migration type 'MIGRATE_ISOLATE' which is very irony type because there are many free pages in free list but we can't allocate it. :(
It doesn't reflect right NR_FREE_PAGES while many places in the kernel use NR_FREE_PAGES to trigger some operation. Kswapd is just one of them confused.
As right fix of this problem, we should fix hot plug code, IMHO which can fix CMA, too.

This patch could make inconsistency between NR_FREE_PAGES and SumOf[free_area[order].nr_free]

I assume that by the inconsistency you mention above, you mean temporary inconsistency.

Sorry, but IMHO as for memory hot plug the main issue with this patch is that the inconsistency you mentioned above would NOT be a temporary inconsistency.

Every time say 'x' number of page frames are off lined, they will introduce a difference of 'x' pages between NR_FREE_PAGES and SumOf[free_area[order].nr_free].
(So for e.g. if we do a frequent offline/online it will make NR_FREE_PAGES negative)

Quoted 7 lines

This is so because, unset_migratetype_isolate() is called from offlining code (to set the migrate type of off lined pages again back to MIGRATE_MOVABLE)
after the pages have been off lined and removed from the buddy list. Since the pages for which unset_migratetype_isolate() is called are not buddy pages so move_freepages_block() does not move any page, and thus introducing a permanent inconsistency.

Good point. Negative NR_FREE_PAGES is caused by double counting by my patch and __offline_isolated_pages. I think at first MIGRATE_ISOLATE type freed page shouldn't account as free page.

Quoted 6 lines

Quoted 5 lines

and it could make __zone_watermark_ok confuse so we might need to fix move_freepages_block itself to reflect free_area[order].nr_free exactly.

Any thought?

Quoted 7 lines

As for fixing move_freepages_block(), At least for memory hot plug, the pages stay in MIGRATE_ISOLATE list only for duration offline_pages() function,
I mean only temporarily. Since fixing move_freepages_block() for will introduce some overhead, So I am not very sure whether that overhead is justified
for a temporary condition. What do you think?

Yes. I don't like hurt fast path, either.
How about this? (Passed just compile test :( )
The patch's goal is to NOT increase nr_free and NR_FREE_PAGES about freed page into MIGRATE_ISOLATED.

This patch hurts high order page free path but I think it's not critical because higher order allocation is rare than order-0 allocation and we already have done same thing on free_hot_cold_page on order-0 free path which is more hot.

Quoted 120 lines · Minchan Kim

Hi Aaditya,

I want to discuss this problem on another thread.

Quoted 49 lines · Aaditya Kumar

Quoted 48 lines · Minchan Kim

Quoted 44 lines · Aaditya Kumar

Quoted 14 lines · Minchan Kim

Quoted 8 lines

pgdat_balanced() doesn't recognized zone. Therefore kswapd may sleep if node has multiple zones. Hm ok, I realized my descriptions was slightly misleading. priority 0 is not needed. bakance_pddat() calls pgdat_balanced()
every priority. Most easy case is, movable zone has a lot of free pages and normal zone has no reclaimable page.

Quoted 4 lines

btw, current pgdat_balanced() logic seems not correct. kswapd should sleep only if every zones have much free pages than high water mark _and_ 25% of present pages in node are free.

Quoted 4 lines

Sorry. I can't understand your point.
Current kswapd doesn't sleep if relevant zones don't have free pages above high watermark. It seems I am missing your point.
Please anybody correct me.

Since currently direct reclaim is given up based on zone->all_unreclaimable flag,
so for e.g in one of the scenarios:

Lets say system has one node with two zones (NORMAL and MOVABLE) and we hot-remove the all the pages of the MOVABLE zone.

While migrating pages during memory hot-unplugging, the allocation function (for new page to which the page in MOVABLE zone would be moved) can end up looping in direct reclaim path for ever.

This is so because when most of the pages in the MOVABLE zone have been migrated,
the zone now contains lots of free memory (basically above low watermark) BUT all are in MIGRATE_ISOLATE list of the buddy list.

So kswapd() would not balance this zone as free pages are above low watermark (but all are in isolate list). So zone->all_unreclaimable flag would never be set for this zone
and allocation function would end up looping forever. (assuming the zone NORMAL is
left with no reclaimable memory)

Quoted 4 lines

Quoted 2 lines

Thanks a lot, Aaditya! Scenario you mentioned makes perfect. But I don't see it's a problem of kswapd.

Hi Kim,

I like called Minchan rather than Kim
Never mind. :)

Quoted 2 lines

Yes I agree it is not a problem of kswapd.

Yeb.

Quoted 23 lines

Quoted 8 lines

a5d76b54 made new migration type 'MIGRATE_ISOLATE' which is very irony type because there are many free pages in free list but we can't allocate it. :(
It doesn't reflect right NR_FREE_PAGES while many places in the kernel use NR_FREE_PAGES to trigger some operation. Kswapd is just one of them confused.
As right fix of this problem, we should fix hot plug code, IMHO which can fix CMA, too.

This patch could make inconsistency between NR_FREE_PAGES and SumOf[free_area[order].nr_free]

I assume that by the inconsistency you mention above, you mean temporary inconsistency.

Sorry, but IMHO as for memory hot plug the main issue with this patch is that the inconsistency you mentioned above would NOT be a temporary inconsistency.

Every time say 'x' number of page frames are off lined, they will introduce a difference of 'x' pages between NR_FREE_PAGES and SumOf[free_area[order].nr_free].
(So for e.g. if we do a frequent offline/online it will make NR_FREE_PAGES negative)

Quoted 7 lines

This is so because, unset_migratetype_isolate() is called from offlining code (to set the migrate type of off lined pages again back to MIGRATE_MOVABLE)
after the pages have been off lined and removed from the buddy list. Since the pages for which unset_migratetype_isolate() is called are not buddy pages so move_freepages_block() does not move any page, and thus introducing a permanent inconsistency.

Good point. Negative NR_FREE_PAGES is caused by double counting by my patch and __offline_isolated_pages. I think at first MIGRATE_ISOLATE type freed page shouldn't account as free page.

Quoted 6 lines

Quoted 5 lines

and it could make __zone_watermark_ok confuse so we might need to fix move_freepages_block itself to reflect free_area[order].nr_free exactly.

Any thought?

Quoted 7 lines

As for fixing move_freepages_block(), At least for memory hot plug, the pages stay in MIGRATE_ISOLATE list only for duration offline_pages() function,
I mean only temporarily. Since fixing move_freepages_block() for will introduce some overhead, So I am not very sure whether that overhead is justified
for a temporary condition. What do you think?

Yes. I don't like hurt fast path, either.
How about this? (Passed just compile test :( )
The patch's goal is to NOT increase nr_free and NR_FREE_PAGES about freed page into MIGRATE_ISOLATED.

Quoted 3 lines

This patch hurts high order page free path but I think it's not critical because higher order allocation is rare than order-0 allocation and we already have done same thing on free_hot_cold_page on order-0 free path which is more hot.

Can't we change zone_water_mark_ok_safe() instead of page allocator? memory hotplug is really rare event.

--
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>

Quoted 121 lines · KOSAKI Motohiro

Quoted 120 lines · Minchan Kim

Hi Aaditya,

I want to discuss this problem on another thread.

Quoted 49 lines · Aaditya Kumar

Quoted 48 lines · Minchan Kim

Quoted 44 lines · Aaditya Kumar

Quoted 14 lines · Minchan Kim

Quoted 8 lines

pgdat_balanced() doesn't recognized zone. Therefore kswapd may sleep if node has multiple zones. Hm ok, I realized my descriptions was slightly misleading. priority 0 is not needed. bakance_pddat() calls pgdat_balanced()
every priority. Most easy case is, movable zone has a lot of free pages and normal zone has no reclaimable page.

Quoted 4 lines

btw, current pgdat_balanced() logic seems not correct. kswapd should sleep only if every zones have much free pages than high water mark _and_ 25% of present pages in node are free.

Quoted 4 lines

Sorry. I can't understand your point.
Current kswapd doesn't sleep if relevant zones don't have free pages above high watermark. It seems I am missing your point.
Please anybody correct me.

Since currently direct reclaim is given up based on zone->all_unreclaimable flag,
so for e.g in one of the scenarios:

Lets say system has one node with two zones (NORMAL and MOVABLE) and we hot-remove the all the pages of the MOVABLE zone.

While migrating pages during memory hot-unplugging, the allocation function (for new page to which the page in MOVABLE zone would be moved)  can end up looping in direct reclaim path for ever.

This is so because when most of the pages in the MOVABLE zone have been migrated,
the zone now contains lots of free memory (basically above low watermark) BUT all are in MIGRATE_ISOLATE list of the buddy list.

So kswapd() would not balance this zone as free pages are above low watermark (but all are in isolate list). So zone->all_unreclaimable flag would never be set for this zone
and allocation function would end up looping forever. (assuming the zone NORMAL is
left with no reclaimable memory)

Quoted 4 lines

Quoted 2 lines

Thanks a lot, Aaditya! Scenario you mentioned makes perfect. But I don't see it's a problem of kswapd.

Hi Kim,

I like called Minchan rather than Kim
Never mind. :)

Quoted 2 lines

Yes I agree it is not a problem of kswapd.

Yeb.

Quoted 23 lines

Quoted 8 lines

a5d76b54 made new migration type 'MIGRATE_ISOLATE' which is very irony type because there are many free pages in free list but we can't allocate it. :(
It doesn't reflect right NR_FREE_PAGES while many places in the kernel use NR_FREE_PAGES to trigger some operation. Kswapd is just one of them confused.
As right fix of this problem, we should fix hot plug code, IMHO which can fix CMA, too.

This patch could make inconsistency between NR_FREE_PAGES and SumOf[free_area[order].nr_free]

I assume that by the inconsistency you mention above, you mean temporary inconsistency.

Sorry, but IMHO as for memory hot plug the main issue with this patch is that the inconsistency you mentioned above would NOT be a temporary inconsistency.

Every time say 'x' number of page frames are off lined, they will introduce a difference of 'x' pages between NR_FREE_PAGES and SumOf[free_area[order].nr_free].
(So for e.g. if we do a frequent offline/online it will make NR_FREE_PAGES  negative)

Quoted 7 lines

This is so because, unset_migratetype_isolate() is called from offlining  code (to set the migrate type of off lined pages again back to MIGRATE_MOVABLE)
after the pages have been off lined and removed from the buddy list. Since the pages for which unset_migratetype_isolate() is called are not buddy pages so move_freepages_block() does not move any page, and thus introducing a permanent inconsistency.

Good point. Negative NR_FREE_PAGES is caused by double counting by my patch and __offline_isolated_pages. I think at first MIGRATE_ISOLATE type freed page shouldn't account as free page.

Quoted 6 lines

Quoted 5 lines

and it could make __zone_watermark_ok confuse so we might need to fix move_freepages_block itself to reflect free_area[order].nr_free exactly.

Any thought?

Quoted 7 lines

As for fixing move_freepages_block(), At least for memory hot plug, the pages stay in MIGRATE_ISOLATE list only for duration offline_pages() function,
I mean only temporarily. Since fixing move_freepages_block() for will introduce some overhead, So I am not very sure whether that overhead is justified
for a temporary condition. What do you think?

Yes. I don't like hurt fast path, either.
How about this? (Passed just compile test :(  )
The patch's goal is to NOT increase nr_free and NR_FREE_PAGES about freed page into MIGRATE_ISOLATED.

Quoted 5 lines

Quoted 3 lines

This patch hurts high order page free path but I think it's not critical because higher order allocation is rare than order-0 allocation and we already have done same thing on free_hot_cold_page on order-0 free path which is more hot.

Can't we change zone_water_mark_ok_safe() instead of page allocator? memory hotplug is really rare event.

+1

Firstly, I want to make zone_page_state(z, NR_FREE_PAGES) itself more accurately because it is used by several places. As I looked over places, I can't find critical places except kswapd forever sleep case. So it's a nice idea!

In that case, we need zone->lock whenever zone_watermark_ok_safe is called.

Most of cases, it's unnecessary and it might hurt alloc/free performance when memory pressure is high. But if memory pressure is high, it may be already meaningless alloc/free performance. So it does make sense, IMHO.

Please raise your hands if anyone has a concern about this.

barrios@bbox:~/linux-next$ git diff

mm/page_alloc.c +17 -3

index d2a515d..82cc0a2 100644
@@ -1748,16 +1748,38 @@ bool zone_watermark_ok(struct zone *z, int order, unsigned long mark,
                                        zone_page_state(z, NR_FREE_PAGES));
  }
 
-bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long mark,
+bool zone_watermark_ok_safe(struct zone *z, int alloc_order, unsigned long mark,
                      int classzone_idx, int alloc_flags)
  {
+       struct free_area *area;
+       struct list_head *curr;
+       int order;
+       unsigned long flags;
        long free_pages = zone_page_state(z, NR_FREE_PAGES);
 
        if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
                free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
-       return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
-                                                               free_pages);
+       /*
+        * Memory hotplug/CMA can isolate freed page into MIGRATE_ISOLATE
+        * so that buddy can't allocate it although they are in free list.
+        */
+       spin_lock_irqsave(&z->lock, flags);
+       for (order = 0; order < MAX_ORDER; order++) {
+               int count = 0;
+               area = &(z->free_area[order]);
+               if (unlikely(!list_empty(&area->free_list[MIGRATE_ISOLATE]))) {
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+                       free_pages -= (count << order);

Quoted 132 lines · Minchan Kim

Quoted 121 lines · KOSAKI Motohiro

Quoted 120 lines · Minchan Kim

Hi Aaditya,

I want to discuss this problem on another thread.

Quoted 49 lines · Aaditya Kumar

Quoted 48 lines · Minchan Kim

Quoted 44 lines · Aaditya Kumar

Quoted 14 lines · Minchan Kim

Quoted 8 lines

pgdat_balanced() doesn't recognized zone. Therefore kswapd may sleep if node has multiple zones. Hm ok, I realized my descriptions was slightly misleading. priority 0 is not needed. bakance_pddat() calls pgdat_balanced()
every priority. Most easy case is, movable zone has a lot of free pages and normal zone has no reclaimable page.

Quoted 4 lines

btw, current pgdat_balanced() logic seems not correct. kswapd should sleep only if every zones have much free pages than high water mark _and_ 25% of present pages in node are free.

Quoted 4 lines

Sorry. I can't understand your point.
Current kswapd doesn't sleep if relevant zones don't have free pages above high watermark. It seems I am missing your point.
Please anybody correct me.

Since currently direct reclaim is given up based on zone->all_unreclaimable flag,
so for e.g in one of the scenarios:

Lets say system has one node with two zones (NORMAL and MOVABLE) and we hot-remove the all the pages of the MOVABLE zone.

While migrating pages during memory hot-unplugging, the allocation function (for new page to which the page in MOVABLE zone would be moved) can end up looping in direct reclaim path for ever.

This is so because when most of the pages in the MOVABLE zone have been migrated,
the zone now contains lots of free memory (basically above low watermark) BUT all are in MIGRATE_ISOLATE list of the buddy list.

So kswapd() would not balance this zone as free pages are above low watermark (but all are in isolate list). So zone->all_unreclaimable flag would never be set for this zone
and allocation function would end up looping forever. (assuming the zone NORMAL is
left with no reclaimable memory)

Quoted 4 lines

Quoted 2 lines

Thanks a lot, Aaditya! Scenario you mentioned makes perfect. But I don't see it's a problem of kswapd.

Hi Kim,

I like called Minchan rather than Kim
Never mind. :)

Quoted 2 lines

Yes I agree it is not a problem of kswapd.

Yeb.

Quoted 23 lines

Quoted 8 lines

a5d76b54 made new migration type 'MIGRATE_ISOLATE' which is very irony type because there are many free pages in free list but we can't allocate it. :(
It doesn't reflect right NR_FREE_PAGES while many places in the kernel use NR_FREE_PAGES to trigger some operation. Kswapd is just one of them confused.
As right fix of this problem, we should fix hot plug code, IMHO which can fix CMA, too.

This patch could make inconsistency between NR_FREE_PAGES and SumOf[free_area[order].nr_free]

I assume that by the inconsistency you mention above, you mean temporary inconsistency.

Sorry, but IMHO as for memory hot plug the main issue with this patch is that the inconsistency you mentioned above would NOT be a temporary inconsistency.

Every time say 'x' number of page frames are off lined, they will introduce a difference of 'x' pages between NR_FREE_PAGES and SumOf[free_area[order].nr_free].
(So for e.g. if we do a frequent offline/online it will make NR_FREE_PAGES negative)

Quoted 7 lines

This is so because, unset_migratetype_isolate() is called from offlining code (to set the migrate type of off lined pages again back to MIGRATE_MOVABLE)
after the pages have been off lined and removed from the buddy list. Since the pages for which unset_migratetype_isolate() is called are not buddy pages so move_freepages_block() does not move any page, and thus introducing a permanent inconsistency.

Good point. Negative NR_FREE_PAGES is caused by double counting by my patch and __offline_isolated_pages. I think at first MIGRATE_ISOLATE type freed page shouldn't account as free page.

Quoted 6 lines

Quoted 5 lines

and it could make __zone_watermark_ok confuse so we might need to fix move_freepages_block itself to reflect free_area[order].nr_free exactly.

Any thought?

Quoted 7 lines

As for fixing move_freepages_block(), At least for memory hot plug, the pages stay in MIGRATE_ISOLATE list only for duration offline_pages() function,
I mean only temporarily. Since fixing move_freepages_block() for will introduce some overhead, So I am not very sure whether that overhead is justified
for a temporary condition. What do you think?

Yes. I don't like hurt fast path, either.
How about this? (Passed just compile test :( )
The patch's goal is to NOT increase nr_free and NR_FREE_PAGES about freed page into MIGRATE_ISOLATED.

Quoted 5 lines

Quoted 3 lines

This patch hurts high order page free path but I think it's not critical because higher order allocation is rare than order-0 allocation and we already have done same thing on free_hot_cold_page on order-0 free path which is more hot.

Can't we change zone_water_mark_ok_safe() instead of page allocator? memory hotplug is really rare event.

+1

Quoted 6 lines

Firstly, I want to make zone_page_state(z, NR_FREE_PAGES) itself more accurately because it is used by several places. As I looked over places, I can't find critical places except kswapd forever sleep case. So it's a nice idea!

In that case, we need zone->lock whenever zone_watermark_ok_safe is called.

Ifdefinery could be utilized for builds with CMA disabled, first.

Quoted 43 lines

Most of cases, it's unnecessary and it might hurt alloc/free performance when memory pressure is high. But if memory pressure is high, it may be already meaningless alloc/free performance. So it does make sense, IMHO.

Please raise your hands if anyone has a concern about this.

barrios@bbox:~/linux-next$ git diff

mm/page_alloc.c +17 -3

index d2a515d..82cc0a2 100644
@@ -1748,16 +1748,38 @@ bool zone_watermark_ok(struct zone *z, int order, unsigned long mark,
                                       zone_page_state(z, NR_FREE_PAGES));
 }
 
-bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long mark,
+bool zone_watermark_ok_safe(struct zone *z, int alloc_order, unsigned long mark,
                     int classzone_idx, int alloc_flags)
 {
+       struct free_area *area;
+       struct list_head *curr;
+       int order;
+       unsigned long flags;
       long free_pages = zone_page_state(z, NR_FREE_PAGES);
 
       if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
               free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
-       return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
-                                                               free_pages);
+       /*
+        * Memory hotplug/CMA can isolate freed page into MIGRATE_ISOLATE
+        * so that buddy can't allocate it although they are in free list.
+        */
+       spin_lock_irqsave(&z->lock, flags);
+       for (order = 0; order < MAX_ORDER; order++) {
+               int count = 0;
+               area = &(z->free_area[order]);
+               if (unlikely(!list_empty(&area->free_list[MIGRATE_ISOLATE]))) {
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+                       free_pages -= (count << order);

mm/page_alloc.c bool zone_watermark_ok(struct zone *z, int order, unsigned long mark, · line 1784

Quoted 10 lines

mm/page_alloc.c +8 -0

+               }
+       }
+       if (free_pages < 0)
+               free_pages = 0;
+       spin_unlock_irqrestore(&z->lock, flags);
+
+       return __zone_watermark_ok(z, alloc_order, mark,
+                               classzone_idx, alloc_flags, free_pages);
 }
 

Then isolated pages could be scanned in another direction?

	spin_lock_irqsave(&z->lock, flags);
	for (order = MAX_ORDER - 1; order >= 0; order--) {
		struct free_area *area = &z->free_area[order];
		long count = 0;
		struct list_head *curr;

		list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
			count++;

		free_pages -= (count << order);
		if (free_pages < 0) {
			free_pages = 0;
			break;
		}
	}
	spin_unlock_irqrestore(&z->lock, flags);

Quoted 121 lines · KOSAKI Motohiro

Quoted 120 lines · Minchan Kim

Hi Aaditya,

I want to discuss this problem on another thread.

Quoted 49 lines · Aaditya Kumar

Quoted 48 lines · Minchan Kim

Quoted 44 lines · Aaditya Kumar

Quoted 14 lines · Minchan Kim

Quoted 8 lines

pgdat_balanced() doesn't recognized zone. Therefore kswapd may sleep if node has multiple zones. Hm ok, I realized my descriptions was slightly misleading. priority 0 is not needed. bakance_pddat() calls pgdat_balanced()
every priority. Most easy case is, movable zone has a lot of free pages and normal zone has no reclaimable page.

Quoted 4 lines

btw, current pgdat_balanced() logic seems not correct. kswapd should sleep only if every zones have much free pages than high water mark _and_ 25% of present pages in node are free.

Quoted 4 lines

Sorry. I can't understand your point.
Current kswapd doesn't sleep if relevant zones don't have free pages above high watermark. It seems I am missing your point.
Please anybody correct me.

Since currently direct reclaim is given up based on zone->all_unreclaimable flag,
so for e.g in one of the scenarios:

Lets say system has one node with two zones (NORMAL and MOVABLE) and we hot-remove the all the pages of the MOVABLE zone.

While migrating pages during memory hot-unplugging, the allocation function (for new page to which the page in MOVABLE zone would be moved) can end up looping in direct reclaim path for ever.

This is so because when most of the pages in the MOVABLE zone have been migrated,
the zone now contains lots of free memory (basically above low watermark) BUT all are in MIGRATE_ISOLATE list of the buddy list.

So kswapd() would not balance this zone as free pages are above low watermark (but all are in isolate list). So zone->all_unreclaimable flag would never be set for this zone
and allocation function would end up looping forever. (assuming the zone NORMAL is
left with no reclaimable memory)

Quoted 4 lines

Quoted 2 lines

Thanks a lot, Aaditya! Scenario you mentioned makes perfect. But I don't see it's a problem of kswapd.

Hi Kim,

I like called Minchan rather than Kim
Never mind. :)

Quoted 2 lines

Yes I agree it is not a problem of kswapd.

Yeb.

Quoted 23 lines

Quoted 8 lines

a5d76b54 made new migration type 'MIGRATE_ISOLATE' which is very irony type because there are many free pages in free list but we can't allocate it. :(
It doesn't reflect right NR_FREE_PAGES while many places in the kernel use NR_FREE_PAGES to trigger some operation. Kswapd is just one of them confused.
As right fix of this problem, we should fix hot plug code, IMHO which can fix CMA, too.

This patch could make inconsistency between NR_FREE_PAGES and SumOf[free_area[order].nr_free]

I assume that by the inconsistency you mention above, you mean temporary inconsistency.

Sorry, but IMHO as for memory hot plug the main issue with this patch is that the inconsistency you mentioned above would NOT be a temporary inconsistency.

Every time say 'x' number of page frames are off lined, they will introduce a difference of 'x' pages between NR_FREE_PAGES and SumOf[free_area[order].nr_free].
(So for e.g. if we do a frequent offline/online it will make NR_FREE_PAGES negative)

Quoted 7 lines

This is so because, unset_migratetype_isolate() is called from offlining code (to set the migrate type of off lined pages again back to MIGRATE_MOVABLE)
after the pages have been off lined and removed from the buddy list. Since the pages for which unset_migratetype_isolate() is called are not buddy pages so move_freepages_block() does not move any page, and thus introducing a permanent inconsistency.

Good point. Negative NR_FREE_PAGES is caused by double counting by my patch and __offline_isolated_pages. I think at first MIGRATE_ISOLATE type freed page shouldn't account as free page.

Quoted 6 lines

Quoted 5 lines

and it could make __zone_watermark_ok confuse so we might need to fix move_freepages_block itself to reflect free_area[order].nr_free exactly.

Any thought?

Quoted 7 lines

As for fixing move_freepages_block(), At least for memory hot plug, the pages stay in MIGRATE_ISOLATE list only for duration offline_pages() function,
I mean only temporarily. Since fixing move_freepages_block() for will introduce some overhead, So I am not very sure whether that overhead is justified
for a temporary condition. What do you think?

Yes. I don't like hurt fast path, either.
How about this? (Passed just compile test :( )
The patch's goal is to NOT increase nr_free and NR_FREE_PAGES about freed page into MIGRATE_ISOLATED.

Quoted 5 lines

Quoted 3 lines

This patch hurts high order page free path but I think it's not critical because higher order allocation is rare than order-0 allocation and we already have done same thing on free_hot_cold_page on order-0 free path which is more hot.

Can't we change zone_water_mark_ok_safe() instead of page allocator? memory hotplug is really rare event.

+1

Firstly, I want to make zone_page_state(z, NR_FREE_PAGES) itself more accurately because it is used by several places. As I looked over places, I can't find critical places except kswapd forever sleep case. So it's a nice idea!

In that case, we need zone->lock whenever zone_watermark_ok_safe is called.
Most of cases, it's unnecessary and it might hurt alloc/free performance when memory pressure is high. But if memory pressure is high, it may be already meaningless alloc/free performance. So it does make sense, IMHO.

Please raise your hands if anyone has a concern about this.

barrios@bbox:~/linux-next$ git diff

mm/page_alloc.c +16 -3

index d2a515d..82cc0a2 100644
@@ -1748,16 +1748,38 @@ bool zone_watermark_ok(struct zone *z, int order, unsigned long mark,
                                        zone_page_state(z, NR_FREE_PAGES));
 }
 
-bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long mark,
+bool zone_watermark_ok_safe(struct zone *z, int alloc_order, unsigned long mark,
                      int classzone_idx, int alloc_flags)
 {
+       struct free_area *area;
+       struct list_head *curr;
+       int order;
+       unsigned long flags;
        long free_pages = zone_page_state(z, NR_FREE_PAGES);
 
        if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
                free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
-       return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
-                                                               free_pages);
+       /*
+        * Memory hotplug/CMA can isolate freed page into MIGRATE_ISOLATE
+        * so that buddy can't allocate it although they are in free list.
+        */
+       spin_lock_irqsave(&z->lock, flags);
+       for (order = 0; order < MAX_ORDER; order++) {
+               int count = 0;
+               area = &(z->free_area[order]);
+               if (unlikely(!list_empty(&area->free_list[MIGRATE_ISOLATE]))) {
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;

Quoted 183 lines · Minchan Kim

Quoted 121 lines · KOSAKI Motohiro

Quoted 120 lines · Minchan Kim

Hi Aaditya,

I want to discuss this problem on another thread.

Quoted 49 lines · Aaditya Kumar

Quoted 48 lines · Minchan Kim

Quoted 44 lines · Aaditya Kumar

Quoted 14 lines · Minchan Kim

Quoted 8 lines

pgdat_balanced() doesn't recognized zone. Therefore kswapd may sleep if node has multiple zones. Hm ok, I realized my descriptions was slightly misleading. priority 0 is not needed. bakance_pddat() calls pgdat_balanced()
every priority. Most easy case is, movable zone has a lot of free pages and normal zone has no reclaimable page.

Quoted 4 lines

btw, current pgdat_balanced() logic seems not correct. kswapd should sleep only if every zones have much free pages than high water mark _and_ 25% of present pages in node are free.

Quoted 4 lines

Sorry. I can't understand your point.
Current kswapd doesn't sleep if relevant zones don't have free pages above high watermark. It seems I am missing your point.
Please anybody correct me.

Since currently direct reclaim is given up based on zone->all_unreclaimable flag,
so for e.g in one of the scenarios:

Lets say system has one node with two zones (NORMAL and MOVABLE) and we hot-remove the all the pages of the MOVABLE zone.

While migrating pages during memory hot-unplugging, the allocation function (for new page to which the page in MOVABLE zone would be moved) can end up looping in direct reclaim path for ever.

This is so because when most of the pages in the MOVABLE zone have been migrated,
the zone now contains lots of free memory (basically above low watermark) BUT all are in MIGRATE_ISOLATE list of the buddy list.

So kswapd() would not balance this zone as free pages are above low watermark (but all are in isolate list). So zone->all_unreclaimable flag would never be set for this zone
and allocation function would end up looping forever. (assuming the zone NORMAL is
left with no reclaimable memory)

Quoted 4 lines

Quoted 2 lines

Thanks a lot, Aaditya! Scenario you mentioned makes perfect. But I don't see it's a problem of kswapd.

Hi Kim,

I like called Minchan rather than Kim
Never mind. :)

Quoted 2 lines

Yes I agree it is not a problem of kswapd.

Yeb.

Quoted 23 lines

Quoted 8 lines

a5d76b54 made new migration type 'MIGRATE_ISOLATE' which is very irony type because there are many free pages in free list but we can't allocate it. :(
It doesn't reflect right NR_FREE_PAGES while many places in the kernel use NR_FREE_PAGES to trigger some operation. Kswapd is just one of them confused.
As right fix of this problem, we should fix hot plug code, IMHO which can fix CMA, too.

This patch could make inconsistency between NR_FREE_PAGES and SumOf[free_area[order].nr_free]

I assume that by the inconsistency you mention above, you mean temporary inconsistency.

Sorry, but IMHO as for memory hot plug the main issue with this patch is that the inconsistency you mentioned above would NOT be a temporary inconsistency.

Every time say 'x' number of page frames are off lined, they will introduce a difference of 'x' pages between NR_FREE_PAGES and SumOf[free_area[order].nr_free].
(So for e.g. if we do a frequent offline/online it will make NR_FREE_PAGES negative)

Quoted 7 lines

This is so because, unset_migratetype_isolate() is called from offlining code (to set the migrate type of off lined pages again back to MIGRATE_MOVABLE)
after the pages have been off lined and removed from the buddy list. Since the pages for which unset_migratetype_isolate() is called are not buddy pages so move_freepages_block() does not move any page, and thus introducing a permanent inconsistency.

Good point. Negative NR_FREE_PAGES is caused by double counting by my patch and __offline_isolated_pages. I think at first MIGRATE_ISOLATE type freed page shouldn't account as free page.

Quoted 6 lines

Quoted 5 lines

and it could make __zone_watermark_ok confuse so we might need to fix move_freepages_block itself to reflect free_area[order].nr_free exactly.

Any thought?

Quoted 7 lines

As for fixing move_freepages_block(), At least for memory hot plug, the pages stay in MIGRATE_ISOLATE list only for duration offline_pages() function,
I mean only temporarily. Since fixing move_freepages_block() for will introduce some overhead, So I am not very sure whether that overhead is justified
for a temporary condition. What do you think?

Yes. I don't like hurt fast path, either.
How about this? (Passed just compile test :( )
The patch's goal is to NOT increase nr_free and NR_FREE_PAGES about freed page into MIGRATE_ISOLATED.

Quoted 5 lines

Quoted 3 lines

This patch hurts high order page free path but I think it's not critical because higher order allocation is rare than order-0 allocation and we already have done same thing on free_hot_cold_page on order-0 free path which is more hot.

Can't we change zone_water_mark_ok_safe() instead of page allocator? memory hotplug is really rare event.

+1

Firstly, I want to make zone_page_state(z, NR_FREE_PAGES) itself more accurately because it is used by several places. As I looked over places, I can't find critical places except kswapd forever sleep case. So it's a nice idea!

In that case, we need zone->lock whenever zone_watermark_ok_safe is called.
Most of cases, it's unnecessary and it might hurt alloc/free performance when memory pressure is high. But if memory pressure is high, it may be already meaningless alloc/free performance. So it does make sense, IMHO.

Please raise your hands if anyone has a concern about this.

barrios@bbox:~/linux-next$ git diff

mm/page_alloc.c +21 -3

index d2a515d..82cc0a2 100644
@@ -1748,16 +1748,38 @@ bool zone_watermark_ok(struct zone *z, int order, unsigned long mark,
                                        zone_page_state(z, NR_FREE_PAGES));
 }
 
-bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long mark,
+bool zone_watermark_ok_safe(struct zone *z, int alloc_order, unsigned long mark,
                      int classzone_idx, int alloc_flags)
 {
+       struct free_area *area;
+       struct list_head *curr;
+       int order;
+       unsigned long flags;
        long free_pages = zone_page_state(z, NR_FREE_PAGES);
 
        if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
                free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
-       return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
-                                                               free_pages);
+       /*
+        * Memory hotplug/CMA can isolate freed page into MIGRATE_ISOLATE
+        * so that buddy can't allocate it although they are in free list.
+        */
+       spin_lock_irqsave(&z->lock, flags);
+       for (order = 0; order < MAX_ORDER; order++) {
+               int count = 0;
+               area = &(z->free_area[order]);
+               if (unlikely(!list_empty(&area->free_list[MIGRATE_ISOLATE]))) {
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+                       free_pages -= (count << order);
+               }
+       }
+       if (free_pages < 0)
+               free_pages = 0;

mm/page_alloc.c bool zone_watermark_ok(struct zone *z, int order, unsigned long mark, · line 1783

Quoted 5 lines

mm/page_alloc.c +4 -0

+       spin_unlock_irqrestore(&z->lock, flags);
+
+       return __zone_watermark_ok(z, alloc_order, mark,
+                               classzone_idx, alloc_flags, free_pages);
 }

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Quoted 2 lines

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. Because we have to tweak in page free path for pages which are going to free later after we mark pageblock type to MIGRATE_ISOLATE.

Quoted 6 lines

Quoted 2 lines

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. Because we have to tweak in page free path for pages which are going to free later after we mark pageblock type to MIGRATE_ISOLATE.

I mean,

if (nr_isolate_pageblock != 0)
free_pages -= nr_isolated_free_pages(); // your counting logic

return __zone_watermark_ok(z, alloc_order, mark,

                              classzone_idx, alloc_flags, free_pages);

I don't think this logic affect your race. zone_watermark_ok() is already racy. then new little race is no big matter.

Quoted 16 lines · KOSAKI Motohiro

Quoted 6 lines

Quoted 2 lines

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. Because we have to tweak in page free path for pages which are going to free later after we mark pageblock type to MIGRATE_ISOLATE.

I mean,

if (nr_isolate_pageblock != 0)
   free_pages -= nr_isolated_free_pages(); // your counting logic

return __zone_watermark_ok(z, alloc_order, mark, classzone_idx, alloc_flags, free_pages);

Quoted 2 lines

I don't think this logic affect your race. zone_watermark_ok() is already racy. then new little race is no big matter.

It seems my explanation wasn't enough. :(
I already understand your intention but we can't make nr_isolate_pageblock. Because we should count two type of free pages.

1. Already freed page so they are already in buddy list.
  Of course, we can count it with return value of move_freepages_block(zone, page, MIGRATE_ISOLATE) easily.

2. Will be FREEed page by do_migrate_range.
  It's a _PROBLEM_. For it, we should tweak free path. No?

If All of pages are PageLRU when hot-plug happens(ie, 2), nr_isolate_pagblock is zero and zone_watermk_ok_safe can't do his role.

Quoted 18 lines · Minchan Kim

Quoted 16 lines · KOSAKI Motohiro

Quoted 6 lines

Quoted 2 lines

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. Because we have to tweak in page free path for pages which are going to free later after we mark pageblock type to MIGRATE_ISOLATE.

I mean,

if (nr_isolate_pageblock != 0)
free_pages -= nr_isolated_free_pages(); // your counting logic

return __zone_watermark_ok(z, alloc_order, mark,

                              classzone_idx, alloc_flags, free_pages);

Quoted 7 lines

Quoted 2 lines

I don't think this logic affect your race. zone_watermark_ok() is already racy. then new little race is no big matter.

It seems my explanation wasn't enough. :(
I already understand your intention but we can't make nr_isolate_pageblock. Because we should count two type of free pages.

I mean, move_freepages_block increment number of page *block*, not pages. number of free *pages* are counted by zone_watermark_ok_safe().

Quoted 5 lines

1. Already freed page so they are already in buddy list.
Of course, we can count it with return value of move_freepages_block(zone, page, MIGRATE_ISOLATE) easily.

2. Will be FREEed page by do_migrate_range.
It's a _PROBLEM_. For it, we should tweak free path. No?

No.

Quoted 2 lines

If All of pages are PageLRU when hot-plug happens(ie, 2), nr_isolate_pagblock is zero and zone_watermk_ok_safe can't do his role.

number of isolate pageblock don't depend on number of free pages. It's a concept of
an attribute of PFN range.

Quoted 40 lines · KOSAKI Motohiro

Quoted 18 lines · Minchan Kim

Quoted 16 lines · KOSAKI Motohiro

Quoted 6 lines

Quoted 2 lines

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. Because we have to tweak in page free path for pages which are going to free later after we mark pageblock type to MIGRATE_ISOLATE.

I mean,

if (nr_isolate_pageblock != 0)

    free_pages -= nr_isolated_free_pages(); // your counting logic

return __zone_watermark_ok(z, alloc_order, mark,

                               classzone_idx, alloc_flags, free_pages);

Quoted 7 lines

Quoted 2 lines

I don't think this logic affect your race. zone_watermark_ok() is already racy. then new little race is no big matter.

It seems my explanation wasn't enough. :(
I already understand your intention but we can't make nr_isolate_pageblock. Because we should count two type of free pages.

I mean, move_freepages_block increment number of page *block*, not pages. number of free *pages* are counted by zone_watermark_ok_safe().

Quoted 5 lines

1. Already freed page so they are already in buddy list.
Of course, we can count it with return value of move_freepages_block(zone, page, MIGRATE_ISOLATE) easily.

2. Will be FREEed page by do_migrate_range.
It's a _PROBLEM_. For it, we should tweak free path. No?

No.

Quoted 6 lines

Quoted 2 lines

If All of pages are PageLRU when hot-plug happens(ie, 2), nr_isolate_pagblock is zero and zone_watermk_ok_safe can't do his role.

number of isolate pageblock don't depend on number of free pages. It's a concept of
an attribute of PFN range.

It seems you mean is_migrate_isolate as a just flag, NOT nr_isolate_pageblock. So do you mean this?

include/linux/page-isolation.h +1 -0

index 3bdcab3..7f4d19c 100644
@@ -1,6 +1,7 @@
  #ifndef __LINUX_PAGEISOLATION_H
  #define __LINUX_PAGEISOLATION_H
 
+extern bool is_migrate_isolate;
  /*
   * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE.
   * If specified range includes migrate types other than MOVABLE or CMA,

mm/page_alloc.c +21 -0

index d2a515d..b997cb3 100644
@@ -1756,6 +1756,27 @@ bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long ma
         if (z->percpu_drift_mark&&  free_pages<  z->percpu_drift_mark)
                 free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
+#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG
+       if (unlikely(is_migrate_isolate)) {
+               unsigned long flags;
+               spin_lock_irqsave(&z->lock, flags);
+               for (order = MAX_ORDER - 1; order>= 0; order--) {
+                       struct free_area *area =&z->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                       list_for_each(curr,&area->free_list[MIGRATE_ISOLATE])
+                               count++;
+
+                       free_pages -= (count<<  order);
+                       if (free_pages<  0) {
+                               free_pages = 0;
+                               break;
+                       }
+               }
+               spin_unlock_irqrestore(&z->lock, flags);
+       }
+#endif
         return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
                                                                 free_pages);
  }

mm/page_isolation.c +8 -0

index c9f0477..212e526 100644
@@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
         return pfn_to_page(pfn + i);
  }
 
+bool is_migrate_isolate = false;
+
  /*
   * start_isolate_page_range() -- make page-allocation-type of range of pages
   * to be MIGRATE_ISOLATE.

@@ -43,6 +45,8 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
         BUG_ON((start_pfn)&  (pageblock_nr_pages - 1));
         BUG_ON((end_pfn)&  (pageblock_nr_pages - 1));
 
+       is_migrate_isolate = true;
+
         for (pfn = start_pfn;
              pfn<  end_pfn;
              pfn += pageblock_nr_pages) {

@@ -59,6 +63,7 @@ undo:
              pfn += pageblock_nr_pages)
                 unset_migratetype_isolate(pfn_to_page(pfn), migratetype);
 
+       is_migrate_isolate = false;
         return -EBUSY;
  }
 

@@ -80,6 +85,9 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
                         continue;
                 unset_migratetype_isolate(page, migratetype);
         }
+
+       is_migrate_isolate = false;
+
         return 0;
  }
  /*

It is still racy as you already mentioned and I don't think it's trivial. Direct reclaim can't wake up kswapd forever by current fragile zone->all_unreclaimable. So it's a livelock.
Then, do you want to fix this problem by your patch[1]?

It could solve the livelock by OOM kill if we apply your patch[1] but still doesn't wake up kswapd although it's not critical. Okay. Then, please write down this problem in detail in your patch's changelog and resend, please.

Hm. I'm sorry if I couldn't chase the disucussion...Can I make summary ?

As you shown, it seems to be not difficult to counting free pages under MIGRATE_ISOLATE.
And we can know the zone contains MIGRATE_ISOLATE area or not by simple check.
for example.
==
               set_pageblock_migratetype(page, MIGRATE_ISOLATE); move_freepages_block(zone, page, MIGRATE_ISOLATE); zone->nr_isolated_areas++;
=

Then, the solution will be adding a function like following =
u64 zone_nr_free_pages(struct zone *zone) { unsigned long free_pages;

       free_pages = zone_page_state(NR_FREE_PAGES); if (unlikely(z->nr_isolated_areas)) { isolated = count_migrate_isolated_pages(zone); free_pages -= isolated;
       }
       return free_pages;
}
=

Right ?

and... zone->all_unreclaimable is a different problem ?

Quoted 164 lines · Minchan Kim

Quoted 48 lines · KOSAKI Motohiro

Quoted 27 lines · Minchan Kim

Quoted 15 lines · KOSAKI Motohiro

Quoted 9 lines

Quoted 3 lines

number of isolate page block is almost always 0. then if we have such counter,
we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE.
Because we have to tweak in page free path for pages which are going to free later after we
mark pageblock type to MIGRATE_ISOLATE.

I mean,

if (nr_isolate_pageblock != 0)

    free_pages -= nr_isolated_free_pages(); // your counting logic

Quoted 8 lines

return __zone_watermark_ok(z, alloc_order, mark,
classzone_idx, alloc_flags,
free_pages);

I don't think this logic affect your race. zone_watermark_ok() is already
racy. then new little race is no big matter.

Quoted 4 lines

It seems my explanation wasn't enough. :(
I already understand your intention but we can't make nr_isolate_pageblock.
Because we should count two type of free pages.

I mean, move_freepages_block increment number of page *block*, not pages.
number of free *pages* are counted by zone_watermark_ok_safe().

Quoted 6 lines

1. Already freed page so they are already in buddy list. Of course, we can count it with return value of
move_freepages_block(zone, page, MIGRATE_ISOLATE) easily.

2. Will be FREEed page by do_migrate_range.
It's a _PROBLEM_. For it, we should tweak free path. No?

No.

Quoted 7 lines

Quoted 3 lines

If All of pages are PageLRU when hot-plug happens(ie, 2), nr_isolate_pagblock is zero and zone_watermk_ok_safe can't do his role.

number of isolate pageblock don't depend on number of free pages. It's a concept of
an attribute of PFN range.

It seems you mean is_migrate_isolate as a just flag, NOT nr_isolate_pageblock.
So do you mean this?

include/linux/page-isolation.h +1 -0

index 3bdcab3..7f4d19c 100644
@@ -1,6 +1,7 @@
  #ifndef __LINUX_PAGEISOLATION_H
  #define __LINUX_PAGEISOLATION_H
 
+extern bool is_migrate_isolate;
  /*
   * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE.
   * If specified range includes migrate types other than MOVABLE or CMA,

mm/page_alloc.c +0 -0

index d2a515d..b997cb3 100644
@@ -1756,6 +1756,27 @@ bool zone_watermark_ok_safe(struct zone *z, int

order, unsigned long ma

         if (z->percpu_drift_mark&&  free_pages<  z->percpu_drift_mark)
                 free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);

mm/page_alloc.c +10 -0

+#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG
+       if (unlikely(is_migrate_isolate)) {
+               unsigned long flags;
+               spin_lock_irqsave(&z->lock, flags);
+               for (order = MAX_ORDER - 1; order>= 0; order--) {
+                       struct free_area *area =&z->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                      

list_for_each(curr,&area->free_list[MIGRATE_ISOLATE])

mm/page_alloc.c +11 -0

+                               count++;
+
+                       free_pages -= (count<<  order);
+                       if (free_pages<  0) {
+                               free_pages = 0;
+                               break;
+                       }
+               }
+               spin_unlock_irqrestore(&z->lock, flags);
+       }
+#endif
         return __zone_watermark_ok(z, order, mark, classzone_idx,

alloc_flags,

free_pages);
}

mm/page_isolation.c +2 -0

index c9f0477..212e526 100644
@@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long
 nr_pages)
         return pfn_to_page(pfn + i);
  }
 
+bool is_migrate_isolate = false;
+
  /*
   * start_isolate_page_range() -- make page-allocation-type of range

of pages
* to be MIGRATE_ISOLATE.

mm/page_isolation.c +2 -0

@@ -43,6 +45,8 @@ int start_isolate_page_range(unsigned long
 start_pfn, unsigned long end_pfn,
         BUG_ON((start_pfn)&  (pageblock_nr_pages - 1));
         BUG_ON((end_pfn)&  (pageblock_nr_pages - 1));
 
+       is_migrate_isolate = true;
+
         for (pfn = start_pfn;
              pfn<  end_pfn;

              pfn += pageblock_nr_pages) {

mm/page_isolation.c +4 -0

@@ -59,6 +63,7 @@ undo:
              pfn += pageblock_nr_pages)
                 unset_migratetype_isolate(pfn_to_page(pfn),
 migratetype);
 
+       is_migrate_isolate = false;
         return -EBUSY;
  }

@@ -80,6 +85,9 @@ int undo_isolate_page_range(unsigned long start_pfn,
 unsigned long end_pfn,
                         continue;
                 unset_migratetype_isolate(page, migratetype);
         }
+
+       is_migrate_isolate = false;
+
         return 0;
  }

/*

It is still racy as you already mentioned and I don't think it's trivial. Direct reclaim can't wake up kswapd forever by current fragile zone->all_unreclaimable.
So it's a livelock.
Then, do you want to fix this problem by your patch[1]?

Quoted 6 lines

It could solve the livelock by OOM kill if we apply your patch[1] but still doesn't wake up
kswapd although it's not critical. Okay. Then, please write down this problem in detail
in your patch's changelog and resend, please.

Hm. I'm sorry if I couldn't chase the disucussion...Can I make summary ?

As you shown, it seems to be not difficult to counting free pages under MIGRATE_ISOLATE.
And we can know the zone contains MIGRATE_ISOLATE area or not by simple check.
for example.
==

                set_pageblock_migratetype(page, MIGRATE_ISOLATE);
                move_freepages_block(zone, page, MIGRATE_ISOLATE);
                zone->nr_isolated_areas++;

=

Then, the solution will be adding a function like following =

u64 zone_nr_free_pages(struct zone *zone) {
    unsigned long free_pages;

    free_pages = zone_page_state(NR_FREE_PAGES);
    if (unlikely(z->nr_isolated_areas)) {
        isolated = count_migrate_isolated_pages(zone);
        free_pages -= isolated;
    }
    return free_pages;
}

=

Right ? and... zone->all_unreclaimable is a different problem ?

Second approach which is suggested by KOSAKI is what you mentioned.
But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas. I mean how to make sure nr_isolated_areas would be zero when isolation is done.
Of course, we can investigate all of current caller and make sure they don't make mistake now. But it's very error-prone if we consider future's user.
So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE);

Quoted 6 lines · Minchan Kim

Second approach which is suggested by KOSAKI is what you mentioned.
But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas. I mean how to make sure nr_isolated_areas would be zero when isolation is done.
Of course, we can investigate all of current caller and make sure they don't make mistake now. But it's very error-prone if we consider future's user.
So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE);

It's an implementation about above approach.

include/linux/mmzone.h +0 -0

index bf3404e..3e9a9e1 100644

include/linux/mmzone.h +0 -0

 } ____cacheline_internodealigned_in_smp;
 
 typedef enum {

mm/page_alloc.c +36 -0

index 2c29b1c..6cb1f9f 100644
@@ -219,6 +219,11 @@ EXPORT_SYMBOL(nr_online_nodes);
 
 int page_group_by_mobility_disabled __read_mostly;
 
+/*
+ * NOTE:
+ * Don't use set_pageblock_migratetype(page, MIGRATE_ISOLATE) direclty.
+ * Instead, use {un}set_pageblock_isolate.
+ */
 void set_pageblock_migratetype(struct page *page, int migratetype)
 {
        if (unlikely(page_group_by_mobility_disabled))

@@ -1622,6 +1627,28 @@ bool zone_watermark_ok(struct zone *z, int order, unsigned long mark,
                                        zone_page_state(z, NR_FREE_PAGES));
 }
 
+unsigned long migrate_isolate_pages(struct zone *zone)
+{
+       unsigned long nr_pages = 0;
+
+       if (unlikely(atomic_read(&zone->nr_migrate_isolate))) {
+               unsigned long flags;
+               int order;
+               spin_lock_irqsave(&zone->lock, flags);
+               for (order = 0; order < MAX_ORDER; order++) {
+                       struct free_area *area = &zone->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+                       nr_pages += (count << order);
+               }
+               spin_unlock_irqrestore(&zone->lock, flags);
+       }
+       return nr_pages;
+}
+
 bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long mark,
                      int classzone_idx, int alloc_flags)
 {

@@ -1630,6 +1657,14 @@ bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long mark,
        if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
                free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
+       /*
+        * If the zone has MIGRATE_ISOLATE type free page,
+        * we should consider it, too. Otherwise, kswapd can sleep forever.
+        */
+       free_pages -= migrate_isolate_pages(z);
+       if (free_pages < 0)
+               free_pages = 0;
+
        return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
                                                                free_pages);
 }

@@ -4408,6 +4443,7 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat,
                lruvec_init(&zone->lruvec, zone);
                zap_zone_vm_stats(zone);
                zone->flags = 0;
+               atomic_set(&zone->nr_migrate_isolate, 0);
                if (!size)
                        continue;
 

mm/page_alloc.c +34 -1

+               set_pageblock_flags_group(page, MIGRATE_UNMOVABLE,
+                                       PB_migrate, PB_migrate_end);
+               return;
+       }
+
+       old_migratetype = get_pageblock_migratetype(page);
+       set_pageblock_flags_group(page, MIGRATE_ISOLATE,
+                                       PB_migrate, PB_migrate_end);
+
+       if (old_migratetype != MIGRATE_ISOLATE)
+               atomic_inc(&zone->nr_migrate_isolate);
+}
+
+static void unset_pageblock_isolate(struct zone *zone, struct page *page,
+                               unsigned long migratetype)
+{
+       assert_spin_locked(&zone->lock);
+
+        if (unlikely(page_group_by_mobility_disabled)) {
+               set_pageblock_flags_group(page, migratetype,
+                                       PB_migrate, PB_migrate_end);
+               return;
+       }
+
+       BUG_ON(get_pageblock_migratetype(page) != MIGRATE_ISOLATE);
+       BUG_ON(migratetype == MIGRATE_ISOLATE);
+
+       set_pageblock_flags_group(page, migratetype,
+                                       PB_migrate, PB_migrate_end);
+       atomic_dec(&zone->nr_migrate_isolate);
+       BUG_ON(atomic_read(&zone->nr_migrate_isolate) < 0);
+}
+
 int set_migratetype_isolate(struct page *page)
 {
        struct zone *zone;

@@ -5601,7 +5676,7 @@ int set_migratetype_isolate(struct page *page)
 
 out:
        if (!ret) {
-               set_pageblock_migratetype(page, MIGRATE_ISOLATE);
+               set_pageblock_isolate(zone, page);
                move_freepages_block(zone, page, MIGRATE_ISOLATE);
        }
 

Quoted 12 lines · Minchan Kim

Quoted 6 lines · Minchan Kim

Second approach which is suggested by KOSAKI is what you mentioned.
But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas. I mean how to make sure nr_isolated_areas would be zero when isolation is done.
Of course, we can investigate all of current caller and make sure they don't make mistake now. But it's very error-prone if we consider future's user.
So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE);

It's an implementation about above approach.

include/linux/mmzone.h struct zone { · line 481

Quoted 13 lines

include/linux/mmzone.h +5 -0

index bf3404e..3e9a9e1 100644
@@ -474,6 +474,11 @@ struct zone {
         * rarely used fields:
         */  
        const char              *name;
+       /*
+        * the number of MIGRATE_ISOLATE pageblock
+        * We need this for accurate free page counting.
+        */
+       atomic_t                nr_migrate_isolate;

#ifdef CONFIG_MEMORY_HOTPLUG?

Quoted 74 lines

include/linux/mmzone.h +0 -0

 } ____cacheline_internodealigned_in_smp;
 
 typedef enum {

mm/page_alloc.c +36 -0

index 2c29b1c..6cb1f9f 100644
@@ -219,6 +219,11 @@ EXPORT_SYMBOL(nr_online_nodes);
 
 int page_group_by_mobility_disabled __read_mostly;
 
+/*
+ * NOTE:
+ * Don't use set_pageblock_migratetype(page, MIGRATE_ISOLATE) direclty.
+ * Instead, use {un}set_pageblock_isolate.
+ */
 void set_pageblock_migratetype(struct page *page, int migratetype)
 {
        if (unlikely(page_group_by_mobility_disabled))

@@ -1622,6 +1627,28 @@ bool zone_watermark_ok(struct zone *z, int order, unsigned long mark,
                                        zone_page_state(z, NR_FREE_PAGES));
 }
 
+unsigned long migrate_isolate_pages(struct zone *zone)
+{
+       unsigned long nr_pages = 0;
+
+       if (unlikely(atomic_read(&zone->nr_migrate_isolate))) {
+               unsigned long flags;
+               int order;
+               spin_lock_irqsave(&zone->lock, flags);
+               for (order = 0; order < MAX_ORDER; order++) {
+                       struct free_area *area = &zone->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+                       nr_pages += (count << order);
+               }
+               spin_unlock_irqrestore(&zone->lock, flags);
+       }
+       return nr_pages;
+}
+
 bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long mark,
                      int classzone_idx, int alloc_flags)
 {

@@ -1630,6 +1657,14 @@ bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long mark,
        if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
                free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
+       /*
+        * If the zone has MIGRATE_ISOLATE type free page,
+        * we should consider it, too. Otherwise, kswapd can sleep forever.
+        */
+       free_pages -= migrate_isolate_pages(z);
+       if (free_pages < 0)
+               free_pages = 0;
+
        return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
                                                                free_pages);
 }

@@ -4408,6 +4443,7 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat,
                lruvec_init(&zone->lruvec, zone);
                zap_zone_vm_stats(zone);
                zone->flags = 0;
+               atomic_set(&zone->nr_migrate_isolate, 0);
                if (!size)
                        continue;
 

mm/page_alloc.c bool is_pageblock_removable_nolock(struct page *page) · line 5599

Quoted 9 lines

mm/page_alloc.c +6 -0

@@ -5555,6 +5591,45 @@ bool is_pageblock_removable_nolock(struct page *page)
        return __count_immobile_pages(zone, page, 0);
 }
 
+static void set_pageblock_isolate(struct zone *zone, struct page *page)
+{
+       int old_migratetype;
+       assert_spin_locked(&zone->lock);
+
+        if (unlikely(page_group_by_mobility_disabled)) {

We don't need this check. page_group_by_mobility_disabled is an optimization for low memory system. but memory hotplug should work even though run on low memory.

In other words, current upstream code is buggy. :-)

Quoted 47 lines

mm/page_alloc.c +34 -1

+               set_pageblock_flags_group(page, MIGRATE_UNMOVABLE,
+                                       PB_migrate, PB_migrate_end);
+               return;
+       }
+
+       old_migratetype = get_pageblock_migratetype(page);
+       set_pageblock_flags_group(page, MIGRATE_ISOLATE,
+                                       PB_migrate, PB_migrate_end);
+
+       if (old_migratetype != MIGRATE_ISOLATE)
+               atomic_inc(&zone->nr_migrate_isolate);
+}
+
+static void unset_pageblock_isolate(struct zone *zone, struct page *page,
+                               unsigned long migratetype)
+{
+       assert_spin_locked(&zone->lock);
+
+        if (unlikely(page_group_by_mobility_disabled)) {
+               set_pageblock_flags_group(page, migratetype,
+                                       PB_migrate, PB_migrate_end);
+               return;
+       }
+
+       BUG_ON(get_pageblock_migratetype(page) != MIGRATE_ISOLATE);
+       BUG_ON(migratetype == MIGRATE_ISOLATE);
+
+       set_pageblock_flags_group(page, migratetype,
+                                       PB_migrate, PB_migrate_end);
+       atomic_dec(&zone->nr_migrate_isolate);
+       BUG_ON(atomic_read(&zone->nr_migrate_isolate) < 0);
+}
+
 int set_migratetype_isolate(struct page *page)
 {
        struct zone *zone;

@@ -5601,7 +5676,7 @@ int set_migratetype_isolate(struct page *page)
 
 out:
        if (!ret) {
-               set_pageblock_migratetype(page, MIGRATE_ISOLATE);
+               set_pageblock_isolate(zone, page);
                move_freepages_block(zone, page, MIGRATE_ISOLATE);
        }
 

mm/page_alloc.c void unset_migratetype_isolate(struct page *page, unsigned migratetype) · line 5698

Quoted 7 lines

mm/page_alloc.c +1 -1

@@ -5619,8 +5694,8 @@ void unset_migratetype_isolate(struct page *page, unsigned migratetype)
        spin_lock_irqsave(&zone->lock, flags);
        if (get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
                goto out;
-       set_pageblock_migratetype(page, migratetype);
        move_freepages_block(zone, page, migratetype);
+       unset_pageblock_isolate(zone, page, migratetype);

I don't think this order change is unnecessary. Why did you swap?

Other than that, looks very good to me.

+/*
+ * NOTE:
+ * Don't use set_pageblock_migratetype(page, MIGRATE_ISOLATE) direclty.
+ * Instead, use {un}set_pageblock_isolate.
+ */
void set_pageblock_migratetype(struct page *page, int migratetype) {
if (unlikely(page_group_by_mobility_disabled))

One more.

Quoted 8 lines

+/*
+ * NOTE:
+ * Don't use set_pageblock_migratetype(page, MIGRATE_ISOLATE) direclty.
+ * Instead, use {un}set_pageblock_isolate.
+ */
void set_pageblock_migratetype(struct page *page, int migratetype) {
if (unlikely(page_group_by_mobility_disabled))

I don't think we need this comment. please just add BUG_ON.

Quoted 6 lines · Minchan Kim

Second approach which is suggested by KOSAKI is what you mentioned.
But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas. I mean how to make sure nr_isolated_areas would be zero when isolation is done.
Of course, we can investigate all of current caller and make sure they don't make mistake now. But it's very error-prone if we consider future's user.
So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE);

It's an implementation about above approach.

include/linux/mmzone.h +0 -0

index bf3404e..3e9a9e1 100644

include/linux/mmzone.h +0 -0

  typedef enum {

mm/page_alloc.c +5 -0

index 2c29b1c..6cb1f9f 100644
@@ -219,6 +219,11 @@ EXPORT_SYMBOL(nr_online_nodes);
 
  int page_group_by_mobility_disabled __read_mostly;
 
+/*
+ * NOTE:
+ * Don't use set_pageblock_migratetype(page, MIGRATE_ISOLATE) direclty.
+ * Instead, use {un}set_pageblock_isolate.
+ */
  void set_pageblock_migratetype(struct page *page, int migratetype)
  {
         if (unlikely(page_group_by_mobility_disabled))

Quoted 13 lines · Minchan Kim

Quoted 9 lines · Minchan Kim

Second approach which is suggested by KOSAKI is what you mentioned. But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas.
I mean how to make sure nr_isolated_areas would be zero when isolation is done.
Of course, we can investigate all of current caller and make sure they don't make mistake
now. But it's very error-prone if we consider future's user. So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE);

Quoted 2 lines

It's an implementation about above approach.

I like this approach.

Quoted 5 lines

include/linux/mmzone.h +0 -0

index bf3404e..3e9a9e1 100644

include/linux/mmzone.h struct zone { · line 482

Quoted 9 lines

include/linux/mmzone.h +5 -0

@@ -474,6 +474,11 @@ struct zone {
          * rarely used fields:
          */
         const char              *name;
+       /*
+        * the number of MIGRATE_ISOLATE pageblock
+        * We need this for accurate free page counting.
+        */
+       atomic_t                nr_migrate_isolate;
  } ____cacheline_internodealigned_in_smp;

Isn't this counter modified only under zone->lock ?

Quoted 12 lines

include/linux/mmzone.h +0 -0

  typedef enum {

mm/page_alloc.c +0 -0

index 2c29b1c..6cb1f9f 100644
@@ -219,6 +219,11 @@ EXPORT_SYMBOL(nr_online_nodes);
 
  int page_group_by_mobility_disabled __read_mostly;
 

mm/page_alloc.c bool zone_watermark_ok(struct zone *z, int · line 1630

Quoted 13 lines

mm/page_alloc.c +5 -0

+/*
+ * NOTE:
+ * Don't use set_pageblock_migratetype(page, MIGRATE_ISOLATE) direclty.
+ * Instead, use {un}set_pageblock_isolate.
+ */
  void set_pageblock_migratetype(struct page *page, int migratetype)
  {
         if (unlikely(page_group_by_mobility_disabled))

@@ -1622,6 +1627,28 @@ bool zone_watermark_ok(struct zone *z, int
 order, unsigned long mark,
                                         zone_page_state(z,
 NR_FREE_PAGES));
  }

I'm glad if this function can be static...Hm. With easy grep, I think it can be...

Quoted 2 lines

I'm glad if this function can be static...Hm. With easy grep, I think it can be...

Yes. :)

Let me summary again.

The problem:

when hotplug offlining happens on zone A, it starts to freed page as MIGRATE_ISOLATE type in buddy. (MIGRATE_ISOLATE is very irony type because it's apparently on buddy but we can't allocate them) When the memory shortage happens during hotplug offlining, current task starts to reclaim, then wake up kswapd. Kswapd checks watermark, then go sleep BECAUSE current zone_watermark_ok_safe doesn't consider MIGRATE_ISOLATE freed page count. Current task continue to reclaim in direct reclaim path without kswapd's help. The problem is that zone->all_unreclaimable is set by only kswapd so that current task would be looping forever like below.

__alloc_pages_slowpath
restart:
       wake_all_kswapd
rebalance:
       __alloc_pages_direct_reclaim
               do_try_to_free_pages
                       if global_reclaim && !all_unreclaimable return 1; /* It means we did did_some_progress */ skip __alloc_pages_may_oom
       should_alloc_retry
               goto rebalance;

If we apply KOSAKI's patch[1] which doesn't depends on kswapd about setting zone->all_unreclaimable, we can solve this problem by killing some task. But it doesn't wake up kswapd, still. It could be a problem still if other subsystem needs GFP_ATOMIC request.
So kswapd should consider MIGRATE_ISOLATE when it calculate free pages before going sleep.

Firstly I tried to solve this problem by this.
https://lkml.org/lkml/2012/6/20/30
The patch's goal was to NOT increase nr_free and NR_FREE_PAGES when we free page into MIGRATE_ISOLATED. But it increases little overhead in higher order free page but I think it's not a big deal. More problem is duplicated codes for handling only MIGRATE_ISOLATE freed page.

Second approach which is suggested by KOSAKI is what you mentioned.
But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas. I mean how to make sure nr_isolated_areas would be zero when isolation is done.
Of course, we can investigate all of current caller and make sure they don't make mistake now. But it's very error-prone if we consider future's user.
So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE);

IMHO, ideal solution is that we remove MIGRATE_ISOLATE type totally in buddy.
For it, there is no problem to isolate already freed page in buddy allocator but the concern is how to handle freed page later by do_migrate_range in memory_hotplug.c.
We can create custom putback_lru_pages

put_page_hotplug(page)
{
       int migratetype = get_pageblock_migratetype(page) VM_BUG_ON(migratetype != MIGRATE_ISOLATE); __page_cache_release(page); free_one_page(zone, page, 0, MIGRATE_ISOLATE); }

putback_lru_pages_hotplug(&source)
{
       foreach page from source
               put_page_hotplug(page)
}

do_migrate_range()
{
       migrate_pages(&source);
       putback_lru_pages_hotplug(&source);
}

I hope this summary can help you, Kame and If I miss something, please let me know it.

Quoted 24 lines

Let me summary again.

The problem:

when hotplug offlining happens on zone A, it starts to freed page as MIGRATE_ISOLATE type in buddy. (MIGRATE_ISOLATE is very irony type because it's apparently on buddy but we can't allocate them) When the memory shortage happens during hotplug offlining, current task starts to reclaim, then wake up kswapd. Kswapd checks watermark, then go sleep BECAUSE current zone_watermark_ok_safe doesn't consider MIGRATE_ISOLATE freed page count. Current task continue to reclaim in direct reclaim path without kswapd's help. The problem is that zone->all_unreclaimable is set by only kswapd so that current task would be looping forever like below.

__alloc_pages_slowpath
restart:
       wake_all_kswapd
rebalance:
       __alloc_pages_direct_reclaim
               do_try_to_free_pages
                       if global_reclaim && !all_unreclaimable return 1; /* It means we did did_some_progress */ skip __alloc_pages_may_oom
       should_alloc_retry
               goto rebalance;

Quoted 4 lines

If we apply KOSAKI's patch[1] which doesn't depends on kswapd about setting zone->all_unreclaimable, we can solve this problem by killing some task. But it doesn't wake up kswapd, still. It could be a problem still if other subsystem needs GFP_ATOMIC request.
So kswapd should consider MIGRATE_ISOLATE when it calculate free pages before going sleep.

I agree. And I believe we should remove rebalance label and alloc retrying should always wake up kswapd.
because wake_all_kswapd is unreliable, it have no guarantee to success to wake up kswapd. then this
micro optimization is NOT optimization. Just trouble source. Our memory reclaim logic has a lot of race
by design. then any reclaim code shouldn't believe some one else works fine.

Quoted 33 lines

Firstly I tried to solve this problem by this.
https://lkml.org/lkml/2012/6/20/30
The patch's goal was to NOT increase nr_free and NR_FREE_PAGES when we free page into MIGRATE_ISOLATED. But it increases little overhead in higher order free page but I think it's not a big deal. More problem is duplicated codes for handling only MIGRATE_ISOLATE freed page.

Second approach which is suggested by KOSAKI is what you mentioned.
But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas. I mean how to make sure nr_isolated_areas would be zero when isolation is done.
Of course, we can investigate all of current caller and make sure they don't make mistake now. But it's very error-prone if we consider future's user.
So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE);

IMHO, ideal solution is that we remove MIGRATE_ISOLATE type totally in buddy.
For it, there is no problem to isolate already freed page in buddy allocator but the concern is how to handle freed page later by do_migrate_range in memory_hotplug.c.
We can create custom putback_lru_pages

put_page_hotplug(page)
{
       int migratetype = get_pageblock_migratetype(page) VM_BUG_ON(migratetype != MIGRATE_ISOLATE); __page_cache_release(page); free_one_page(zone, page, 0, MIGRATE_ISOLATE); }

putback_lru_pages_hotplug(&source)
{
       foreach page from source
               put_page_hotplug(page)
}

Quoted 7 lines

do_migrate_range()
{
       migrate_pages(&source);
       putback_lru_pages_hotplug(&source);
}

I hope this summary can help you, Kame and If I miss something, please let me know it.

I disagree this. Because of, memory hotplug intentionally don't use stopmachine. It is because
we don't stop any system service when memory is being unpluged. That's said various subsystem
try to allocate memory during page migration for memory unplug. IOW, we shouldn't do_migrate_page()
is only one caller.

Quoted 29 lines · KOSAKI Motohiro

Quoted 24 lines

Let me summary again.

The problem:

when hotplug offlining happens on zone A, it starts to freed page as MIGRATE_ISOLATE type in buddy. (MIGRATE_ISOLATE is very irony type because it's apparently on buddy but we can't allocate them) When the memory shortage happens during hotplug offlining, current task starts to reclaim, then wake up kswapd. Kswapd checks watermark, then go sleep BECAUSE current zone_watermark_ok_safe doesn't consider MIGRATE_ISOLATE freed page count. Current task continue to reclaim in direct reclaim path without kswapd's help. The problem is that zone->all_unreclaimable is set by only kswapd so that current task would be looping forever like below.

__alloc_pages_slowpath
restart:
       wake_all_kswapd
rebalance:
       __alloc_pages_direct_reclaim
               do_try_to_free_pages
                       if global_reclaim && !all_unreclaimable return 1; /* It means we did did_some_progress */ skip __alloc_pages_may_oom
       should_alloc_retry
               goto rebalance;

Quoted 4 lines

If we apply KOSAKI's patch[1] which doesn't depends on kswapd about setting zone->all_unreclaimable, we can solve this problem by killing some task. But it doesn't wake up kswapd, still. It could be a problem still if other subsystem needs GFP_ATOMIC request.
So kswapd should consider MIGRATE_ISOLATE when it calculate free pages before going sleep.

Quoted 8 lines

I agree. And I believe we should remove rebalance label and alloc retrying should always wake up kswapd.
because wake_all_kswapd is unreliable, it have no guarantee to success to wake up kswapd. then this
micro optimization is NOT optimization. Just trouble source. Our memory reclaim logic has a lot of race
by design. then any reclaim code shouldn't believe some one else works fine.

I think this is a better approach, since MIGRATE_ISLOATE is really a temporary phenomenon, it makes sense to just retry allocation. One issue however, with this approach is that it does not exactly work for PAGE_ALLOC_COSTLY_ORDER, But well, given the
frequency of such allocation, I think may be it is an acceptable compromise to handle such request by OOM in case of many MIGRATE_ISOLATE
pages present.

what do you think ?

Quoted 40 lines · KOSAKI Motohiro

Quoted 18 lines · Minchan Kim

Quoted 16 lines · KOSAKI Motohiro

Quoted 6 lines

Quoted 2 lines

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. Because we have to tweak in page free path for pages which are going to free later after we mark pageblock type to MIGRATE_ISOLATE.

I mean,

if (nr_isolate_pageblock != 0)
   free_pages -= nr_isolated_free_pages(); // your counting logic

return __zone_watermark_ok(z, alloc_order, mark, classzone_idx, alloc_flags, free_pages);

Quoted 7 lines

Quoted 2 lines

I don't think this logic affect your race. zone_watermark_ok() is already racy. then new little race is no big matter.

It seems my explanation wasn't enough. :(
I already understand your intention but we can't make nr_isolate_pageblock. Because we should count two type of free pages.

I mean, move_freepages_block increment number of page *block*, not pages. number of free *pages* are counted by zone_watermark_ok_safe().

Quoted 5 lines

1. Already freed page so they are already in buddy list.
  Of course, we can count it with return value of move_freepages_block(zone, page, MIGRATE_ISOLATE) easily.

2. Will be FREEed page by do_migrate_range.
  It's a _PROBLEM_. For it, we should tweak free path. No?

No.

Quoted 6 lines

Quoted 2 lines

If All of pages are PageLRU when hot-plug happens(ie, 2), nr_isolate_pagblock is zero and zone_watermk_ok_safe can't do his role.

number of isolate pageblock don't depend on number of free pages. It's a concept of
an attribute of PFN range.

It seems you mean is_migrate_isolate as a just flag, NOT nr_isolate_pageblock. So do you mean this?

include/linux/page-isolation.h +1 -0

index 3bdcab3..7f4d19c 100644
@@ -1,6 +1,7 @@
  #ifndef __LINUX_PAGEISOLATION_H
  #define __LINUX_PAGEISOLATION_H
 
+extern bool is_migrate_isolate;
  /*
  * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE.
  * If specified range includes migrate types other than MOVABLE or CMA,

mm/page_alloc.c +21 -0

index d2a515d..b997cb3 100644
@@ -1756,6 +1756,27 @@ bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long ma
        if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
                free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
+#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG
+       if (unlikely(is_migrate_isolate)) {
+               unsigned long flags;
+               spin_lock_irqsave(&z->lock, flags);
+               for (order = MAX_ORDER - 1; order >= 0; order--) {
+                       struct free_area *area = &z->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+
+                       free_pages -= (count << order);
+                       if (free_pages < 0) {
+                               free_pages = 0;
+                               break;
+                       }
+               }
+               spin_unlock_irqrestore(&z->lock, flags);
+       }
+#endif
        return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
                                                                free_pages);
  }

mm/page_isolation.c +5 -0

index c9f0477..212e526 100644
@@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
        return pfn_to_page(pfn + i);
  }
 
+bool is_migrate_isolate = false;
+
  /*
  * start_isolate_page_range() -- make page-allocation-type of range of pages
  * to be MIGRATE_ISOLATE.

@@ -43,6 +45,8 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
        BUG_ON((start_pfn) & (pageblock_nr_pages - 1));
        BUG_ON((end_pfn) & (pageblock_nr_pages - 1));
 
+       is_migrate_isolate = true;
+
        for (pfn = start_pfn;
             pfn < end_pfn;
             pfn += pageblock_nr_pages) {

@@ -59,6 +63,7 @@ undo:
             pfn += pageblock_nr_pages)
                unset_migratetype_isolate(pfn_to_page(pfn), migratetype);
 
+       is_migrate_isolate = false;
        return -EBUSY;
  }
 

It is still racy as you already mentioned and I don't think it's trivial. Direct reclaim can't wake up kswapd forever by current fragile zone->all_unreclaimable. So it's a livelock.
Then, do you want to fix this problem by your patch[1]?

It could solve the livelock by OOM kill if we apply your patch[1] but still doesn't wake up kswapd although it's not critical. Okay. Then, please write down this problem in detail in your patch's changelog and resend, please.

[1] http://lkml.org/lkml/2012/6/14/74

--
Kind regards,
Minchan Kim

Quoted 129 lines · Minchan Kim

Quoted 40 lines · KOSAKI Motohiro

Quoted 18 lines · Minchan Kim

Quoted 16 lines · KOSAKI Motohiro

Quoted 6 lines

Quoted 2 lines

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. Because we have to tweak in page free path for pages which are going to free later after we mark pageblock type to MIGRATE_ISOLATE.

I mean,

if (nr_isolate_pageblock != 0)
free_pages -= nr_isolated_free_pages(); // your counting logic

return __zone_watermark_ok(z, alloc_order, mark,

                              classzone_idx, alloc_flags, free_pages);

Quoted 7 lines

Quoted 2 lines

I don't think this logic affect your race. zone_watermark_ok() is already racy. then new little race is no big matter.

It seems my explanation wasn't enough. :(
I already understand your intention but we can't make nr_isolate_pageblock. Because we should count two type of free pages.

I mean, move_freepages_block increment number of page *block*, not pages. number of free *pages* are counted by zone_watermark_ok_safe().

Quoted 5 lines

1. Already freed page so they are already in buddy list.
Of course, we can count it with return value of move_freepages_block(zone, page, MIGRATE_ISOLATE) easily.

2. Will be FREEed page by do_migrate_range.
It's a _PROBLEM_. For it, we should tweak free path. No?

No.

Quoted 6 lines

Quoted 2 lines

If All of pages are PageLRU when hot-plug happens(ie, 2), nr_isolate_pagblock is zero and zone_watermk_ok_safe can't do his role.

number of isolate pageblock don't depend on number of free pages. It's a concept of
an attribute of PFN range.

It seems you mean is_migrate_isolate as a just flag, NOT nr_isolate_pageblock. So do you mean this?

include/linux/page-isolation.h +1 -0

index 3bdcab3..7f4d19c 100644
@@ -1,6 +1,7 @@
 #ifndef __LINUX_PAGEISOLATION_H
 #define __LINUX_PAGEISOLATION_H
 
+extern bool is_migrate_isolate;
 /*
 * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE.
 * If specified range includes migrate types other than MOVABLE or CMA,

mm/page_alloc.c +21 -0

index d2a515d..b997cb3 100644
@@ -1756,6 +1756,27 @@ bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long ma
       if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
               free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
+#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG
+       if (unlikely(is_migrate_isolate)) {
+               unsigned long flags;
+               spin_lock_irqsave(&z->lock, flags);
+               for (order = MAX_ORDER - 1; order >= 0; order--) {
+                       struct free_area *area = &z->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+
+                       free_pages -= (count << order);
+                       if (free_pages < 0) {
+                               free_pages = 0;
+                               break;
+                       }
+               }
+               spin_unlock_irqrestore(&z->lock, flags);
+       }
+#endif
       return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
                                                               free_pages);
 }

mm/page_isolation.c +5 -0

index c9f0477..212e526 100644
@@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
       return pfn_to_page(pfn + i);
 }
 
+bool is_migrate_isolate = false;
+
 /*
 * start_isolate_page_range() -- make page-allocation-type of range of pages
 * to be MIGRATE_ISOLATE.

@@ -43,6 +45,8 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
       BUG_ON((start_pfn) & (pageblock_nr_pages - 1));
       BUG_ON((end_pfn) & (pageblock_nr_pages - 1));
 
+       is_migrate_isolate = true;
+
       for (pfn = start_pfn;
            pfn < end_pfn;
            pfn += pageblock_nr_pages) {

@@ -59,6 +63,7 @@ undo:
            pfn += pageblock_nr_pages)
               unset_migratetype_isolate(pfn_to_page(pfn), migratetype);
 
+       is_migrate_isolate = false;
       return -EBUSY;
 }
 

mm/page_isolation.c int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, · line 93

Quoted 9 lines

mm/page_isolation.c +3 -0

@@ -80,6 +85,9 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
                       continue;
               unset_migratetype_isolate(page, migratetype);
       }
+
+       is_migrate_isolate = false;
+
       return 0;
 }
 /*

Hello Minchan,

Sorry for delayed response.

Instead of above how about something like this:

include/linux/page-isolation.h +2 -0

index 3bdcab3..fe9215f 100644
@@ -34,4 +34,6 @@ extern int set_migratetype_isolate(struct page *page);
 extern void unset_migratetype_isolate(struct page *page, unsigned migratetype);
 
 
+extern atomic_t is_migrate_isolated;
 
+

#endif

mm/memory-failure.c +0 -0

index ab1e714..e076fa2 100644

mm/memory-failure.c +0 -0

 	set_migratetype_isolate(p);
 	/*

mm/memory-failure.c +1 -0

 	 * When the target page is a free hugepage, just remove it

@@ -1406,6 +1407,7 @@ static int get_any_page(struct page *p, unsigned
 long pfn, int flags)
 	}
 	unset_migratetype_isolate(p, MIGRATE_MOVABLE);
 	unlock_memory_hotplug();
+	atomic_dec(&is_migrate_isolated);
 	return ret;
 }

mm/memory_hotplug.c +3 -0

index 0d7e3ec..cd7805c 100644
@@ -892,6 +892,7 @@ static int __ref offline_pages(unsigned long start_pfn,
 	nr_pages = end_pfn - start_pfn;
 
 	/* set above range as isolated */
+	atomic_inc(&is_migrate_isolated);
 	ret = start_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 	if (ret)
 		goto out;

@@ -958,6 +959,7 @@ repeat:
 	offline_isolated_pages(start_pfn, end_pfn);
 	/* reset pagetype flags and makes migrate type to be MOVABLE */
 	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
+	atomic_dec(&is_migrate_isolated);
 	/* removal success */
 	zone->present_pages -= offlined_pages;
 	zone->zone_pgdat->node_present_pages -= offlined_pages;

@@ -986,6 +988,7 @@ failed_removal:
 	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 
 out:
+	atomic_dec(&is_migrate_isolated);
 	unlock_memory_hotplug();
 	return ret;
 }

mm/page_alloc.c +22 -0

index 4403009..f549361 100644
@@ -1632,6 +1632,28 @@ bool zone_watermark_ok_safe(struct zone *z, int
 order, unsigned long mark,
 	if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
 		free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
+#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG
+       if (unlikely(atomic_read(is_migrate_isolated)) {
+               unsigned long flags;
+               spin_lock_irqsave(&z->lock, flags);
+               for (order = MAX_ORDER - 1; order >= 0; order--) {
+                       struct free_area *area = &z->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+
+                       free_pages -= (count << order);
+                       if (free_pages < 0) {
+                               free_pages = 0;
+                               break;
+                       }
+               }
+               spin_unlock_irqrestore(&z->lock, flags);
+       }
+#endif
+
 	return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
 								free_pages);

}

mm/page_alloc.c +1 -0

@@ -5785,6 +5807,7 @@ int alloc_contig_range(unsigned long start,
 unsigned long end,
 	 * put back to page allocator so that buddy can use them.
 	 */
 
+	atomic_inc(&is_migrate_isolated);
 	ret = start_isolate_page_range(pfn_max_align_down(start),
 				       pfn_max_align_up(end), migratetype);

mm/page_alloc.c +1 -0

 	if (ret)

@@ -5854,6 +5877,7 @@ int alloc_contig_range(unsigned long start,
 unsigned long end,
 done:
 	undo_isolate_page_range(pfn_max_align_down(start),
 				pfn_max_align_up(end), migratetype);
+	atomic_dec(&is_migrate_isolated);
 	return ret;
 }

mm/page_isolation.c +2 -0

index c9f0477..e8eb241 100644
@@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
 	return pfn_to_page(pfn + i);
 }
 
+atomic_t is_migrate_isolated;
+
 /*
  * start_isolate_page_range() -- make page-allocation-type of range of pages
  * to be MIGRATE_ISOLATE.

Quoted 16 lines

It is still racy as you already mentioned and I don't think it's trivial. Direct reclaim can't wake up kswapd forever by current fragile zone->all_unreclaimable. So it's a livelock.
Then, do you want to fix this problem by your patch[1]?

It could solve the livelock by OOM kill if we apply your patch[1] but still doesn't wake up kswapd although it's not critical. Okay. Then, please write down this problem in detail in your patch's changelog and resend, please.

[1] http://lkml.org/lkml/2012/6/14/74

--
Kind regards,
Minchan Kim

Hi Aaditya,

Quoted 157 lines · Aaditya Kumar

Quoted 129 lines · Minchan Kim

Quoted 40 lines · KOSAKI Motohiro

Quoted 18 lines · Minchan Kim

Quoted 16 lines · KOSAKI Motohiro

Quoted 6 lines

Quoted 2 lines

number of isolate page block is almost always 0. then if we have such counter, we almost always can avoid zone->lock. Just idea.

Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. Because we have to tweak in page free path for pages which are going to free later after we mark pageblock type to MIGRATE_ISOLATE.

I mean,

if (nr_isolate_pageblock != 0)
   free_pages -= nr_isolated_free_pages(); // your counting logic

return __zone_watermark_ok(z, alloc_order, mark, classzone_idx, alloc_flags, free_pages);

Quoted 7 lines

Quoted 2 lines

I don't think this logic affect your race. zone_watermark_ok() is already racy. then new little race is no big matter.

It seems my explanation wasn't enough. :(
I already understand your intention but we can't make nr_isolate_pageblock. Because we should count two type of free pages.

I mean, move_freepages_block increment number of page *block*, not pages. number of free *pages* are counted by zone_watermark_ok_safe().

Quoted 5 lines

1. Already freed page so they are already in buddy list.
  Of course, we can count it with return value of move_freepages_block(zone, page, MIGRATE_ISOLATE) easily.

2. Will be FREEed page by do_migrate_range.
  It's a _PROBLEM_. For it, we should tweak free path. No?

No.

Quoted 6 lines

Quoted 2 lines

If All of pages are PageLRU when hot-plug happens(ie, 2), nr_isolate_pagblock is zero and zone_watermk_ok_safe can't do his role.

number of isolate pageblock don't depend on number of free pages. It's a concept of
an attribute of PFN range.

It seems you mean is_migrate_isolate as a just flag, NOT nr_isolate_pageblock. So do you mean this?

include/linux/page-isolation.h +1 -0

index 3bdcab3..7f4d19c 100644
@@ -1,6 +1,7 @@
  #ifndef __LINUX_PAGEISOLATION_H
  #define __LINUX_PAGEISOLATION_H
 
+extern bool is_migrate_isolate;
  /*
  * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE.
  * If specified range includes migrate types other than MOVABLE or CMA,

mm/page_alloc.c +21 -0

index d2a515d..b997cb3 100644
@@ -1756,6 +1756,27 @@ bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long ma
        if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
                free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
+#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG
+       if (unlikely(is_migrate_isolate)) {
+               unsigned long flags;
+               spin_lock_irqsave(&z->lock, flags);
+               for (order = MAX_ORDER - 1; order >= 0; order--) {
+                       struct free_area *area = &z->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+
+                       free_pages -= (count << order);
+                       if (free_pages < 0) {
+                               free_pages = 0;
+                               break;
+                       }
+               }
+               spin_unlock_irqrestore(&z->lock, flags);
+       }
+#endif
        return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
                                                                free_pages);
  }

mm/page_isolation.c +5 -0

index c9f0477..212e526 100644
@@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
        return pfn_to_page(pfn + i);
  }
 
+bool is_migrate_isolate = false;
+
  /*
  * start_isolate_page_range() -- make page-allocation-type of range of pages
  * to be MIGRATE_ISOLATE.

@@ -43,6 +45,8 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
        BUG_ON((start_pfn) & (pageblock_nr_pages - 1));
        BUG_ON((end_pfn) & (pageblock_nr_pages - 1));
 
+       is_migrate_isolate = true;
+
        for (pfn = start_pfn;
             pfn < end_pfn;
             pfn += pageblock_nr_pages) {

@@ -59,6 +63,7 @@ undo:
             pfn += pageblock_nr_pages)
                unset_migratetype_isolate(pfn_to_page(pfn), migratetype);
 
+       is_migrate_isolate = false;
        return -EBUSY;
  }
 

mm/page_isolation.c int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, · line 93

Quoted 9 lines

mm/page_isolation.c +3 -0

@@ -80,6 +85,9 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
                        continue;
                unset_migratetype_isolate(page, migratetype);
        }
+
+       is_migrate_isolate = false;
+
        return 0;
  }
  /*

Hello Minchan,

Sorry for delayed response.

Instead of above how about something like this:

include/linux/page-isolation.h +2 -0

index 3bdcab3..fe9215f 100644
@@ -34,4 +34,6 @@ extern int set_migratetype_isolate(struct page *page);
  extern void unset_migratetype_isolate(struct page *page, unsigned migratetype);
 
 
+extern atomic_t is_migrate_isolated;
 
+

mm/memory-failure.c static int get_any_page(struct page *p, unsigned · line 1385

Quoted 11 lines

 #endif

mm/memory-failure.c +1 -0

index ab1e714..e076fa2 100644
@@ -1381,6 +1381,7 @@ static int get_any_page(struct page *p, unsigned
 long pfn, int flags)
        * Isolate the page, so that it doesn't get reallocated if it
        * was free.
        */
+     atomic_inc(&is_migrate_isolated);

I didn't take a detail look in your patch yet.

Yes. In my patch, I missed several caller.
It was just a patch for showing my intention, NOT formal patch. But I admit I didn't consider nesting case. brain-dead  :( Technically other problem about this is atomic doesn't imply memory barrier so we need barrier.

But the concern about this approach is following as Copy/Paste from my reply of Kame.

***
But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas. I mean how to make sure nr_isolated_areas would be zero when isolation is done.
Of course, we can investigate all of current caller and make sure they don't make mistake now. But it's very error-prone if we consider future's user.
So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE);

IMHO, ideal solution is that we remove MIGRATE_ISOLATE type totally in buddy.
...
...
***

Of course, We can choose this approach as interim. What do you think about it, Fujitsu guys?

Quoted 116 lines

mm/memory-failure.c +0 -0

       set_migratetype_isolate(p);
       /*

       * When the target page is a free hugepage, just remove it

mm/memory-failure.c +1 -0

@@ -1406,6 +1407,7 @@ static int get_any_page(struct page *p, unsigned
 long pfn, int flags)
       }
       unset_migratetype_isolate(p, MIGRATE_MOVABLE);
       unlock_memory_hotplug();
+     atomic_dec(&is_migrate_isolated);
       return ret;
  }

mm/memory_hotplug.c +2 -0

index 0d7e3ec..cd7805c 100644
@@ -892,6 +892,7 @@ static int __ref offline_pages(unsigned long start_pfn,
       nr_pages = end_pfn - start_pfn;
 
       /* set above range as isolated */
+     atomic_inc(&is_migrate_isolated);
       ret = start_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
       if (ret)
               goto out;

@@ -958,6 +959,7 @@ repeat:
       offline_isolated_pages(start_pfn, end_pfn);
       /* reset pagetype flags and makes migrate type to be MOVABLE */
       undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
+     atomic_dec(&is_migrate_isolated);
       /* removal success */
       zone->present_pages -= offlined_pages;
       zone->zone_pgdat->node_present_pages -= offlined_pages;

@@ -986,6 +988,7 @@ failed_removal:
       undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 

 out:
+     atomic_dec(&is_migrate_isolated);
      unlock_memory_hotplug();
      return ret;
 }

mm/page_alloc.c +22 -0

index 4403009..f549361 100644
@@ -1632,6 +1632,28 @@ bool zone_watermark_ok_safe(struct zone *z, int
 order, unsigned long mark,
       if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark)
               free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES);
 
+#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG
+       if (unlikely(atomic_read(is_migrate_isolated)) {
+               unsigned long flags;
+               spin_lock_irqsave(&z->lock, flags);
+               for (order = MAX_ORDER - 1; order >= 0; order--) {
+                       struct free_area *area = &z->free_area[order];
+                       long count = 0;
+                       struct list_head *curr;
+
+                       list_for_each(curr, &area->free_list[MIGRATE_ISOLATE])
+                               count++;
+
+                       free_pages -= (count << order);
+                       if (free_pages < 0) {
+                               free_pages = 0;
+                               break;
+                       }
+               }
+               spin_unlock_irqrestore(&z->lock, flags);
+       }
+#endif
+
       return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags,
                                                               free_pages);

 }

mm/page_alloc.c +1 -0

@@ -5785,6 +5807,7 @@ int alloc_contig_range(unsigned long start,
 unsigned long end,
        * put back to page allocator so that buddy can use them.
        */
 
+     atomic_inc(&is_migrate_isolated);
       ret = start_isolate_page_range(pfn_max_align_down(start),
                                      pfn_max_align_up(end), migratetype);

      if (ret)

mm/page_alloc.c +0 -0

@@ -5854,6 +5877,7 @@ int alloc_contig_range(unsigned long start,
 unsigned long end,

 done:
      undo_isolate_page_range(pfn_max_align_down(start), pfn_max_align_up(end), migratetype);
+     atomic_dec(&is_migrate_isolated);
      return ret;
 }

mm/page_isolation.c +2 -0

index c9f0477..e8eb241 100644
@@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
       return pfn_to_page(pfn + i);
  }
 
+atomic_t is_migrate_isolated;
+
  /*
   * start_isolate_page_range() -- make page-allocation-type of range of pages
   * to be MIGRATE_ISOLATE.

Quoted 11 lines

It is still racy as you already mentioned and I don't think it's trivial. Direct reclaim can't wake up kswapd forever by current fragile zone->all_unreclaimable. So it's a livelock.
Then, do you want to fix this problem by your patch[1]?

It could solve the livelock by OOM kill if we apply your patch[1] but still doesn't wake up kswapd although it's not critical. Okay. Then, please write down this problem in detail in your patch's changelog and resend, please.

Quoted 5 lines

Quoted 5 lines

[1] http://lkml.org/lkml/2012/6/14/74

--
Kind regards,
Minchan Kim

--
Kind regards,
Minchan Kim