* [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