[PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Wei Yang]

This is not necessary to save the pfn to page->private.

The pfn could be retrieved by page_to_pfn() directly.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
Maybe I missed some critical reason to save pfn to private.

Thanks in advance if someone could reveal the special reason.
---
 mm/page_alloc.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 15ea511fb41c..a398eafbae46 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2793,24 +2793,19 @@ void free_unref_page(struct page *page)
 void free_unref_page_list(struct list_head *list)
 {
 	struct page *page, *next;
-	unsigned long flags, pfn;
+	unsigned long flags;
 	int batch_count = 0;
 
 	/* Prepare pages for freeing */
-	list_for_each_entry_safe(page, next, list, lru) {
-		pfn = page_to_pfn(page);
-		if (!free_unref_page_prepare(page, pfn))
+	list_for_each_entry_safe(page, next, list, lru)
+		if (!free_unref_page_prepare(page, page_to_pfn(page)))
 			list_del(&page->lru);
-		set_page_private(page, pfn);
-	}
 
 	local_irq_save(flags);
 	list_for_each_entry_safe(page, next, list, lru) {
-		unsigned long pfn = page_private(page);
-
 		set_page_private(page, 0);
 		trace_mm_page_free_batched(page);
-		free_unref_page_commit(page, pfn);
+		free_unref_page_commit(page, page_to_pfn(page));
 
 		/*
 		 * Guard against excessive IRQ disabled times when we get
-- 
2.15.1

Re: [PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Matthew Wilcox]

On Thu, Oct 18, 2018 at 09:04:29PM +0800, Wei Yang wrote:
> This is not necessary to save the pfn to page->private.
> 
> The pfn could be retrieved by page_to_pfn() directly.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
> Maybe I missed some critical reason to save pfn to private.
> 
> Thanks in advance if someone could reveal the special reason.

Performance.  Did you benchmark this?

Re: [PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Michal Hocko]

On Thu 18-10-18 21:04:29, Wei Yang wrote:
> This is not necessary to save the pfn to page->private.
> 
> The pfn could be retrieved by page_to_pfn() directly.

Yes it can, but a cursory look at the commit which has introduced this
suggests that this is a micro-optimization. Mel would know more of
course. There are some memory models where page_to_pfn is close to free.

If that is the case I am not really sure it is measurable or worth it.
In any case any change to this code should have a proper justification.
In other words, is this change really needed? Does it help in any
aspect? Possibly readability? The only thing I can guess from this
changelog is that you read the code and stumble over this. If that is
the case I would recommend asking author for the motivation and
potentially add a comment to explain it better rather than shoot a patch
rightaway.

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
> Maybe I missed some critical reason to save pfn to private.
> 
> Thanks in advance if someone could reveal the special reason.
> ---
>  mm/page_alloc.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 15ea511fb41c..a398eafbae46 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2793,24 +2793,19 @@ void free_unref_page(struct page *page)
>  void free_unref_page_list(struct list_head *list)
>  {
>  	struct page *page, *next;
> -	unsigned long flags, pfn;
> +	unsigned long flags;
>  	int batch_count = 0;
>  
>  	/* Prepare pages for freeing */
> -	list_for_each_entry_safe(page, next, list, lru) {
> -		pfn = page_to_pfn(page);
> -		if (!free_unref_page_prepare(page, pfn))
> +	list_for_each_entry_safe(page, next, list, lru)
> +		if (!free_unref_page_prepare(page, page_to_pfn(page)))
>  			list_del(&page->lru);
> -		set_page_private(page, pfn);
> -	}
>  
>  	local_irq_save(flags);
>  	list_for_each_entry_safe(page, next, list, lru) {
> -		unsigned long pfn = page_private(page);
> -
>  		set_page_private(page, 0);
>  		trace_mm_page_free_batched(page);
> -		free_unref_page_commit(page, pfn);
> +		free_unref_page_commit(page, page_to_pfn(page));
>  
>  		/*
>  		 * Guard against excessive IRQ disabled times when we get
> -- 
> 2.15.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Wei Yang]

On Thu, Oct 18, 2018 at 03:15:04PM +0200, Michal Hocko wrote:
>On Thu 18-10-18 21:04:29, Wei Yang wrote:
>> This is not necessary to save the pfn to page->private.
>> 
>> The pfn could be retrieved by page_to_pfn() directly.
>
>Yes it can, but a cursory look at the commit which has introduced this
>suggests that this is a micro-optimization. Mel would know more of
>course. There are some memory models where page_to_pfn is close to free.
>
>If that is the case I am not really sure it is measurable or worth it.
>In any case any change to this code should have a proper justification.
>In other words, is this change really needed? Does it help in any
>aspect? Possibly readability? The only thing I can guess from this
>changelog is that you read the code and stumble over this. If that is
>the case I would recommend asking author for the motivation and
>potentially add a comment to explain it better rather than shoot a patch
>rightaway.
>

Your are right. I am really willing to understand why we want to use
this mechanisum.

So the correct procedure is to send a mail to the mail list to query the
reason?

Re: [PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Michal Hocko]

On Thu 18-10-18 14:10:08, Wei Yang wrote:
> On Thu, Oct 18, 2018 at 03:15:04PM +0200, Michal Hocko wrote:
> >On Thu 18-10-18 21:04:29, Wei Yang wrote:
> >> This is not necessary to save the pfn to page->private.
> >> 
> >> The pfn could be retrieved by page_to_pfn() directly.
> >
> >Yes it can, but a cursory look at the commit which has introduced this
> >suggests that this is a micro-optimization. Mel would know more of
> >course. There are some memory models where page_to_pfn is close to free.
> >
> >If that is the case I am not really sure it is measurable or worth it.
> >In any case any change to this code should have a proper justification.
> >In other words, is this change really needed? Does it help in any
> >aspect? Possibly readability? The only thing I can guess from this
> >changelog is that you read the code and stumble over this. If that is
> >the case I would recommend asking author for the motivation and
> >potentially add a comment to explain it better rather than shoot a patch
> >rightaway.
> >
> 
> Your are right. I am really willing to understand why we want to use
> this mechanisum.

I am happy to hear that.

> So the correct procedure is to send a mail to the mail list to query the
> reason?

It is certainly better to ask a question than send a patch without a
proper justification. I would also encourage to use git blame to see
which patch has introduced the specific piece of code. Many times it
helps to understand the motivation. I would also encourage to go back to
the mailing list archives and the associate discussion to the specific
patch. In many cases there is Link: tag which can help you to find the
respective discussion.

Thanks!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Wei Yang]

On Thu, Oct 18, 2018 at 06:30:39PM +0200, Michal Hocko wrote:
>On Thu 18-10-18 14:10:08, Wei Yang wrote:
>> On Thu, Oct 18, 2018 at 03:15:04PM +0200, Michal Hocko wrote:
>> >On Thu 18-10-18 21:04:29, Wei Yang wrote:
>> >> This is not necessary to save the pfn to page->private.
>> >> 
>> >> The pfn could be retrieved by page_to_pfn() directly.
>> >
>> >Yes it can, but a cursory look at the commit which has introduced this
>> >suggests that this is a micro-optimization. Mel would know more of
>> >course. There are some memory models where page_to_pfn is close to free.
>> >
>> >If that is the case I am not really sure it is measurable or worth it.
>> >In any case any change to this code should have a proper justification.
>> >In other words, is this change really needed? Does it help in any
>> >aspect? Possibly readability? The only thing I can guess from this
>> >changelog is that you read the code and stumble over this. If that is
>> >the case I would recommend asking author for the motivation and
>> >potentially add a comment to explain it better rather than shoot a patch
>> >rightaway.
>> >
>> 
>> Your are right. I am really willing to understand why we want to use
>> this mechanisum.
>
>I am happy to hear that.
>
>> So the correct procedure is to send a mail to the mail list to query the
>> reason?
>
>It is certainly better to ask a question than send a patch without a
>proper justification. I would also encourage to use git blame to see
>which patch has introduced the specific piece of code. Many times it
>helps to understand the motivation. I would also encourage to go back to
>the mailing list archives and the associate discussion to the specific
>patch. In many cases there is Link: tag which can help you to find the
>respective discussion.
>

Sure, thanks for your suggestion.

>Thanks!
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Mel Gorman]

On Thu, Oct 18, 2018 at 09:04:29PM +0800, Wei Yang wrote:
> This is not necessary to save the pfn to page->private.
> 
> The pfn could be retrieved by page_to_pfn() directly.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

page_to_pfn is not free which is why it's cached.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Wei Yang]

On Thu, Oct 18, 2018 at 02:39:17PM +0100, Mel Gorman wrote:
>On Thu, Oct 18, 2018 at 09:04:29PM +0800, Wei Yang wrote:
>> This is not necessary to save the pfn to page->private.
>> 
>> The pfn could be retrieved by page_to_pfn() directly.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>page_to_pfn is not free which is why it's cached.
>

Hi, Mel

Thanks for your response.

Not free means the access to mem_section?

I have thought about the cache thing, so we assume the list is not that
long, and the cache could hold those page->private for the whole loop?

In my understand, it the cache has limited size, if more data accessed
the cache will be overwritten.

And another thing is:

In case of CONFIG_SPARSEMEM_VMEMMAP, would this be a little different?
Becase we get pfn by a simple addition. Which I think no need to cache
it?

Well, let me take a chance to say thanks to all, Mel, Michael and
Matthew. Hope my silly question won't bother you too much. :-)

>-- 
>Mel Gorman
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: get pfn by page_to_pfn() instead of save in page->private

[Mel Gorman]

On Thu, Oct 18, 2018 at 02:19:26PM +0000, Wei Yang wrote:
> On Thu, Oct 18, 2018 at 02:39:17PM +0100, Mel Gorman wrote:
> >On Thu, Oct 18, 2018 at 09:04:29PM +0800, Wei Yang wrote:
> >> This is not necessary to save the pfn to page->private.
> >> 
> >> The pfn could be retrieved by page_to_pfn() directly.
> >> 
> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >
> >page_to_pfn is not free which is why it's cached.
> >
> 
> Hi, Mel
> 
> Thanks for your response.
> 
> Not free means the access to mem_section?
> 

That's memory model specific but in some cases yes, it's accessing
mem_section.

> I have thought about the cache thing, so we assume the list is not that
> long, and the cache could hold those page->private for the whole loop?
> 

The intent was to avoid multiple page->pfn translations.

> In my understand, it the cache has limited size, if more data accessed
> the cache will be overwritten.
> 
> And another thing is:
> 
> In case of CONFIG_SPARSEMEM_VMEMMAP, would this be a little different?

Yes because the lookup has a different cost

> Becase we get pfn by a simple addition. Which I think no need to cache
> it?
> 

Because SPARSEMEM_VMEMMAP is not always used but also because it's
harmless to cache even in the SPARSEMEM_VMEMMAP case.

-- 
Mel Gorman
SUSE Labs