| Message ID | 20181018130429.37837-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: get pfn by page_to_pfn() instead of save in page->private | expand |
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?
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 >
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.
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?
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
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.
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!
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
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
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(-)