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@xxxxxxxxx>
>> CC: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
>> CC: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
>> CC: David Hildenbrand <david@xxxxxxxxxx>
>>
>> ---
>> 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
