* [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages @ 2024-06-05 1:07 Wei Yang 2024-06-10 6:40 ` Mike Rapoport 0 siblings, 1 reply; 6+ messages in thread From: Wei Yang @ 2024-06-05 1:07 UTC (permalink / raw) To: rppt, akpm; +Cc: linux-mm, Wei Yang, Kirill A . Shutemov, David Hildenbrand Function deferred_[init|free]_pages are only used in deferred_init_maxorder(), which makes sure the range to init/free is within MAX_ORDER_NR_PAGES size. With this knowledge, we can simplify these two functions. Since * only the first pfn could be IS_MAX_ORDER_ALIGNED() Signed-off-by: Wei Yang <richard.weiyang@gmail.com> CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> CC: Mike Rapoport (IBM) <rppt@kernel.org> CC: David Hildenbrand <david@redhat.com> --- But my question is why we just test pfn_valid for the IS_MAX_ORDER_ALIGNED pfn? I thought we should test pfn_valid for each pfn until the first one in MAX_ORDER pages. Do I miss something? --- mm/mm_init.c | 43 ++++++++++++++----------------------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/mm/mm_init.c b/mm/mm_init.c index bbaf3a2c1cfd..6a4adf9e7d9a 100644 --- a/mm/mm_init.c +++ b/mm/mm_init.c @@ -1970,21 +1970,10 @@ static inline bool __init deferred_pfn_valid(unsigned long pfn) static void __init deferred_free_pages(unsigned long pfn, unsigned long end_pfn) { - unsigned long nr_free = 0; - - for (; pfn < end_pfn; pfn++) { - if (!deferred_pfn_valid(pfn)) { - deferred_free_range(pfn - nr_free, nr_free); - nr_free = 0; - } else if (IS_MAX_ORDER_ALIGNED(pfn)) { - deferred_free_range(pfn - nr_free, nr_free); - nr_free = 1; - } else { - nr_free++; - } - } - /* Free the last block of pages to allocator */ - deferred_free_range(pfn - nr_free, nr_free); + if (!deferred_pfn_valid(pfn)) + pfn++; + + deferred_free_range(pfn, end_pfn - pfn); } /* @@ -1992,27 +1981,23 @@ static void __init deferred_free_pages(unsigned long pfn, * by performing it only once every MAX_ORDER_NR_PAGES. * Return number of pages initialized. */ -static unsigned long __init deferred_init_pages(struct zone *zone, - unsigned long pfn, - unsigned long end_pfn) +static unsigned long __init deferred_init_pages(struct zone *zone, + unsigned long pfn, + unsigned long end_pfn) { int nid = zone_to_nid(zone); unsigned long nr_pages = 0; int zid = zone_idx(zone); struct page *page = NULL; - for (; pfn < end_pfn; pfn++) { - if (!deferred_pfn_valid(pfn)) { - page = NULL; - continue; - } else if (!page || IS_MAX_ORDER_ALIGNED(pfn)) { - page = pfn_to_page(pfn); - } else { - page++; - } + if (!deferred_pfn_valid(pfn)) + pfn++; + + page = pfn_to_page(pfn); + nr_pages = end_pfn - pfn; + + for (; pfn < end_pfn; pfn++, page++) __init_single_page(page, pfn, zid, nid); - nr_pages++; - } return nr_pages; } -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages 2024-06-05 1:07 [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages Wei Yang @ 2024-06-10 6:40 ` Mike Rapoport 2024-06-10 14:54 ` Wei Yang 0 siblings, 1 reply; 6+ messages in thread From: Mike Rapoport @ 2024-06-10 6:40 UTC (permalink / raw) To: Wei Yang; +Cc: akpm, linux-mm, Kirill A . Shutemov, David Hildenbrand On Wed, Jun 05, 2024 at 01:07:42AM +0000, Wei Yang wrote: > Function deferred_[init|free]_pages are only used in > deferred_init_maxorder(), which makes sure the range to init/free is > within MAX_ORDER_NR_PAGES size. > > With this knowledge, we can simplify these two functions. Since > > * only the first pfn could be IS_MAX_ORDER_ALIGNED() No, the first pfn is not necessarily IS_MAX_ORDER_ALIGNED(). Start pfn is a beginning of a region in memblock.memory, and there's no guarantee on it's alignment. > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > CC: Mike Rapoport (IBM) <rppt@kernel.org> > CC: David Hildenbrand <david@redhat.com> > > --- > But my question is why we just test pfn_valid for the > IS_MAX_ORDER_ALIGNED pfn? I thought we should test pfn_valid for each > pfn until the first one in MAX_ORDER pages. Do I miss something? > --- > mm/mm_init.c | 43 ++++++++++++++----------------------------- > 1 file changed, 14 insertions(+), 29 deletions(-) > > diff --git a/mm/mm_init.c b/mm/mm_init.c > index bbaf3a2c1cfd..6a4adf9e7d9a 100644 > --- a/mm/mm_init.c > +++ b/mm/mm_init.c > @@ -1970,21 +1970,10 @@ static inline bool __init deferred_pfn_valid(unsigned long pfn) > static void __init deferred_free_pages(unsigned long pfn, > unsigned long end_pfn) > { > - unsigned long nr_free = 0; > - > - for (; pfn < end_pfn; pfn++) { > - if (!deferred_pfn_valid(pfn)) { > - deferred_free_range(pfn - nr_free, nr_free); > - nr_free = 0; > - } else if (IS_MAX_ORDER_ALIGNED(pfn)) { > - deferred_free_range(pfn - nr_free, nr_free); > - nr_free = 1; > - } else { > - nr_free++; > - } > - } > - /* Free the last block of pages to allocator */ > - deferred_free_range(pfn - nr_free, nr_free); > + if (!deferred_pfn_valid(pfn)) > + pfn++; > + > + deferred_free_range(pfn, end_pfn - pfn); > } > > /* > @@ -1992,27 +1981,23 @@ static void __init deferred_free_pages(unsigned long pfn, > * by performing it only once every MAX_ORDER_NR_PAGES. > * Return number of pages initialized. > */ > -static unsigned long __init deferred_init_pages(struct zone *zone, > - unsigned long pfn, > - unsigned long end_pfn) > +static unsigned long __init deferred_init_pages(struct zone *zone, > + unsigned long pfn, > + unsigned long end_pfn) > { > int nid = zone_to_nid(zone); > unsigned long nr_pages = 0; > int zid = zone_idx(zone); > struct page *page = NULL; > > - for (; pfn < end_pfn; pfn++) { > - if (!deferred_pfn_valid(pfn)) { > - page = NULL; > - continue; > - } else if (!page || IS_MAX_ORDER_ALIGNED(pfn)) { > - page = pfn_to_page(pfn); > - } else { > - page++; > - } > + if (!deferred_pfn_valid(pfn)) > + pfn++; > + > + page = pfn_to_page(pfn); > + nr_pages = end_pfn - pfn; > + > + for (; pfn < end_pfn; pfn++, page++) > __init_single_page(page, pfn, zid, nid); > - nr_pages++; > - } > return nr_pages; > } > > -- > 2.34.1 > -- Sincerely yours, Mike. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages 2024-06-10 6:40 ` Mike Rapoport @ 2024-06-10 14:54 ` Wei Yang 2024-06-11 9:58 ` Mike Rapoport 0 siblings, 1 reply; 6+ messages in thread From: Wei Yang @ 2024-06-10 14:54 UTC (permalink / raw) To: Mike Rapoport Cc: Wei Yang, akpm, linux-mm, Kirill A . Shutemov, David Hildenbrand On Mon, Jun 10, 2024 at 09:40:33AM +0300, Mike Rapoport wrote: >On Wed, Jun 05, 2024 at 01:07:42AM +0000, Wei Yang wrote: >> Function deferred_[init|free]_pages are only used in >> deferred_init_maxorder(), which makes sure the range to init/free is >> within MAX_ORDER_NR_PAGES size. >> >> With this knowledge, we can simplify these two functions. Since >> >> * only the first pfn could be IS_MAX_ORDER_ALIGNED() > >No, the first pfn is not necessarily IS_MAX_ORDER_ALIGNED(). Start pfn is a >beginning of a region in memblock.memory, and there's no guarantee on it's >alignment. > Yes, I mean only the first pfn is possible to be IS_MAX_ORDER_ALIGNED(), not must be IS_MAX_ORDER_ALIGNED(). The range passed to deferred_[init|free]_pages must be within one MAX_ORDER_NR_PAGES. If the first pfn is not IS_MAX_ORDER_ALIGNED(), others could not be IS_MAX_ORDER_ALIGNED(). Currently these two functions would iterate all pfn, and check IS_MAX_ORDER_ALIGNED() to break init|free on each MAX_ORDER_NR_PAGES. But this is only possible and necessary on the first pfn. >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> CC: Mike Rapoport (IBM) <rppt@kernel.org> >> CC: David Hildenbrand <david@redhat.com> >> >> --- >> But my question is why we just test pfn_valid for the >> IS_MAX_ORDER_ALIGNED pfn? I thought we should test pfn_valid for each >> pfn until the first one in MAX_ORDER pages. Do I miss something? >> --- >> mm/mm_init.c | 43 ++++++++++++++----------------------------- >> 1 file changed, 14 insertions(+), 29 deletions(-) >> >> diff --git a/mm/mm_init.c b/mm/mm_init.c >> index bbaf3a2c1cfd..6a4adf9e7d9a 100644 >> --- a/mm/mm_init.c >> +++ b/mm/mm_init.c >> @@ -1970,21 +1970,10 @@ static inline bool __init deferred_pfn_valid(unsigned long pfn) >> static void __init deferred_free_pages(unsigned long pfn, >> unsigned long end_pfn) >> { >> - unsigned long nr_free = 0; >> - >> - for (; pfn < end_pfn; pfn++) { >> - if (!deferred_pfn_valid(pfn)) { >> - deferred_free_range(pfn - nr_free, nr_free); >> - nr_free = 0; >> - } else if (IS_MAX_ORDER_ALIGNED(pfn)) { >> - deferred_free_range(pfn - nr_free, nr_free); >> - nr_free = 1; >> - } else { >> - nr_free++; >> - } >> - } >> - /* Free the last block of pages to allocator */ >> - deferred_free_range(pfn - nr_free, nr_free); >> + if (!deferred_pfn_valid(pfn)) >> + pfn++; >> + >> + deferred_free_range(pfn, end_pfn - pfn); >> } >> >> /* >> @@ -1992,27 +1981,23 @@ static void __init deferred_free_pages(unsigned long pfn, >> * by performing it only once every MAX_ORDER_NR_PAGES. >> * Return number of pages initialized. >> */ >> -static unsigned long __init deferred_init_pages(struct zone *zone, >> - unsigned long pfn, >> - unsigned long end_pfn) >> +static unsigned long __init deferred_init_pages(struct zone *zone, >> + unsigned long pfn, >> + unsigned long end_pfn) >> { >> int nid = zone_to_nid(zone); >> unsigned long nr_pages = 0; >> int zid = zone_idx(zone); >> struct page *page = NULL; >> >> - for (; pfn < end_pfn; pfn++) { >> - if (!deferred_pfn_valid(pfn)) { >> - page = NULL; >> - continue; >> - } else if (!page || IS_MAX_ORDER_ALIGNED(pfn)) { >> - page = pfn_to_page(pfn); >> - } else { >> - page++; >> - } >> + if (!deferred_pfn_valid(pfn)) >> + pfn++; >> + >> + page = pfn_to_page(pfn); >> + nr_pages = end_pfn - pfn; >> + >> + for (; pfn < end_pfn; pfn++, page++) >> __init_single_page(page, pfn, zid, nid); >> - nr_pages++; >> - } >> return nr_pages; >> } >> >> -- >> 2.34.1 >> > >-- >Sincerely yours, >Mike. -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages 2024-06-10 14:54 ` Wei Yang @ 2024-06-11 9:58 ` Mike Rapoport 2024-06-11 14:57 ` Wei Yang 2024-06-12 1:18 ` Wei Yang 0 siblings, 2 replies; 6+ messages in thread From: Mike Rapoport @ 2024-06-11 9:58 UTC (permalink / raw) To: Wei Yang; +Cc: akpm, linux-mm, Kirill A . Shutemov, David Hildenbrand On Mon, Jun 10, 2024 at 02:54:57PM +0000, Wei Yang wrote: > On Mon, Jun 10, 2024 at 09:40:33AM +0300, Mike Rapoport wrote: > >On Wed, Jun 05, 2024 at 01:07:42AM +0000, Wei Yang wrote: > >> Function deferred_[init|free]_pages are only used in > >> deferred_init_maxorder(), which makes sure the range to init/free is > >> within MAX_ORDER_NR_PAGES size. > >> > >> With this knowledge, we can simplify these two functions. Since > >> > >> * only the first pfn could be IS_MAX_ORDER_ALIGNED() > > > >No, the first pfn is not necessarily IS_MAX_ORDER_ALIGNED(). Start pfn is a > >beginning of a region in memblock.memory, and there's no guarantee on it's > >alignment. > > > > Yes, I mean only the first pfn is possible to be IS_MAX_ORDER_ALIGNED(), not > must be IS_MAX_ORDER_ALIGNED(). > > The range passed to deferred_[init|free]_pages must be within one > MAX_ORDER_NR_PAGES. If the first pfn is not IS_MAX_ORDER_ALIGNED(), others > could not be IS_MAX_ORDER_ALIGNED(). > > Currently these two functions would iterate all pfn, and check > IS_MAX_ORDER_ALIGNED() to break init|free on each MAX_ORDER_NR_PAGES. But this > is only possible and necessary on the first pfn. Thinking more about it, it looks to me that deferred_pfn_valid() cannot ever return false. The ranges that are passed to deferred_{free,init}_pages are always from memblock.memory and we allocate the memory map to cover memblock.memory with at least MAX_ORDER_NR_PAGES alignment. So I don't see how pfn_valid() in deferred_pfn_valid() may ever return false. > >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >> CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > >> CC: Mike Rapoport (IBM) <rppt@kernel.org> > >> CC: David Hildenbrand <david@redhat.com> > >> > >> --- > >> But my question is why we just test pfn_valid for the > >> IS_MAX_ORDER_ALIGNED pfn? I thought we should test pfn_valid for each > >> pfn until the first one in MAX_ORDER pages. Do I miss something? > >> --- > >> mm/mm_init.c | 43 ++++++++++++++----------------------------- > >> 1 file changed, 14 insertions(+), 29 deletions(-) > >> > >> diff --git a/mm/mm_init.c b/mm/mm_init.c > >> index bbaf3a2c1cfd..6a4adf9e7d9a 100644 > >> --- a/mm/mm_init.c > >> +++ b/mm/mm_init.c > >> @@ -1970,21 +1970,10 @@ static inline bool __init deferred_pfn_valid(unsigned long pfn) > >> static void __init deferred_free_pages(unsigned long pfn, > >> unsigned long end_pfn) > >> { > >> - unsigned long nr_free = 0; > >> - > >> - for (; pfn < end_pfn; pfn++) { > >> - if (!deferred_pfn_valid(pfn)) { > >> - deferred_free_range(pfn - nr_free, nr_free); > >> - nr_free = 0; > >> - } else if (IS_MAX_ORDER_ALIGNED(pfn)) { > >> - deferred_free_range(pfn - nr_free, nr_free); > >> - nr_free = 1; > >> - } else { > >> - nr_free++; > >> - } > >> - } > >> - /* Free the last block of pages to allocator */ > >> - deferred_free_range(pfn - nr_free, nr_free); > >> + if (!deferred_pfn_valid(pfn)) > >> + pfn++; > >> + > >> + deferred_free_range(pfn, end_pfn - pfn); > >> } > >> > >> /* > >> @@ -1992,27 +1981,23 @@ static void __init deferred_free_pages(unsigned long pfn, > >> * by performing it only once every MAX_ORDER_NR_PAGES. > >> * Return number of pages initialized. > >> */ > >> -static unsigned long __init deferred_init_pages(struct zone *zone, > >> - unsigned long pfn, > >> - unsigned long end_pfn) > >> +static unsigned long __init deferred_init_pages(struct zone *zone, > >> + unsigned long pfn, > >> + unsigned long end_pfn) > >> { > >> int nid = zone_to_nid(zone); > >> unsigned long nr_pages = 0; > >> int zid = zone_idx(zone); > >> struct page *page = NULL; > >> > >> - for (; pfn < end_pfn; pfn++) { > >> - if (!deferred_pfn_valid(pfn)) { > >> - page = NULL; > >> - continue; > >> - } else if (!page || IS_MAX_ORDER_ALIGNED(pfn)) { > >> - page = pfn_to_page(pfn); > >> - } else { > >> - page++; > >> - } > >> + if (!deferred_pfn_valid(pfn)) > >> + pfn++; > >> + > >> + page = pfn_to_page(pfn); > >> + nr_pages = end_pfn - pfn; > >> + > >> + for (; pfn < end_pfn; pfn++, page++) > >> __init_single_page(page, pfn, zid, nid); > >> - nr_pages++; > >> - } > >> return nr_pages; > >> } > >> > >> -- > >> 2.34.1 > >> > > > >-- > >Sincerely yours, > >Mike. > > -- > Wei Yang > Help you, Help me -- Sincerely yours, Mike. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages 2024-06-11 9:58 ` Mike Rapoport @ 2024-06-11 14:57 ` Wei Yang 2024-06-12 1:18 ` Wei Yang 1 sibling, 0 replies; 6+ messages in thread From: Wei Yang @ 2024-06-11 14:57 UTC (permalink / raw) To: Mike Rapoport Cc: Wei Yang, akpm, linux-mm, Kirill A . Shutemov, David Hildenbrand On Tue, Jun 11, 2024 at 12:58:30PM +0300, Mike Rapoport wrote: >On Mon, Jun 10, 2024 at 02:54:57PM +0000, Wei Yang wrote: >> On Mon, Jun 10, 2024 at 09:40:33AM +0300, Mike Rapoport wrote: >> >On Wed, Jun 05, 2024 at 01:07:42AM +0000, Wei Yang wrote: >> >> Function deferred_[init|free]_pages are only used in >> >> deferred_init_maxorder(), which makes sure the range to init/free is >> >> within MAX_ORDER_NR_PAGES size. >> >> >> >> With this knowledge, we can simplify these two functions. Since >> >> >> >> * only the first pfn could be IS_MAX_ORDER_ALIGNED() >> > >> >No, the first pfn is not necessarily IS_MAX_ORDER_ALIGNED(). Start pfn is a >> >beginning of a region in memblock.memory, and there's no guarantee on it's >> >alignment. >> > >> >> Yes, I mean only the first pfn is possible to be IS_MAX_ORDER_ALIGNED(), not >> must be IS_MAX_ORDER_ALIGNED(). >> >> The range passed to deferred_[init|free]_pages must be within one >> MAX_ORDER_NR_PAGES. If the first pfn is not IS_MAX_ORDER_ALIGNED(), others >> could not be IS_MAX_ORDER_ALIGNED(). >> >> Currently these two functions would iterate all pfn, and check >> IS_MAX_ORDER_ALIGNED() to break init|free on each MAX_ORDER_NR_PAGES. But this >> is only possible and necessary on the first pfn. > >Thinking more about it, it looks to me that deferred_pfn_valid() cannot >ever return false. > >The ranges that are passed to deferred_{free,init}_pages are always from >memblock.memory and we allocate the memory map to cover memblock.memory >with at least MAX_ORDER_NR_PAGES alignment. > I need another look into this, forget where it ensure this requirement. >So I don't see how pfn_valid() in deferred_pfn_valid() may ever return >false. > The code is originally intended to work for pageblock, so maybe we can remove this and simplify it more. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> >> CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> >> CC: Mike Rapoport (IBM) <rppt@kernel.org> >> >> CC: David Hildenbrand <david@redhat.com> >> >> >> >> --- >> >> But my question is why we just test pfn_valid for the >> >> IS_MAX_ORDER_ALIGNED pfn? I thought we should test pfn_valid for each >> >> pfn until the first one in MAX_ORDER pages. Do I miss something? >> >> --- >> >> mm/mm_init.c | 43 ++++++++++++++----------------------------- >> >> 1 file changed, 14 insertions(+), 29 deletions(-) >> >> >> >> diff --git a/mm/mm_init.c b/mm/mm_init.c >> >> index bbaf3a2c1cfd..6a4adf9e7d9a 100644 >> >> --- a/mm/mm_init.c >> >> +++ b/mm/mm_init.c >> >> @@ -1970,21 +1970,10 @@ static inline bool __init deferred_pfn_valid(unsigned long pfn) >> >> static void __init deferred_free_pages(unsigned long pfn, >> >> unsigned long end_pfn) >> >> { >> >> - unsigned long nr_free = 0; >> >> - >> >> - for (; pfn < end_pfn; pfn++) { >> >> - if (!deferred_pfn_valid(pfn)) { >> >> - deferred_free_range(pfn - nr_free, nr_free); >> >> - nr_free = 0; >> >> - } else if (IS_MAX_ORDER_ALIGNED(pfn)) { >> >> - deferred_free_range(pfn - nr_free, nr_free); >> >> - nr_free = 1; >> >> - } else { >> >> - nr_free++; >> >> - } >> >> - } >> >> - /* Free the last block of pages to allocator */ >> >> - deferred_free_range(pfn - nr_free, nr_free); >> >> + if (!deferred_pfn_valid(pfn)) >> >> + pfn++; >> >> + >> >> + deferred_free_range(pfn, end_pfn - pfn); >> >> } >> >> >> >> /* >> >> @@ -1992,27 +1981,23 @@ static void __init deferred_free_pages(unsigned long pfn, >> >> * by performing it only once every MAX_ORDER_NR_PAGES. >> >> * Return number of pages initialized. >> >> */ >> >> -static unsigned long __init deferred_init_pages(struct zone *zone, >> >> - unsigned long pfn, >> >> - unsigned long end_pfn) >> >> +static unsigned long __init deferred_init_pages(struct zone *zone, >> >> + unsigned long pfn, >> >> + unsigned long end_pfn) >> >> { >> >> int nid = zone_to_nid(zone); >> >> unsigned long nr_pages = 0; >> >> int zid = zone_idx(zone); >> >> struct page *page = NULL; >> >> >> >> - for (; pfn < end_pfn; pfn++) { >> >> - if (!deferred_pfn_valid(pfn)) { >> >> - page = NULL; >> >> - continue; >> >> - } else if (!page || IS_MAX_ORDER_ALIGNED(pfn)) { >> >> - page = pfn_to_page(pfn); >> >> - } else { >> >> - page++; >> >> - } >> >> + if (!deferred_pfn_valid(pfn)) >> >> + pfn++; >> >> + >> >> + page = pfn_to_page(pfn); >> >> + nr_pages = end_pfn - pfn; >> >> + >> >> + for (; pfn < end_pfn; pfn++, page++) >> >> __init_single_page(page, pfn, zid, nid); >> >> - nr_pages++; >> >> - } >> >> return nr_pages; >> >> } >> >> >> >> -- >> >> 2.34.1 >> >> >> > >> >-- >> >Sincerely yours, >> >Mike. >> >> -- >> Wei Yang >> Help you, Help me > >-- >Sincerely yours, >Mike. -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages 2024-06-11 9:58 ` Mike Rapoport 2024-06-11 14:57 ` Wei Yang @ 2024-06-12 1:18 ` Wei Yang 1 sibling, 0 replies; 6+ messages in thread From: Wei Yang @ 2024-06-12 1:18 UTC (permalink / raw) To: Mike Rapoport Cc: Wei Yang, akpm, linux-mm, Kirill A . Shutemov, David Hildenbrand On Tue, Jun 11, 2024 at 12:58:30PM +0300, Mike Rapoport wrote: >On Mon, Jun 10, 2024 at 02:54:57PM +0000, Wei Yang wrote: >> On Mon, Jun 10, 2024 at 09:40:33AM +0300, Mike Rapoport wrote: >> >On Wed, Jun 05, 2024 at 01:07:42AM +0000, Wei Yang wrote: >> >> Function deferred_[init|free]_pages are only used in >> >> deferred_init_maxorder(), which makes sure the range to init/free is >> >> within MAX_ORDER_NR_PAGES size. >> >> >> >> With this knowledge, we can simplify these two functions. Since >> >> >> >> * only the first pfn could be IS_MAX_ORDER_ALIGNED() >> > >> >No, the first pfn is not necessarily IS_MAX_ORDER_ALIGNED(). Start pfn is a >> >beginning of a region in memblock.memory, and there's no guarantee on it's >> >alignment. >> > >> >> Yes, I mean only the first pfn is possible to be IS_MAX_ORDER_ALIGNED(), not >> must be IS_MAX_ORDER_ALIGNED(). >> >> The range passed to deferred_[init|free]_pages must be within one >> MAX_ORDER_NR_PAGES. If the first pfn is not IS_MAX_ORDER_ALIGNED(), others >> could not be IS_MAX_ORDER_ALIGNED(). >> >> Currently these two functions would iterate all pfn, and check >> IS_MAX_ORDER_ALIGNED() to break init|free on each MAX_ORDER_NR_PAGES. But this >> is only possible and necessary on the first pfn. > >Thinking more about it, it looks to me that deferred_pfn_valid() cannot >ever return false. > >The ranges that are passed to deferred_{free,init}_pages are always from >memblock.memory and we allocate the memory map to cover memblock.memory >with at least MAX_ORDER_NR_PAGES alignment. > We allocate and populate memmap in sparse_init_nid() for each present section. While subsection_map_init() would mark if there is hole in a section. And pfn_valid() would check the subsection bit. The granularity of subsection is 2 ^ 21 = 2M, while MAX_ORDER_NR_PAGES is default to 10. Suppose the default page size is 4K, then MAX_ORDER_NR_PAGES cover a range of 4M. But as you mentioned, the range passed to deferred_{free,init}_pages are from memblock.memory, this means we must allocate the memmap and mark present for its subsection. >So I don't see how pfn_valid() in deferred_pfn_valid() may ever return >false. > Agree. And I think this resolve my previous confusion why just check on the first pfn and don't care about others. Actually, it doesn't matter. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> >> CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> >> CC: Mike Rapoport (IBM) <rppt@kernel.org> >> >> CC: David Hildenbrand <david@redhat.com> >> >> >> >> --- >> >> But my question is why we just test pfn_valid for the >> >> IS_MAX_ORDER_ALIGNED pfn? I thought we should test pfn_valid for each >> >> pfn until the first one in MAX_ORDER pages. Do I miss something? >> >> --- >> >> mm/mm_init.c | 43 ++++++++++++++----------------------------- >> >> 1 file changed, 14 insertions(+), 29 deletions(-) >> >> >> >> diff --git a/mm/mm_init.c b/mm/mm_init.c >> >> index bbaf3a2c1cfd..6a4adf9e7d9a 100644 >> >> --- a/mm/mm_init.c >> >> +++ b/mm/mm_init.c >> >> @@ -1970,21 +1970,10 @@ static inline bool __init deferred_pfn_valid(unsigned long pfn) >> >> static void __init deferred_free_pages(unsigned long pfn, >> >> unsigned long end_pfn) >> >> { >> >> - unsigned long nr_free = 0; >> >> - >> >> - for (; pfn < end_pfn; pfn++) { >> >> - if (!deferred_pfn_valid(pfn)) { >> >> - deferred_free_range(pfn - nr_free, nr_free); >> >> - nr_free = 0; >> >> - } else if (IS_MAX_ORDER_ALIGNED(pfn)) { >> >> - deferred_free_range(pfn - nr_free, nr_free); >> >> - nr_free = 1; >> >> - } else { >> >> - nr_free++; >> >> - } >> >> - } >> >> - /* Free the last block of pages to allocator */ >> >> - deferred_free_range(pfn - nr_free, nr_free); >> >> + if (!deferred_pfn_valid(pfn)) >> >> + pfn++; >> >> + >> >> + deferred_free_range(pfn, end_pfn - pfn); >> >> } >> >> >> >> /* >> >> @@ -1992,27 +1981,23 @@ static void __init deferred_free_pages(unsigned long pfn, >> >> * by performing it only once every MAX_ORDER_NR_PAGES. >> >> * Return number of pages initialized. >> >> */ >> >> -static unsigned long __init deferred_init_pages(struct zone *zone, >> >> - unsigned long pfn, >> >> - unsigned long end_pfn) >> >> +static unsigned long __init deferred_init_pages(struct zone *zone, >> >> + unsigned long pfn, >> >> + unsigned long end_pfn) >> >> { >> >> int nid = zone_to_nid(zone); >> >> unsigned long nr_pages = 0; >> >> int zid = zone_idx(zone); >> >> struct page *page = NULL; >> >> >> >> - for (; pfn < end_pfn; pfn++) { >> >> - if (!deferred_pfn_valid(pfn)) { >> >> - page = NULL; >> >> - continue; >> >> - } else if (!page || IS_MAX_ORDER_ALIGNED(pfn)) { >> >> - page = pfn_to_page(pfn); >> >> - } else { >> >> - page++; >> >> - } >> >> + if (!deferred_pfn_valid(pfn)) >> >> + pfn++; >> >> + >> >> + page = pfn_to_page(pfn); >> >> + nr_pages = end_pfn - pfn; >> >> + >> >> + for (; pfn < end_pfn; pfn++, page++) >> >> __init_single_page(page, pfn, zid, nid); >> >> - nr_pages++; >> >> - } >> >> return nr_pages; >> >> } >> >> >> >> -- >> >> 2.34.1 >> >> >> > >> >-- >> >Sincerely yours, >> >Mike. >> >> -- >> Wei Yang >> Help you, Help me > >-- >Sincerely yours, >Mike. -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-12 1:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-05 1:07 [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages Wei Yang 2024-06-10 6:40 ` Mike Rapoport 2024-06-10 14:54 ` Wei Yang 2024-06-11 9:58 ` Mike Rapoport 2024-06-11 14:57 ` Wei Yang 2024-06-12 1:18 ` Wei Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox