When I tested my new patches, I found that page pointer which is used
for setting page_owner information is changed. This is because page
pointer is used to set new migratetype in loop. After this work,
page pointer could be out of bound. If this wrong pointer is used for
page_owner, access violation happens. Below is error message that I got.
[ 6175.025217] BUG: unable to handle kernel paging request at 0000000000b00018
[ 6175.026400] IP: [<ffffffff81025f30>] save_stack_address+0x30/0x40
[ 6175.027341] PGD 1af2d067 PUD 166e0067 PMD 0
[ 6175.028129] Oops: 0002 [#1] SMP
snip...
[ 6175.055349] Call Trace:
[ 6175.055780] [<ffffffff81018c0f>] print_context_stack+0xcf/0x100
[ 6175.056794] [<ffffffff810f8552>] ? __module_text_address+0x12/0x70
[ 6175.057848] [<ffffffff810177cf>] dump_trace+0x15f/0x320
[ 6175.058751] [<ffffffff8106b140>] ? do_flush_tlb_all+0x50/0x50
[ 6175.059732] [<ffffffff810f5529>] ? smp_call_function_single+0xb9/0x120
[ 6175.060856] [<ffffffff81025e3f>] save_stack_trace+0x2f/0x50
[ 6175.061812] [<ffffffff811e3366>] __set_page_owner+0x46/0x70
[ 6175.062774] [<ffffffff8117bd47>] __isolate_free_page+0x1f7/0x210
[ 6175.063804] [<ffffffff8117bd81>] split_free_page+0x21/0xb0
[ 6175.064757] [<ffffffff8119aa82>] isolate_freepages_block+0x1e2/0x410
[ 6175.065855] [<ffffffff8119b53d>] compaction_alloc+0x22d/0x2d0
[ 6175.066850] [<ffffffff811d3779>] migrate_pages+0x289/0x8b0
[ 6175.067798] [<ffffffff8119c16a>] ? isolate_migratepages_block+0x28a/0x6e0
[ 6175.068960] [<ffffffff8119a000>] ? kmalloc_slab+0xa0/0xa0
[ 6175.069892] [<ffffffff8119b310>] ? ftrace_raw_event_mm_compaction_deplete_template+0xc0/0xc0
[ 6175.071327] [<ffffffff8119ce49>] compact_zone+0x409/0x880
[ 6175.072261] [<ffffffff8119d32d>] compact_zone_order+0x6d/0x90
[ 6175.073250] [<ffffffff8119d5d0>] try_to_compact_pages+0x110/0x210
[ 6175.074297] [<ffffffff8176e9e8>] __alloc_pages_direct_compact+0x3d/0xe6
[ 6175.075427] [<ffffffff8117d42d>] __alloc_pages_nodemask+0x6cd/0x9a0
[ 6175.076517] [<ffffffff811c2bf1>] alloc_pages_current+0x91/0x100
[ 6175.077545] [<ffffffff811e7216>] runtest_store+0x296/0xa50
[ 6175.078497] [<ffffffff813a553c>] ? simple_strtoull+0x2c/0x50
[ 6175.079465] [<ffffffff812130bd>] simple_attr_write+0xbd/0xe0
[ 6175.080458] [<ffffffff811eb038>] __vfs_write+0x28/0xf0
[ 6175.081349] [<ffffffff811edc39>] ? __sb_start_write+0x49/0xf0
[ 6175.082345] [<ffffffff8130fe25>] ? security_file_permission+0x45/0xd0
[ 6175.083453] [<ffffffff811eb729>] vfs_write+0xa9/0x1b0
[ 6175.084334] [<ffffffff811ec4f6>] SyS_write+0x46/0xb0
[ 6175.085196] [<ffffffff81172803>] ? context_tracking_user_enter+0x13/0x20
[ 6175.086339] [<ffffffff81024c55>] ? syscall_trace_leave+0xa5/0x120
[ 6175.087389] [<ffffffff81779472>] system_call_fastpath+0x16/0x75
This patch fixes this error by moving up set_page_owner().
Signed-off-by: Joonsoo Kim <[email protected]>
---
mm/page_alloc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index afd5459..70d6a85 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2003,6 +2003,8 @@ int __isolate_free_page(struct page *page, unsigned int order)
zone->free_area[order].nr_free--;
rmv_page_order(page);
+ set_page_owner(page, order, 0);
+
/* Set the pageblock if the isolated page is at least a pageblock */
if (order >= pageblock_order - 1) {
struct page *endpage = page + (1 << order) - 1;
@@ -2014,7 +2016,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
}
}
- set_page_owner(page, order, 0);
+
return 1UL << order;
}
--
1.9.1
Currently, we set wrong gfp_mask to page_owner info in case of
isolated freepage by compaction and split page. It causes incorrect
mixed pageblock report that we can get from '/proc/pagetypeinfo'.
This metric is really useful to measure fragmentation effect so
should be accurate. This patch fixes it by setting correct
information.
Without this patch, after kernel build workload is finished, number
of mixed pageblock is 112 among roughly 210 movable pageblocks.
But, with this fix, output shows that mixed pageblock is just 57.
Signed-off-by: Joonsoo Kim <[email protected]>
---
include/linux/page_owner.h | 13 +++++++++++++
mm/page_alloc.c | 8 +++++---
mm/page_owner.c | 7 +++++++
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
index b48c347..cacaabe 100644
--- a/include/linux/page_owner.h
+++ b/include/linux/page_owner.h
@@ -8,6 +8,7 @@ extern struct page_ext_operations page_owner_ops;
extern void __reset_page_owner(struct page *page, unsigned int order);
extern void __set_page_owner(struct page *page,
unsigned int order, gfp_t gfp_mask);
+extern gfp_t __get_page_owner_gfp(struct page *page);
static inline void reset_page_owner(struct page *page, unsigned int order)
{
@@ -25,6 +26,14 @@ static inline void set_page_owner(struct page *page,
__set_page_owner(page, order, gfp_mask);
}
+
+static inline gfp_t get_page_owner_gfp(struct page *page)
+{
+ if (likely(!page_owner_inited))
+ return 0;
+
+ return __get_page_owner_gfp(page);
+}
#else
static inline void reset_page_owner(struct page *page, unsigned int order)
{
@@ -33,6 +42,10 @@ static inline void set_page_owner(struct page *page,
unsigned int order, gfp_t gfp_mask)
{
}
+static inline gfp_t get_page_owner_gfp(struct page *page)
+{
+ return 0;
+}
#endif /* CONFIG_PAGE_OWNER */
#endif /* __LINUX_PAGE_OWNER_H */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 70d6a85..3ce3ec2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1957,6 +1957,7 @@ void free_hot_cold_page_list(struct list_head *list, bool cold)
void split_page(struct page *page, unsigned int order)
{
int i;
+ gfp_t gfp_mask;
VM_BUG_ON_PAGE(PageCompound(page), page);
VM_BUG_ON_PAGE(!page_count(page), page);
@@ -1970,10 +1971,11 @@ void split_page(struct page *page, unsigned int order)
split_page(virt_to_page(page[0].shadow), order);
#endif
- set_page_owner(page, 0, 0);
+ gfp_mask = get_page_owner_gfp(page);
+ set_page_owner(page, 0, gfp_mask);
for (i = 1; i < (1 << order); i++) {
set_page_refcounted(page + i);
- set_page_owner(page + i, 0, 0);
+ set_page_owner(page + i, 0, gfp_mask);
}
}
EXPORT_SYMBOL_GPL(split_page);
@@ -2003,7 +2005,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
zone->free_area[order].nr_free--;
rmv_page_order(page);
- set_page_owner(page, order, 0);
+ set_page_owner(page, order, __GFP_MOVABLE);
/* Set the pageblock if the isolated page is at least a pageblock */
if (order >= pageblock_order - 1) {
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 0993f5f..a3c4aed 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -76,6 +76,13 @@ void __set_page_owner(struct page *page, unsigned int order, gfp_t gfp_mask)
__set_bit(PAGE_EXT_OWNER, &page_ext->flags);
}
+gfp_t __get_page_owner_gfp(struct page *page)
+{
+ struct page_ext *page_ext = lookup_page_ext(page);
+
+ return page_ext->gfp_mask;
+}
+
static ssize_t
print_page_owner(char __user *buf, size_t count, unsigned long pfn,
struct page *page, struct page_ext *page_ext)
--
1.9.1
On Wed, Jul 15, 2015 at 03:33:58PM +0900, Joonsoo Kim wrote:
> When I tested my new patches, I found that page pointer which is used
> for setting page_owner information is changed. This is because page
> pointer is used to set new migratetype in loop. After this work,
> page pointer could be out of bound. If this wrong pointer is used for
> page_owner, access violation happens. Below is error message that I got.
>
> [ 6175.025217] BUG: unable to handle kernel paging request at 0000000000b00018
> [ 6175.026400] IP: [<ffffffff81025f30>] save_stack_address+0x30/0x40
> [ 6175.027341] PGD 1af2d067 PUD 166e0067 PMD 0
> [ 6175.028129] Oops: 0002 [#1] SMP
> snip...
> [ 6175.055349] Call Trace:
> [ 6175.055780] [<ffffffff81018c0f>] print_context_stack+0xcf/0x100
> [ 6175.056794] [<ffffffff810f8552>] ? __module_text_address+0x12/0x70
> [ 6175.057848] [<ffffffff810177cf>] dump_trace+0x15f/0x320
> [ 6175.058751] [<ffffffff8106b140>] ? do_flush_tlb_all+0x50/0x50
> [ 6175.059732] [<ffffffff810f5529>] ? smp_call_function_single+0xb9/0x120
> [ 6175.060856] [<ffffffff81025e3f>] save_stack_trace+0x2f/0x50
> [ 6175.061812] [<ffffffff811e3366>] __set_page_owner+0x46/0x70
> [ 6175.062774] [<ffffffff8117bd47>] __isolate_free_page+0x1f7/0x210
> [ 6175.063804] [<ffffffff8117bd81>] split_free_page+0x21/0xb0
> [ 6175.064757] [<ffffffff8119aa82>] isolate_freepages_block+0x1e2/0x410
> [ 6175.065855] [<ffffffff8119b53d>] compaction_alloc+0x22d/0x2d0
> [ 6175.066850] [<ffffffff811d3779>] migrate_pages+0x289/0x8b0
> [ 6175.067798] [<ffffffff8119c16a>] ? isolate_migratepages_block+0x28a/0x6e0
> [ 6175.068960] [<ffffffff8119a000>] ? kmalloc_slab+0xa0/0xa0
> [ 6175.069892] [<ffffffff8119b310>] ? ftrace_raw_event_mm_compaction_deplete_template+0xc0/0xc0
> [ 6175.071327] [<ffffffff8119ce49>] compact_zone+0x409/0x880
> [ 6175.072261] [<ffffffff8119d32d>] compact_zone_order+0x6d/0x90
> [ 6175.073250] [<ffffffff8119d5d0>] try_to_compact_pages+0x110/0x210
> [ 6175.074297] [<ffffffff8176e9e8>] __alloc_pages_direct_compact+0x3d/0xe6
> [ 6175.075427] [<ffffffff8117d42d>] __alloc_pages_nodemask+0x6cd/0x9a0
> [ 6175.076517] [<ffffffff811c2bf1>] alloc_pages_current+0x91/0x100
> [ 6175.077545] [<ffffffff811e7216>] runtest_store+0x296/0xa50
> [ 6175.078497] [<ffffffff813a553c>] ? simple_strtoull+0x2c/0x50
> [ 6175.079465] [<ffffffff812130bd>] simple_attr_write+0xbd/0xe0
> [ 6175.080458] [<ffffffff811eb038>] __vfs_write+0x28/0xf0
> [ 6175.081349] [<ffffffff811edc39>] ? __sb_start_write+0x49/0xf0
> [ 6175.082345] [<ffffffff8130fe25>] ? security_file_permission+0x45/0xd0
> [ 6175.083453] [<ffffffff811eb729>] vfs_write+0xa9/0x1b0
> [ 6175.084334] [<ffffffff811ec4f6>] SyS_write+0x46/0xb0
> [ 6175.085196] [<ffffffff81172803>] ? context_tracking_user_enter+0x13/0x20
> [ 6175.086339] [<ffffffff81024c55>] ? syscall_trace_leave+0xa5/0x120
> [ 6175.087389] [<ffffffff81779472>] system_call_fastpath+0x16/0x75
>
> This patch fixes this error by moving up set_page_owner().
>
> Signed-off-by: Joonsoo Kim <[email protected]>
Acked-by: Minchan Kim <[email protected]>
-stable material?
On Wed, Jul 15, 2015 at 03:33:59PM +0900, Joonsoo Kim wrote:
> Currently, we set wrong gfp_mask to page_owner info in case of
> isolated freepage by compaction and split page. It causes incorrect
> mixed pageblock report that we can get from '/proc/pagetypeinfo'.
> This metric is really useful to measure fragmentation effect so
> should be accurate. This patch fixes it by setting correct
> information.
>
> Without this patch, after kernel build workload is finished, number
> of mixed pageblock is 112 among roughly 210 movable pageblocks.
>
> But, with this fix, output shows that mixed pageblock is just 57.
>
> Signed-off-by: Joonsoo Kim <[email protected]>
> ---
> include/linux/page_owner.h | 13 +++++++++++++
> mm/page_alloc.c | 8 +++++---
> mm/page_owner.c | 7 +++++++
> 3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
> index b48c347..cacaabe 100644
> --- a/include/linux/page_owner.h
> +++ b/include/linux/page_owner.h
> @@ -8,6 +8,7 @@ extern struct page_ext_operations page_owner_ops;
> extern void __reset_page_owner(struct page *page, unsigned int order);
> extern void __set_page_owner(struct page *page,
> unsigned int order, gfp_t gfp_mask);
> +extern gfp_t __get_page_owner_gfp(struct page *page);
>
> static inline void reset_page_owner(struct page *page, unsigned int order)
> {
> @@ -25,6 +26,14 @@ static inline void set_page_owner(struct page *page,
>
> __set_page_owner(page, order, gfp_mask);
> }
> +
> +static inline gfp_t get_page_owner_gfp(struct page *page)
> +{
> + if (likely(!page_owner_inited))
> + return 0;
> +
> + return __get_page_owner_gfp(page);
> +}
> #else
> static inline void reset_page_owner(struct page *page, unsigned int order)
> {
> @@ -33,6 +42,10 @@ static inline void set_page_owner(struct page *page,
> unsigned int order, gfp_t gfp_mask)
> {
> }
> +static inline gfp_t get_page_owner_gfp(struct page *page)
> +{
> + return 0;
> +}
>
> #endif /* CONFIG_PAGE_OWNER */
> #endif /* __LINUX_PAGE_OWNER_H */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 70d6a85..3ce3ec2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1957,6 +1957,7 @@ void free_hot_cold_page_list(struct list_head *list, bool cold)
> void split_page(struct page *page, unsigned int order)
> {
> int i;
> + gfp_t gfp_mask;
>
> VM_BUG_ON_PAGE(PageCompound(page), page);
> VM_BUG_ON_PAGE(!page_count(page), page);
> @@ -1970,10 +1971,11 @@ void split_page(struct page *page, unsigned int order)
> split_page(virt_to_page(page[0].shadow), order);
> #endif
>
> - set_page_owner(page, 0, 0);
> + gfp_mask = get_page_owner_gfp(page);
> + set_page_owner(page, 0, gfp_mask);
> for (i = 1; i < (1 << order); i++) {
> set_page_refcounted(page + i);
> - set_page_owner(page + i, 0, 0);
> + set_page_owner(page + i, 0, gfp_mask);
> }
> }
> EXPORT_SYMBOL_GPL(split_page);
> @@ -2003,7 +2005,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
> zone->free_area[order].nr_free--;
> rmv_page_order(page);
>
> - set_page_owner(page, order, 0);
> + set_page_owner(page, order, __GFP_MOVABLE);
It seems the reason why __GFP_MOVABLE is okay is that __isolate_free_page
works on a free page on MIGRATE_MOVABLE|MIGRATE_CMA's pageblock. But if we
break the assumption in future, here is broken again?
Please put the comment here to cause it.
Otherwise, Good spot!
Reviewed-by: Minchan Kim <[email protected]>
On 07/16/2015 02:06 AM, Minchan Kim wrote:
> On Wed, Jul 15, 2015 at 03:33:59PM +0900, Joonsoo Kim wrote:
>> @@ -2003,7 +2005,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
>> zone->free_area[order].nr_free--;
>> rmv_page_order(page);
>>
>> - set_page_owner(page, order, 0);
>> + set_page_owner(page, order, __GFP_MOVABLE);
>
> It seems the reason why __GFP_MOVABLE is okay is that __isolate_free_page
> works on a free page on MIGRATE_MOVABLE|MIGRATE_CMA's pageblock. But if we
> break the assumption in future, here is broken again?
I didn't study the page owner code yet and I'm catching up after
vacation, but I share your concern. But I don't think the correctness
depends on the pageblock we are isolating from. I think the assumption
is that the isolated freepage will be used as a target for migration,
and that only movable pages can be successfully migrated (but also CMA
pages, and that information can be lost?). However there are also
efforts to allow migrate e.g. driver pages that won't be marked as
movable. And I'm not sure which migratetype are balloon pages which
already have special migration code.
So what I would think (without knowing all details) that the page owner
info should be transferred during page migration with all the other
flags, and shouldn't concern __isolate_free_page() at all?
> Please put the comment here to cause it.
>
> Otherwise, Good spot!
>
> Reviewed-by: Minchan Kim <[email protected]>
>
On Mon, Jul 20, 2015 at 01:27:55PM +0200, Vlastimil Babka wrote:
> On 07/16/2015 02:06 AM, Minchan Kim wrote:
> >On Wed, Jul 15, 2015 at 03:33:59PM +0900, Joonsoo Kim wrote:
> >>@@ -2003,7 +2005,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
> >> zone->free_area[order].nr_free--;
> >> rmv_page_order(page);
> >>
> >>- set_page_owner(page, order, 0);
> >>+ set_page_owner(page, order, __GFP_MOVABLE);
> >
> >It seems the reason why __GFP_MOVABLE is okay is that __isolate_free_page
> >works on a free page on MIGRATE_MOVABLE|MIGRATE_CMA's pageblock. But if we
> >break the assumption in future, here is broken again?
>
> I didn't study the page owner code yet and I'm catching up after
> vacation, but I share your concern. But I don't think the
> correctness depends on the pageblock we are isolating from. I think
> the assumption is that the isolated freepage will be used as a
> target for migration, and that only movable pages can be
> successfully migrated (but also CMA pages, and that information can
> be lost?). However there are also efforts to allow migrate e.g.
> driver pages that won't be marked as movable. And I'm not sure which
> migratetype are balloon pages which already have special migration
> code.
I am one of people who want to migrate driver pages from compaction
from zram point of view so I agree with you.
However, If I make zram support migratepages, I will use __GFP_MOVABLE.
So, I'm not sure there is any special driver that it can support migrate
via migratepage but it doesn't set __GFP_MOVABLE.
Having said that, I support your opinion because __GFP_MOVABLE is not
only gfp mask for allocating so we should take care of complete gfp
mask from original page.
>
> So what I would think (without knowing all details) that the page
> owner info should be transferred during page migration with all the
> other flags, and shouldn't concern __isolate_free_page() at all?
>
I agree.
Thanks.
On Thu, Jul 16, 2015 at 08:53:35AM +0900, Minchan Kim wrote:
> On Wed, Jul 15, 2015 at 03:33:58PM +0900, Joonsoo Kim wrote:
> > When I tested my new patches, I found that page pointer which is used
> > for setting page_owner information is changed. This is because page
> > pointer is used to set new migratetype in loop. After this work,
> > page pointer could be out of bound. If this wrong pointer is used for
> > page_owner, access violation happens. Below is error message that I got.
> >
> > [ 6175.025217] BUG: unable to handle kernel paging request at 0000000000b00018
> > [ 6175.026400] IP: [<ffffffff81025f30>] save_stack_address+0x30/0x40
> > [ 6175.027341] PGD 1af2d067 PUD 166e0067 PMD 0
> > [ 6175.028129] Oops: 0002 [#1] SMP
> > snip...
> > [ 6175.055349] Call Trace:
> > [ 6175.055780] [<ffffffff81018c0f>] print_context_stack+0xcf/0x100
> > [ 6175.056794] [<ffffffff810f8552>] ? __module_text_address+0x12/0x70
> > [ 6175.057848] [<ffffffff810177cf>] dump_trace+0x15f/0x320
> > [ 6175.058751] [<ffffffff8106b140>] ? do_flush_tlb_all+0x50/0x50
> > [ 6175.059732] [<ffffffff810f5529>] ? smp_call_function_single+0xb9/0x120
> > [ 6175.060856] [<ffffffff81025e3f>] save_stack_trace+0x2f/0x50
> > [ 6175.061812] [<ffffffff811e3366>] __set_page_owner+0x46/0x70
> > [ 6175.062774] [<ffffffff8117bd47>] __isolate_free_page+0x1f7/0x210
> > [ 6175.063804] [<ffffffff8117bd81>] split_free_page+0x21/0xb0
> > [ 6175.064757] [<ffffffff8119aa82>] isolate_freepages_block+0x1e2/0x410
> > [ 6175.065855] [<ffffffff8119b53d>] compaction_alloc+0x22d/0x2d0
> > [ 6175.066850] [<ffffffff811d3779>] migrate_pages+0x289/0x8b0
> > [ 6175.067798] [<ffffffff8119c16a>] ? isolate_migratepages_block+0x28a/0x6e0
> > [ 6175.068960] [<ffffffff8119a000>] ? kmalloc_slab+0xa0/0xa0
> > [ 6175.069892] [<ffffffff8119b310>] ? ftrace_raw_event_mm_compaction_deplete_template+0xc0/0xc0
> > [ 6175.071327] [<ffffffff8119ce49>] compact_zone+0x409/0x880
> > [ 6175.072261] [<ffffffff8119d32d>] compact_zone_order+0x6d/0x90
> > [ 6175.073250] [<ffffffff8119d5d0>] try_to_compact_pages+0x110/0x210
> > [ 6175.074297] [<ffffffff8176e9e8>] __alloc_pages_direct_compact+0x3d/0xe6
> > [ 6175.075427] [<ffffffff8117d42d>] __alloc_pages_nodemask+0x6cd/0x9a0
> > [ 6175.076517] [<ffffffff811c2bf1>] alloc_pages_current+0x91/0x100
> > [ 6175.077545] [<ffffffff811e7216>] runtest_store+0x296/0xa50
> > [ 6175.078497] [<ffffffff813a553c>] ? simple_strtoull+0x2c/0x50
> > [ 6175.079465] [<ffffffff812130bd>] simple_attr_write+0xbd/0xe0
> > [ 6175.080458] [<ffffffff811eb038>] __vfs_write+0x28/0xf0
> > [ 6175.081349] [<ffffffff811edc39>] ? __sb_start_write+0x49/0xf0
> > [ 6175.082345] [<ffffffff8130fe25>] ? security_file_permission+0x45/0xd0
> > [ 6175.083453] [<ffffffff811eb729>] vfs_write+0xa9/0x1b0
> > [ 6175.084334] [<ffffffff811ec4f6>] SyS_write+0x46/0xb0
> > [ 6175.085196] [<ffffffff81172803>] ? context_tracking_user_enter+0x13/0x20
> > [ 6175.086339] [<ffffffff81024c55>] ? syscall_trace_leave+0xa5/0x120
> > [ 6175.087389] [<ffffffff81779472>] system_call_fastpath+0x16/0x75
> >
> > This patch fixes this error by moving up set_page_owner().
> >
> > Signed-off-by: Joonsoo Kim <[email protected]>
> Acked-by: Minchan Kim <[email protected]>
>
> -stable material?
Hello,
Strangely, I didn't hit the error on the kernel without some of my
patches. But, yes, it seems stable candidate.
This patch is already merged in the mainline so I will send it to
stable tree soon.
Thanks.
Hello, all.
On Mon, Jul 20, 2015 at 08:54:13PM +0900, Minchan Kim wrote:
> On Mon, Jul 20, 2015 at 01:27:55PM +0200, Vlastimil Babka wrote:
> > On 07/16/2015 02:06 AM, Minchan Kim wrote:
> > >On Wed, Jul 15, 2015 at 03:33:59PM +0900, Joonsoo Kim wrote:
> > >>@@ -2003,7 +2005,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
> > >> zone->free_area[order].nr_free--;
> > >> rmv_page_order(page);
> > >>
> > >>- set_page_owner(page, order, 0);
> > >>+ set_page_owner(page, order, __GFP_MOVABLE);
> > >
> > >It seems the reason why __GFP_MOVABLE is okay is that __isolate_free_page
> > >works on a free page on MIGRATE_MOVABLE|MIGRATE_CMA's pageblock. But if we
> > >break the assumption in future, here is broken again?
> >
> > I didn't study the page owner code yet and I'm catching up after
> > vacation, but I share your concern. But I don't think the
> > correctness depends on the pageblock we are isolating from. I think
> > the assumption is that the isolated freepage will be used as a
> > target for migration, and that only movable pages can be
> > successfully migrated (but also CMA pages, and that information can
> > be lost?). However there are also efforts to allow migrate e.g.
> > driver pages that won't be marked as movable. And I'm not sure which
> > migratetype are balloon pages which already have special migration
> > code.
FYI, migratetype of ballon pages is also MIGRATE_MOVABLE if balloon
compaction is enabled.
> I am one of people who want to migrate driver pages from compaction
> from zram point of view so I agree with you.
> However, If I make zram support migratepages, I will use __GFP_MOVABLE.
> So, I'm not sure there is any special driver that it can support migrate
> via migratepage but it doesn't set __GFP_MOVABLE.
>
> Having said that, I support your opinion because __GFP_MOVABLE is not
> only gfp mask for allocating so we should take care of complete gfp
> mask from original page.
Ah... In this patch, I've solved the issue very narrowly. It works for
me to get correct fragmentation information but not generally correct.
It's my mistake.
There is another information like as stack trace so I should take care
of it, too. I will handle it in migration function.
>
> >
> > So what I would think (without knowing all details) that the page
> > owner info should be transferred during page migration with all the
> > other flags, and shouldn't concern __isolate_free_page() at all?
> >
>
> I agree.
Okay.
Thanks.