__count_immobile_pages naming is rather awkward.
This patch clean up the function and add comment.
Cc: Mel Gorman <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: KAMEZAWA Hiroyuki <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
mm/page_alloc.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 019c4fe..2c71ac9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5467,26 +5467,27 @@ void set_pageblock_flags_group(struct page *page, unsigned long flags,
}
/*
- * This is designed as sub function...plz see page_isolation.c also.
- * set/clear page block's type to be ISOLATE.
- * page allocater never alloc memory from ISOLATE block.
+ * This function checks whether pageblock includes unmovable pages or not.
+ * If @count is not zero, it is okay to include less @count unmovable pages
+ *
+ * This function can race in PageLRU and MIGRATE_MOVABLE can have unmovable
+ * pages so that it might be not exact.
*/
-
-static int
-__count_immobile_pages(struct zone *zone, struct page *page, int count)
+static bool
+__has_unmovable_pages(struct zone *zone, struct page *page, int count)
{
unsigned long pfn, iter, found;
int mt;
/*
* For avoiding noise data, lru_add_drain_all() should be called
- * If ZONE_MOVABLE, the zone never contains immobile pages
+ * If ZONE_MOVABLE, the zone never contains unmovable pages
*/
if (zone_idx(zone) == ZONE_MOVABLE)
- return true;
+ return false;
mt = get_pageblock_migratetype(page);
if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))
- return true;
+ return false;
pfn = page_to_pfn(page);
for (found = 0, iter = 0; iter < pageblock_nr_pages; iter++) {
@@ -5521,9 +5522,9 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
* page at boot.
*/
if (found > count)
- return false;
+ return true;
}
- return true;
+ return false;
}
bool is_pageblock_removable_nolock(struct page *page)
@@ -5547,7 +5548,7 @@ bool is_pageblock_removable_nolock(struct page *page)
zone->zone_start_pfn + zone->spanned_pages <= pfn)
return false;
- return __count_immobile_pages(zone, page, 0);
+ return !__has_unmovable_pages(zone, page, 0);
}
int set_migratetype_isolate(struct page *page)
@@ -5586,12 +5587,12 @@ int set_migratetype_isolate(struct page *page)
* FIXME: Now, memory hotplug doesn't call shrink_slab() by itself.
* We just check MOVABLE pages.
*/
- if (__count_immobile_pages(zone, page, arg.pages_found))
+ if (!__has_unmovable_pages(zone, page, arg.pages_found))
ret = 0;
-
/*
- * immobile means "not-on-lru" paes. If immobile is larger than
- * removable-by-driver pages reported by notifier, we'll fail.
+ * Unmovable means "not-on-lru" pages. If Unmovable pages are
+ * larger than removable-by-driver pages reported by notifier,
+ * we'll fail.
*/
out:
--
1.7.9.5
(2012/06/11 11:07), Minchan Kim wrote:
> __count_immobile_pages naming is rather awkward.
> This patch clean up the function and add comment.
>
> Cc: Mel Gorman<[email protected]>
> Cc: Michal Hocko<[email protected]>
> Cc: KAMEZAWA Hiroyuki<[email protected]>
> Signed-off-by: Minchan Kim<[email protected]>
exchange true<->false caused by renaming ?
Acked-by: KAMEZAWA Hiroyuki <[email protected]>
> ---
> mm/page_alloc.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 019c4fe..2c71ac9 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5467,26 +5467,27 @@ void set_pageblock_flags_group(struct page *page, unsigned long flags,
> }
>
> /*
> - * This is designed as sub function...plz see page_isolation.c also.
> - * set/clear page block's type to be ISOLATE.
> - * page allocater never alloc memory from ISOLATE block.
> + * This function checks whether pageblock includes unmovable pages or not.
> + * If @count is not zero, it is okay to include less @count unmovable pages
> + *
> + * This function can race in PageLRU and MIGRATE_MOVABLE can have unmovable
> + * pages so that it might be not exact.
> */
> -
> -static int
> -__count_immobile_pages(struct zone *zone, struct page *page, int count)
> +static bool
> +__has_unmovable_pages(struct zone *zone, struct page *page, int count)
> {
> unsigned long pfn, iter, found;
> int mt;
>
> /*
> * For avoiding noise data, lru_add_drain_all() should be called
> - * If ZONE_MOVABLE, the zone never contains immobile pages
> + * If ZONE_MOVABLE, the zone never contains unmovable pages
> */
> if (zone_idx(zone) == ZONE_MOVABLE)
> - return true;
> + return false;
> mt = get_pageblock_migratetype(page);
> if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))
> - return true;
> + return false;
>
> pfn = page_to_pfn(page);
> for (found = 0, iter = 0; iter< pageblock_nr_pages; iter++) {
> @@ -5521,9 +5522,9 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> * page at boot.
> */
> if (found> count)
> - return false;
> + return true;
> }
> - return true;
> + return false;
> }
>
> bool is_pageblock_removable_nolock(struct page *page)
> @@ -5547,7 +5548,7 @@ bool is_pageblock_removable_nolock(struct page *page)
> zone->zone_start_pfn + zone->spanned_pages<= pfn)
> return false;
>
> - return __count_immobile_pages(zone, page, 0);
> + return !__has_unmovable_pages(zone, page, 0);
> }
>
> int set_migratetype_isolate(struct page *page)
> @@ -5586,12 +5587,12 @@ int set_migratetype_isolate(struct page *page)
> * FIXME: Now, memory hotplug doesn't call shrink_slab() by itself.
> * We just check MOVABLE pages.
> */
> - if (__count_immobile_pages(zone, page, arg.pages_found))
> + if (!__has_unmovable_pages(zone, page, arg.pages_found))
> ret = 0;
> -
> /*
> - * immobile means "not-on-lru" paes. If immobile is larger than
> - * removable-by-driver pages reported by notifier, we'll fail.
> + * Unmovable means "not-on-lru" pages. If Unmovable pages are
> + * larger than removable-by-driver pages reported by notifier,
> + * we'll fail.
> */
>
> out:
On Mon, Jun 11, 2012 at 04:08:02PM +0900, Kamezawa Hiroyuki wrote:
> (2012/06/11 11:07), Minchan Kim wrote:
> >__count_immobile_pages naming is rather awkward.
> >This patch clean up the function and add comment.
> >
> >Cc: Mel Gorman<[email protected]>
> >Cc: Michal Hocko<[email protected]>
> >Cc: KAMEZAWA Hiroyuki<[email protected]>
> >Signed-off-by: Minchan Kim<[email protected]>
>
> exchange true<->false caused by renaming ?
Exactly.
>
> Acked-by: KAMEZAWA Hiroyuki <[email protected]>
Thanks, Kame.
On Mon, 11 Jun 2012 11:07:22 +0900
Minchan Kim <[email protected]> wrote:
> __count_immobile_pages naming is rather awkward.
> This patch clean up the function and add comment.
This conflicts with
mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
and its fixes.
> + * This function can race in PageLRU and MIGRATE_MOVABLE can have unmovable
> + * pages so that it might be not exact.
I don't understand this. Functions race against other functions, not
against a page flag. Can we have another attempt at this description
please?
On 06/12/2012 06:40 AM, Andrew Morton wrote:
> On Mon, 11 Jun 2012 11:07:22 +0900
> Minchan Kim <[email protected]> wrote:
>
>> __count_immobile_pages naming is rather awkward.
>> This patch clean up the function and add comment.
>
> This conflicts with
> mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
> and its fixes.
I wanted to revert [1] and friends and merge again based on [2] and this patch.
Because [1] has still bug I explained in [2]. If it is merged without [2], it simply can
spread bug from one place(memory hotplug) to two place(memory hotplug and compaction).
We discussed real effectiveness of [1] because the patch is rather complicated than
expectation. I don't want to add unnecessary maintain cost if it doesn't have proved benefit.
KOSAKI and me : doesn't want to merge without proving (https://lkml.org/lkml/2012/6/5/3)
Mel: Pass the decision to CMA guys (https://lkml.org/lkml/2012/6/11/242)
Rik: want to test it based on THP alloc ratio (https://lkml.org/lkml/2012/6/11/293)
I guess anyone has no sure for needing it, at least.
Even, [1] added new vmstat "compact_rescued_unmovable_blocks".
Why I firstly suggest is just for the proving the effectiveness easily and wanted to
revert the vmstat later before merging mainline if we prove it.
(But it seems that KOSAKI doesn't like it - https://lkml.org/lkml/2012/6/5/282)
But now Bartlomiej want to maintain it permanently in vmstat.
IMHO, it's not a good idea.
Anyway, adding new vmstat part should be careful and get a agreement from mm guys.
[1] mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
[2] [PATCH] mm: do not use page_count without a page pin
>
>> + * This function can race in PageLRU and MIGRATE_MOVABLE can have unmovable
>> + * pages so that it might be not exact.
>
> I don't understand this. Functions race against other functions, not
> against a page flag. Can we have another attempt at this description
You're right. I meant page flags.
> please?
Before that, I would like to clear out how you handle this patch dependencies.
What should I do?
Any tree and any patchset based on for the work?
>
> --
> 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>
>
--
Kind regards,
Minchan Kim
On Tue, 12 Jun 2012 08:23:44 +0900
Minchan Kim <[email protected]> wrote:
> On 06/12/2012 06:40 AM, Andrew Morton wrote:
>
> > On Mon, 11 Jun 2012 11:07:22 +0900
> > Minchan Kim <[email protected]> wrote:
> >
> >> __count_immobile_pages naming is rather awkward.
> >> This patch clean up the function and add comment.
> >
> > This conflicts with
> > mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
> > and its fixes.
>
It would be useful to Cc Bart when we're discussing his patch...
> I wanted to revert [1] and friends and merge again based on [2] and this patch.
> Because [1] has still bug I explained in [2]. If it is merged without [2], it simply can
> spread bug from one place(memory hotplug) to two place(memory hotplug and compaction).
>
> We discussed real effectiveness of [1] because the patch is rather complicated than
> expectation. I don't want to add unnecessary maintain cost if it doesn't have proved benefit.
>
> KOSAKI and me : doesn't want to merge without proving (https://lkml.org/lkml/2012/6/5/3)
> Mel: Pass the decision to CMA guys (https://lkml.org/lkml/2012/6/11/242)
> Rik: want to test it based on THP alloc ratio (https://lkml.org/lkml/2012/6/11/293)
>
> I guess anyone has no sure for needing it, at least.
>
> Even, [1] added new vmstat "compact_rescued_unmovable_blocks".
> Why I firstly suggest is just for the proving the effectiveness easily and wanted to
> revert the vmstat later before merging mainline if we prove it.
> (But it seems that KOSAKI doesn't like it - https://lkml.org/lkml/2012/6/5/282)
> But now Bartlomiej want to maintain it permanently in vmstat.
> IMHO, it's not a good idea.
> Anyway, adding new vmstat part should be careful and get a agreement from mm guys.
>
> [1] mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
> [2] [PATCH] mm: do not use page_count without a page pin
Right now I'm inclined to drop
mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
then sit back and let you guys hash out a new patch(set).
On 06/14/2012 08:19 AM, Andrew Morton wrote:
> On Tue, 12 Jun 2012 08:23:44 +0900
> Minchan Kim <[email protected]> wrote:
>
>> On 06/12/2012 06:40 AM, Andrew Morton wrote:
>>
>>> On Mon, 11 Jun 2012 11:07:22 +0900
>>> Minchan Kim <[email protected]> wrote:
>>>
>>>> __count_immobile_pages naming is rather awkward.
>>>> This patch clean up the function and add comment.
>>>
>>> This conflicts with
>>> mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
>>> and its fixes.
>>
>
> It would be useful to Cc Bart when we're discussing his patch...
>
>> I wanted to revert [1] and friends and merge again based on [2] and this patch.
>> Because [1] has still bug I explained in [2]. If it is merged without [2], it simply can
>> spread bug from one place(memory hotplug) to two place(memory hotplug and compaction).
>>
>> We discussed real effectiveness of [1] because the patch is rather complicated than
>> expectation. I don't want to add unnecessary maintain cost if it doesn't have proved benefit.
>>
>> KOSAKI and me : doesn't want to merge without proving (https://lkml.org/lkml/2012/6/5/3)
>> Mel: Pass the decision to CMA guys (https://lkml.org/lkml/2012/6/11/242)
>> Rik: want to test it based on THP alloc ratio (https://lkml.org/lkml/2012/6/11/293)
>>
>> I guess anyone has no sure for needing it, at least.
>>
>> Even, [1] added new vmstat "compact_rescued_unmovable_blocks".
>> Why I firstly suggest is just for the proving the effectiveness easily and wanted to
>> revert the vmstat later before merging mainline if we prove it.
>> (But it seems that KOSAKI doesn't like it - https://lkml.org/lkml/2012/6/5/282)
>> But now Bartlomiej want to maintain it permanently in vmstat.
>> IMHO, it's not a good idea.
>> Anyway, adding new vmstat part should be careful and get a agreement from mm guys.
>>
>> [1] mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
>> [2] [PATCH] mm: do not use page_count without a page pin
>
> Right now I'm inclined to drop
> mm-compaction-handle-incorrect-migrate_unmovable-type-pageblocks.patch
> then sit back and let you guys hash out a new patch(set).
Bartlomiej already agreed resending based on [2].
Quote from last discussion with Bart
<<
On Monday 11 June 2012 03:26:49 Minchan Kim wrote:
> Hi Bartlomiej,
>
> On 06/08/2012 05:46 PM, Bartlomiej Zolnierkiewicz wrote:
>
> >
> > Hi,
> >
> > This version is much simpler as it just uses __count_immobile_pages()
> > instead of using its own open coded version and it integrates changes
>
>
> That's a good idea. I don't have noticed that function is there.
> When I look at the function, it has a problem, too.
> Please, look at this.
>
> https://lkml.org/lkml/2012/6/10/180
>
> If reviewer is okay that patch, I would like to resend your patch based on that.
Ok, I would later merge all changes into v11 and rebase on top of your patch."
>>
Remain thing is that I should resend [2] with Andrea's two suggestion.
(1) add comment THP tail page skip
(2) find unmovable page improvement on THP page
(1) is trivial so I am happy to add in my patch but (2) is going on discuss so didn't complete now
And (2) is optimization patch, not critical BUG fix so I will make it as another patch.
So, I will resend [2] and this patch mm: clean up __count_immobile_pages as soon as possible.
I hope Bartlomiej resend his patch based on them.
Thanks, Andrew.
>
> --
> 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>
>
--
Kind regards,
Minchan Kim