Re: [PATCH] memblock: remove repeat round

["Yajun Deng" <yajun.deng@linux.dev>]

October 25, 2022 12:15 AM, "Mike Rapoport" <rppt@kernel.org> wrote:

> Hi,
> 
> On Wed, Oct 19, 2022 at 08:03:37PM +0800, Yajun Deng wrote:
> 
>> Subject: memblock: remove repeat round
> 
> Please make the patch subject more detailed. Say
> 
> membloc: don't run loop in memblock_add_range() twice
> 

Okay!
>> There is no need round twice in memblock_add_range().
>> 
>> We can call memblock_double_array() to extand the size if type->cnt no
> 
> ^ extend
> 
>> less than type->max before memblock_insert_region(), otherwise we can
> 
> s/no less than/greater or equal to/
> 

Got it.
>> insert the new region directly.
>> 
>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
>> ---
>> mm/memblock.c | 54 +++++++++++++++------------------------------------
>> 1 file changed, 16 insertions(+), 38 deletions(-)
>> 
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index 511d4783dcf1..1679244b4a1a 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -578,7 +578,6 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>> phys_addr_t base, phys_addr_t size,
>> int nid, enum memblock_flags flags)
>> {
>> - bool insert = false;
>> phys_addr_t obase = base;
>> phys_addr_t end = base + memblock_cap_size(base, &size);
>> int idx, nr_new;
>> @@ -598,22 +597,6 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>> return 0;
>> }
>> 
>> - /*
>> - * The worst case is when new range overlaps all existing regions,
>> - * then we'll need type->cnt + 1 empty regions in @type. So if
>> - * type->cnt * 2 + 1 is less than type->max, we know
>> - * that there is enough empty regions in @type, and we can insert
>> - * regions directly.
>> - */
>> - if (type->cnt * 2 + 1 < type->max)
>> - insert = true;
>> -
>> -repeat:
>> - /*
>> - * The following is executed twice. Once with %false @insert and
>> - * then with %true. The first counts the number of regions needed
>> - * to accommodate the new area. The second actually inserts them.
>> - */
>> base = obase;
>> nr_new = 0;
> 
> I believe nr_new variable is no longer needed, is it?
> 
No, nr_new is needed before memblock_merge_regions() for return.

>> @@ -635,10 +618,14 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>> #endif
>> WARN_ON(flags != rgn->flags);
>> nr_new++;
>> - if (insert)
>> - memblock_insert_region(type, idx++, base,
>> - rbase - base, nid,
>> - flags);
>> +
>> + if ((type->cnt >= type->max) &&
>> + (memblock_double_array(type, obase, size) < 0))
> 
> if ((type->cnt >= type->max) &&
> memblock_double_array(type, obase, size))
> 
> would be just fine.
> 
> I'd appreciate a comment above the if statement explaining when the
> allocation is required.
> 
Got it.

>> + return -ENOMEM;
>> +
>> + memblock_insert_region(type, idx++, base,
>> + rbase - base, nid,
>> + flags);
>> }
>> /* area below @rend is dealt with, forget about it */
>> base = min(rend, end);
>> @@ -647,28 +634,19 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>> /* insert the remaining portion */
>> if (base < end) {
>> nr_new++;
>> - if (insert)
>> - memblock_insert_region(type, idx, base, end - base,
>> - nid, flags);
>> + if ((type->cnt >= type->max) &&
>> + (memblock_double_array(type, obase, size) < 0))
>> + return -ENOMEM;
>> +
>> + memblock_insert_region(type, idx, base, end - base,
>> + nid, flags);
>> }
>> 
>> if (!nr_new)
>> return 0;
>> 
>> - /*
>> - * If this was the first round, resize array and repeat for actual
>> - * insertions; otherwise, merge and return.
>> - */
>> - if (!insert) {
>> - while (type->cnt + nr_new > type->max)
>> - if (memblock_double_array(type, obase, size) < 0)
>> - return -ENOMEM;
>> - insert = true;
>> - goto repeat;
>> - } else {
>> - memblock_merge_regions(type);
>> - return 0;
>> - }
>> + memblock_merge_regions(type);
> 
> A blank line here would be nice.
> 
Got it.

>> + return 0;
>> }
>> 
>> /**
>> --
>> 2.25.1
> 
> --
> Sincerely yours,
> Mike.