diff mbox series

memblock: remove repeat round

Message ID 20221019120337.2098298-1-yajun.deng@linux.dev
State New
Headers
Series memblock: remove repeat round |

Commit Message

Yajun Deng Oct. 19, 2022, 12:03 p.m. UTC 
  There is no need round twice in memblock_add_range().

We can call memblock_double_array() to extand the size if type->cnt no
less than type->max before memblock_insert_region(), otherwise we can
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(-)

Comments

Mike Rapoport Oct. 24, 2022, 4:15 p.m. UTC | #1 
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

> 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/

> 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?
  
> @@ -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.

> +				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.

> +	return 0;
>  }
>  
>  /**
> -- 
> 2.25.1
> 
>
Yajun Deng Oct. 25, 2022, 3:24 a.m. UTC | #2 
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.
Mike Rapoport Oct. 26, 2022, 6:04 a.m. UTC | #3 
On Tue, Oct 25, 2022 at 03:24:58AM +0000, Yajun Deng wrote:
> October 25, 2022 12:15 AM, "Mike Rapoport" <rppt@kernel.org> wrote:
> 
> >> @@ -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.

Why?
Yajun Deng Oct. 26, 2022, 6:18 a.m. UTC | #4 
October 26, 2022 2:04 PM, "Mike Rapoport" <rppt@kernel.org> wrote:

> On Tue, Oct 25, 2022 at 03:24:58AM +0000, Yajun Deng wrote:
> 
>> October 25, 2022 12:15 AM, "Mike Rapoport" <rppt@kernel.org> wrote:
>> 
>> @@ -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.
> 
> Why?
> 
Sorry, nr_new was removed, and added ocnt variable for the original of type->cnt.

I already sent another patch with '[PATCH v2] memblock: don't run loop in 
memblock_add_range() twice' subject, you can see that.

> --
> Sincerely yours,
> Mike.
diff mbox series

Patch

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;
 
@@ -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))
+				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);
+	return 0;
 }
 
 /**