Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

[Vlastimil Babka]

On 05/12/2016 04:58 AM, Joonsoo Kim wrote:

On Tue, May 10, 2016 at 05:13:12PM +0200, Vlastimil Babka wrote:

Hmm... if it is the case, other fields are also misleading. I think
that we can tolerate this corner case and keeping function semantic as
function name suggests is better practice.


Hmm, OK. Let's not complicate it by specially marking pages that are 
under migration for now.



Thanks.

Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

[Vlastimil Babka]

On 05/12/2016 04:58 AM, Joonsoo Kim wrote:

On Tue, May 10, 2016 at 05:13:12PM +0200, Vlastimil Babka wrote:

Hmm... if it is the case, other fields are also misleading. I think
that we can tolerate this corner case and keeping function semantic as
function name suggests is better practice.


Hmm, OK. Let's not complicate it by specially marking pages that are 
under migration for now.



Thanks.

Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

[Joonsoo Kim]

On Tue, May 10, 2016 at 05:13:12PM +0200, Vlastimil Babka wrote:
> On 05/03/2016 07:23 AM, js1...@gmail.com wrote:
> >From: Joonsoo Kim 
> >
> >Currently, copy_page_owner() doesn't copy all the owner information.
> >It skips last_migrate_reason because copy_page_owner() is used for
> >migration and it will be properly set soon. But, following patch
> >will use copy_page_owner() and this skip will cause the problem that
> >allocated page has uninitialied last_migrate_reason. To prevent it,
> >this patch also copy last_migrate_reason in copy_page_owner().
> 
> Hmm it's a corner case, but if the "new" page was dumped e.g. due to
> a bug during the migration, is the copied migrate reason from the
> "old" page actually meaningful? I'd say it might be misleading and
> it's simpler to just make sure it's initialized to -1.

Hmm... if it is the case, other fields are also misleading. I think
that we can tolerate this corner case and keeping function semantic as
function name suggests is better practice.

Thanks.

Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

[Joonsoo Kim]

On Tue, May 10, 2016 at 05:13:12PM +0200, Vlastimil Babka wrote:
> On 05/03/2016 07:23 AM, js1...@gmail.com wrote:
> >From: Joonsoo Kim 
> >
> >Currently, copy_page_owner() doesn't copy all the owner information.
> >It skips last_migrate_reason because copy_page_owner() is used for
> >migration and it will be properly set soon. But, following patch
> >will use copy_page_owner() and this skip will cause the problem that
> >allocated page has uninitialied last_migrate_reason. To prevent it,
> >this patch also copy last_migrate_reason in copy_page_owner().
> 
> Hmm it's a corner case, but if the "new" page was dumped e.g. due to
> a bug during the migration, is the copied migrate reason from the
> "old" page actually meaningful? I'd say it might be misleading and
> it's simpler to just make sure it's initialized to -1.

Hmm... if it is the case, other fields are also misleading. I think
that we can tolerate this corner case and keeping function semantic as
function name suggests is better practice.

Thanks.

Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

[Vlastimil Babka]

On 05/03/2016 07:23 AM, js1...@gmail.com wrote:

From: Joonsoo Kim 

Currently, copy_page_owner() doesn't copy all the owner information.
It skips last_migrate_reason because copy_page_owner() is used for
migration and it will be properly set soon. But, following patch
will use copy_page_owner() and this skip will cause the problem that
allocated page has uninitialied last_migrate_reason. To prevent it,
this patch also copy last_migrate_reason in copy_page_owner().


Hmm it's a corner case, but if the "new" page was dumped e.g. due to a 
bug during the migration, is the copied migrate reason from the "old" 
page actually meaningful? I'd say it might be misleading and it's 
simpler to just make sure it's initialized to -1.



Signed-off-by: Joonsoo Kim 
---
  mm/page_owner.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index 792b56d..6693959 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -101,6 +101,7 @@ void __copy_page_owner(struct page *oldpage, struct page 
*newpage)

new_ext->order = old_ext->order;
new_ext->gfp_mask = old_ext->gfp_mask;
+   new_ext->last_migrate_reason = old_ext->last_migrate_reason;
new_ext->nr_entries = old_ext->nr_entries;

for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)

Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

[Vlastimil Babka]

On 05/03/2016 07:23 AM, js1...@gmail.com wrote:

From: Joonsoo Kim 

Currently, copy_page_owner() doesn't copy all the owner information.
It skips last_migrate_reason because copy_page_owner() is used for
migration and it will be properly set soon. But, following patch
will use copy_page_owner() and this skip will cause the problem that
allocated page has uninitialied last_migrate_reason. To prevent it,
this patch also copy last_migrate_reason in copy_page_owner().


Hmm it's a corner case, but if the "new" page was dumped e.g. due to a 
bug during the migration, is the copied migrate reason from the "old" 
page actually meaningful? I'd say it might be misleading and it's 
simpler to just make sure it's initialized to -1.



Signed-off-by: Joonsoo Kim 
---
  mm/page_owner.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index 792b56d..6693959 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -101,6 +101,7 @@ void __copy_page_owner(struct page *oldpage, struct page 
*newpage)

new_ext->order = old_ext->order;
new_ext->gfp_mask = old_ext->gfp_mask;
+   new_ext->last_migrate_reason = old_ext->last_migrate_reason;
new_ext->nr_entries = old_ext->nr_entries;

for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)

[PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

[js1304]

From: Joonsoo Kim 

Currently, copy_page_owner() doesn't copy all the owner information.
It skips last_migrate_reason because copy_page_owner() is used for
migration and it will be properly set soon. But, following patch
will use copy_page_owner() and this skip will cause the problem that
allocated page has uninitialied last_migrate_reason. To prevent it,
this patch also copy last_migrate_reason in copy_page_owner().

Signed-off-by: Joonsoo Kim 
---
 mm/page_owner.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index 792b56d..6693959 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -101,6 +101,7 @@ void __copy_page_owner(struct page *oldpage, struct page 
*newpage)
 
new_ext->order = old_ext->order;
new_ext->gfp_mask = old_ext->gfp_mask;
+   new_ext->last_migrate_reason = old_ext->last_migrate_reason;
new_ext->nr_entries = old_ext->nr_entries;
 
for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
-- 
1.9.1

[PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

[js1304]

From: Joonsoo Kim 

Currently, copy_page_owner() doesn't copy all the owner information.
It skips last_migrate_reason because copy_page_owner() is used for
migration and it will be properly set soon. But, following patch
will use copy_page_owner() and this skip will cause the problem that
allocated page has uninitialied last_migrate_reason. To prevent it,
this patch also copy last_migrate_reason in copy_page_owner().

Signed-off-by: Joonsoo Kim 
---
 mm/page_owner.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index 792b56d..6693959 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -101,6 +101,7 @@ void __copy_page_owner(struct page *oldpage, struct page 
*newpage)
 
new_ext->order = old_ext->order;
new_ext->gfp_mask = old_ext->gfp_mask;
+   new_ext->last_migrate_reason = old_ext->last_migrate_reason;
new_ext->nr_entries = old_ext->nr_entries;
 
for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
-- 
1.9.1