[PATCH 1/2] MM: fixup on addition to bootmem data list

[PATCH 1/2] MM: fixup on addition to bootmem data list

[Gavin Shan]

The objects of "struct bootmem_data_t" are being linked together
to form double-linked list sequentially based on its minimal page
frame number. Current implementation implicitly supports the
following cases, which means the inserting point for current bootmem
data depends on how "list_for_each" works. That makes the code a
little hard to read. Besides, "list_for_each" and "list_entry" can
be replaced with "list_for_each_entry".

	- The linked list is empty.
	- There has no entry in the linked list, whose minimal page
	  frame number is bigger than current one.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 mm/bootmem.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/mm/bootmem.c b/mm/bootmem.c
index 0131170..5a04536 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -77,16 +77,16 @@ unsigned long __init bootmem_bootmap_pages(unsigned long pages)
  */
 static void __init link_bootmem(bootmem_data_t *bdata)
 {
-	struct list_head *iter;
+	bootmem_data_t *ent;
 
-	list_for_each(iter, &bdata_list) {
-		bootmem_data_t *ent;
-
-		ent = list_entry(iter, bootmem_data_t, list);
-		if (bdata->node_min_pfn < ent->node_min_pfn)
-			break;
+	list_for_each_entry(ent, &bdata_list, list) {
+		if (bdata->node_min_pfn < ent->node_min_pfn) {
+			list_add_tail(&bdata->list, &ent->list);
+			return;
+		}
 	}
-	list_add_tail(&bdata->list, iter);
+
+	list_add_tail(&bdata->list, &bdata_list);
 }
 
 /*
-- 
1.7.5.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] MM: check limit while deallocating bootmem node

[Gavin Shan]

For the particular bootmem node, the minimal and maximal PFN (
Page Frame Number) have been traced in the instance of "struct
bootmem_data_t". On current implementation, the maximal PFN isn't
checked while deallocating a bunch (BITS_PER_LONG) of page frames.
So the current implementation won't work if the maximal PFN isn't
aligned with BITS_PER_LONG.

The patch will check the maximal PFN of the given bootmem node.
Also, we needn't check all the bits map when the starting PFN isn't
BITS_PER_LONG aligned. Actually, we should start from the offset
of the bits map, which indicated by the starting PFN.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 mm/bootmem.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/mm/bootmem.c b/mm/bootmem.c
index 5a04536..ebac3ba 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -194,16 +194,20 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
 		 * BITS_PER_LONG block of pages in front of us, free
 		 * it in one go.
 		 */
-		if (IS_ALIGNED(start, BITS_PER_LONG) && vec == ~0UL) {
+		if (end - start >= BITS_PER_LONG &&
+		    IS_ALIGNED(start, BITS_PER_LONG) &&
+		    vec == ~0UL) {
 			int order = ilog2(BITS_PER_LONG);
 
 			__free_pages_bootmem(pfn_to_page(start), order);
 			count += BITS_PER_LONG;
 			start += BITS_PER_LONG;
 		} else {
-			unsigned long off = 0;
+			unsigned long cursor = start;
+			unsigned long off = cursor & (BITS_PER_LONG - 1);
 
-			while (vec && off < BITS_PER_LONG) {
+			vec >>= off;
+			while (vec && off < BITS_PER_LONG && cursor < end) {
 				if (vec & 1) {
 					page = pfn_to_page(start + off);
 					__free_pages_bootmem(page, 0);
@@ -211,6 +215,7 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
 				}
 				vec >>= 1;
 				off++;
+				cursor++;
 			}
 			start = ALIGN(start + 1, BITS_PER_LONG);
 		}
-- 
1.7.5.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] MM: check limit while deallocating bootmem node

[Johannes Weiner]

On Fri, Apr 27, 2012 at 11:41:44AM +0800, Gavin Shan wrote:
> For the particular bootmem node, the minimal and maximal PFN (
> Page Frame Number) have been traced in the instance of "struct
> bootmem_data_t". On current implementation, the maximal PFN isn't
> checked while deallocating a bunch (BITS_PER_LONG) of page frames.
> So the current implementation won't work if the maximal PFN isn't
> aligned with BITS_PER_LONG.
>
> The patch will check the maximal PFN of the given bootmem node.
> Also, we needn't check all the bits map when the starting PFN isn't
> BITS_PER_LONG aligned. Actually, we should start from the offset
> of the bits map, which indicated by the starting PFN.
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  mm/bootmem.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index 5a04536..ebac3ba 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -194,16 +194,20 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>  		 * BITS_PER_LONG block of pages in front of us, free
>  		 * it in one go.
>  		 */
> -		if (IS_ALIGNED(start, BITS_PER_LONG) && vec == ~0UL) {
> +		if (end - start >= BITS_PER_LONG &&
> +		    IS_ALIGNED(start, BITS_PER_LONG) &&
> +		    vec == ~0UL) {

Did you have any actual problems with the code or was this just by
review?

vec has bits set for unreserved pages and the bitmap is aligned and
reserved per default.  So if the chunk is smaller than (end - start),
then vec is already != ~0UL.  The check you add should be redundant.

>  			int order = ilog2(BITS_PER_LONG);
>  
>  			__free_pages_bootmem(pfn_to_page(start), order);
>  			count += BITS_PER_LONG;
>  			start += BITS_PER_LONG;
>  		} else {
> -			unsigned long off = 0;
> +			unsigned long cursor = start;
> +			unsigned long off = cursor & (BITS_PER_LONG - 1);
>  
> -			while (vec && off < BITS_PER_LONG) {
> +			vec >>= off;
> +			while (vec && off < BITS_PER_LONG && cursor < end) {

Optimization looks ok, although I doubt it makes a notable difference,
this case should be pretty rare.

Also, if you reach end, vec has no more bits set, so the cursor < end
check should again be redundant.  I think we can also remove the
off < BITS_PER_LONG, there can hardly be more than BITS_PER_LONG
set bits in vec.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] MM: check limit while deallocating bootmem node

[Gavin Shan]

>> For the particular bootmem node, the minimal and maximal PFN (
>> Page Frame Number) have been traced in the instance of "struct
>> bootmem_data_t". On current implementation, the maximal PFN isn't
>> checked while deallocating a bunch (BITS_PER_LONG) of page frames.
>> So the current implementation won't work if the maximal PFN isn't
>> aligned with BITS_PER_LONG.
>>
>> The patch will check the maximal PFN of the given bootmem node.
>> Also, we needn't check all the bits map when the starting PFN isn't
>> BITS_PER_LONG aligned. Actually, we should start from the offset
>> of the bits map, which indicated by the starting PFN.
>> 
>> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>> ---
>>  mm/bootmem.c |   11 ++++++++---
>>  1 files changed, 8 insertions(+), 3 deletions(-)
>> 
>> diff --git a/mm/bootmem.c b/mm/bootmem.c
>> index 5a04536..ebac3ba 100644
>> --- a/mm/bootmem.c
>> +++ b/mm/bootmem.c
>> @@ -194,16 +194,20 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>>  		 * BITS_PER_LONG block of pages in front of us, free
>>  		 * it in one go.
>>  		 */
>> -		if (IS_ALIGNED(start, BITS_PER_LONG) && vec == ~0UL) {
>> +		if (end - start >= BITS_PER_LONG &&
>> +		    IS_ALIGNED(start, BITS_PER_LONG) &&
>> +		    vec == ~0UL) {
>
>Did you have any actual problems with the code or was this just by
>review?
>

I just got this for review. There're no real problem against it ;-)

>vec has bits set for unreserved pages and the bitmap is aligned and
>reserved per default.  So if the chunk is smaller than (end - start),
>then vec is already != ~0UL.  The check you add should be redundant.
>

Yes. I'll remove the duplicate check in next revision.

>>  			int order = ilog2(BITS_PER_LONG);
>>  
>>  			__free_pages_bootmem(pfn_to_page(start), order);
>>  			count += BITS_PER_LONG;
>>  			start += BITS_PER_LONG;
>>  		} else {
>> -			unsigned long off = 0;
>> +			unsigned long cursor = start;
>> +			unsigned long off = cursor & (BITS_PER_LONG - 1);
>>  
>> -			while (vec && off < BITS_PER_LONG) {
>> +			vec >>= off;
>> +			while (vec && off < BITS_PER_LONG && cursor < end) {
>
>Optimization looks ok, although I doubt it makes a notable difference,
>this case should be pretty rare.
>
>Also, if you reach end, vec has no more bits set, so the cursor < end
>check should again be redundant.  I think we can also remove the
>off < BITS_PER_LONG, there can hardly be more than BITS_PER_LONG
>set bits in vec.
>

Yes. I'll change that into "while (vec)" in next revision.

Thanks,
Gavin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] MM: check limit while deallocating bootmem node

[Gavin Shan]

Hi Johannes,

Could you take a look on this while you have available time?

Thanks,
Gavin

>For the particular bootmem node, the minimal and maximal PFN (
>Page Frame Number) have been traced in the instance of "struct
>bootmem_data_t". On current implementation, the maximal PFN isn't
>checked while deallocating a bunch (BITS_PER_LONG) of page frames.
>So the current implementation won't work if the maximal PFN isn't
>aligned with BITS_PER_LONG.
>
>The patch will check the maximal PFN of the given bootmem node.
>Also, we needn't check all the bits map when the starting PFN isn't
>BITS_PER_LONG aligned. Actually, we should start from the offset
>of the bits map, which indicated by the starting PFN.
>
>Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>---
> mm/bootmem.c |   11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
>diff --git a/mm/bootmem.c b/mm/bootmem.c
>index 5a04536..ebac3ba 100644
>--- a/mm/bootmem.c
>+++ b/mm/bootmem.c
>@@ -194,16 +194,20 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
> 		 * BITS_PER_LONG block of pages in front of us, free
> 		 * it in one go.
> 		 */
>-		if (IS_ALIGNED(start, BITS_PER_LONG) && vec == ~0UL) {
>+		if (end - start >= BITS_PER_LONG &&
>+		    IS_ALIGNED(start, BITS_PER_LONG) &&
>+		    vec == ~0UL) {
> 			int order = ilog2(BITS_PER_LONG);
>
> 			__free_pages_bootmem(pfn_to_page(start), order);
> 			count += BITS_PER_LONG;
> 			start += BITS_PER_LONG;
> 		} else {
>-			unsigned long off = 0;
>+			unsigned long cursor = start;
>+			unsigned long off = cursor & (BITS_PER_LONG - 1);
>
>-			while (vec && off < BITS_PER_LONG) {
>+			vec >>= off;
>+			while (vec && off < BITS_PER_LONG && cursor < end) {
> 				if (vec & 1) {
> 					page = pfn_to_page(start + off);
> 					__free_pages_bootmem(page, 0);
>@@ -211,6 +215,7 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
> 				}
> 				vec >>= 1;
> 				off++;
>+				cursor++;
> 			}
> 			start = ALIGN(start + 1, BITS_PER_LONG);
> 		}
>-- 
>1.7.5.4
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] MM: check limit while deallocating bootmem node

[Gavin Shan]

>Hi Johannes,
>
>Could you take a look on this while you have available time?
>

It's my bad. It should be this one:

[PATCH 1/2] MM: fixup on addition to bootmem data list

>Thanks,
>Gavin
>
>>For the particular bootmem node, the minimal and maximal PFN (
>>Page Frame Number) have been traced in the instance of "struct
>>bootmem_data_t". On current implementation, the maximal PFN isn't
>>checked while deallocating a bunch (BITS_PER_LONG) of page frames.
>>So the current implementation won't work if the maximal PFN isn't
>>aligned with BITS_PER_LONG.
>>
>>The patch will check the maximal PFN of the given bootmem node.
>>Also, we needn't check all the bits map when the starting PFN isn't
>>BITS_PER_LONG aligned. Actually, we should start from the offset
>>of the bits map, which indicated by the starting PFN.
>>
>>Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>---
>> mm/bootmem.c |   11 ++++++++---
>> 1 files changed, 8 insertions(+), 3 deletions(-)
>>
>>diff --git a/mm/bootmem.c b/mm/bootmem.c
>>index 5a04536..ebac3ba 100644
>>--- a/mm/bootmem.c
>>+++ b/mm/bootmem.c
>>@@ -194,16 +194,20 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>> 		 * BITS_PER_LONG block of pages in front of us, free
>> 		 * it in one go.
>> 		 */
>>-		if (IS_ALIGNED(start, BITS_PER_LONG) && vec == ~0UL) {
>>+		if (end - start >= BITS_PER_LONG &&
>>+		    IS_ALIGNED(start, BITS_PER_LONG) &&
>>+		    vec == ~0UL) {
>> 			int order = ilog2(BITS_PER_LONG);
>>
>> 			__free_pages_bootmem(pfn_to_page(start), order);
>> 			count += BITS_PER_LONG;
>> 			start += BITS_PER_LONG;
>> 		} else {
>>-			unsigned long off = 0;
>>+			unsigned long cursor = start;
>>+			unsigned long off = cursor & (BITS_PER_LONG - 1);
>>
>>-			while (vec && off < BITS_PER_LONG) {
>>+			vec >>= off;
>>+			while (vec && off < BITS_PER_LONG && cursor < end) {
>> 				if (vec & 1) {
>> 					page = pfn_to_page(start + off);
>> 					__free_pages_bootmem(page, 0);
>>@@ -211,6 +215,7 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>> 				}
>> 				vec >>= 1;
>> 				off++;
>>+				cursor++;
>> 			}
>> 			start = ALIGN(start + 1, BITS_PER_LONG);
>> 		}
>>-- 
>>1.7.5.4
>>
>
>--
>To unsubscribe, send a message with 'unsubscribe linux-mm' in
>the body to majordomo@kvack.org.  For more info on Linux MM,
>see: http://www.linux-mm.org/ .
>Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
>Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] MM: fixup on addition to bootmem data list

[Johannes Weiner]

On Fri, Apr 27, 2012 at 11:41:43AM +0800, Gavin Shan wrote:
> The objects of "struct bootmem_data_t" are being linked together
> to form double-linked list sequentially based on its minimal page
> frame number. Current implementation implicitly supports the
> following cases, which means the inserting point for current bootmem
> data depends on how "list_for_each" works. That makes the code a
> little hard to read. Besides, "list_for_each" and "list_entry" can
> be replaced with "list_for_each_entry".
> 
> 	- The linked list is empty.
> 	- There has no entry in the linked list, whose minimal page
> 	  frame number is bigger than current one.
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  mm/bootmem.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index 0131170..5a04536 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -77,16 +77,16 @@ unsigned long __init bootmem_bootmap_pages(unsigned long pages)
>   */
>  static void __init link_bootmem(bootmem_data_t *bdata)
>  {
> -	struct list_head *iter;
> +	bootmem_data_t *ent;
>  
> -	list_for_each(iter, &bdata_list) {
> -		bootmem_data_t *ent;
> -
> -		ent = list_entry(iter, bootmem_data_t, list);
> -		if (bdata->node_min_pfn < ent->node_min_pfn)
> -			break;
> +	list_for_each_entry(ent, &bdata_list, list) {
> +		if (bdata->node_min_pfn < ent->node_min_pfn) {
> +			list_add_tail(&bdata->list, &ent->list);
> +			return;
> +		}
>  	}
> -	list_add_tail(&bdata->list, iter);
> +
> +	list_add_tail(&bdata->list, &bdata_list);

Yes, this is better, thanks.

Would you care to fix up the patch subject (it's a cleanup, not a fix)
and send it on to Andrew Morton?  You can include

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] MM: fixup on addition to bootmem data list

[Gavin Shan]

>> The objects of "struct bootmem_data_t" are being linked together
>> to form double-linked list sequentially based on its minimal page
>> frame number. Current implementation implicitly supports the
>> following cases, which means the inserting point for current bootmem
>> data depends on how "list_for_each" works. That makes the code a
>> little hard to read. Besides, "list_for_each" and "list_entry" can
>> be replaced with "list_for_each_entry".
>> 
>> 	- The linked list is empty.
>> 	- There has no entry in the linked list, whose minimal page
>> 	  frame number is bigger than current one.
>> 
>> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>> ---
>>  mm/bootmem.c |   16 ++++++++--------
>>  1 files changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/mm/bootmem.c b/mm/bootmem.c
>> index 0131170..5a04536 100644
>> --- a/mm/bootmem.c
>> +++ b/mm/bootmem.c
>> @@ -77,16 +77,16 @@ unsigned long __init bootmem_bootmap_pages(unsigned long pages)
>>   */
>>  static void __init link_bootmem(bootmem_data_t *bdata)
>>  {
>> -	struct list_head *iter;
>> +	bootmem_data_t *ent;
>>  
>> -	list_for_each(iter, &bdata_list) {
>> -		bootmem_data_t *ent;
>> -
>> -		ent = list_entry(iter, bootmem_data_t, list);
>> -		if (bdata->node_min_pfn < ent->node_min_pfn)
>> -			break;
>> +	list_for_each_entry(ent, &bdata_list, list) {
>> +		if (bdata->node_min_pfn < ent->node_min_pfn) {
>> +			list_add_tail(&bdata->list, &ent->list);
>> +			return;
>> +		}
>>  	}
>> -	list_add_tail(&bdata->list, iter);
>> +
>> +	list_add_tail(&bdata->list, &bdata_list);
>
>Yes, this is better, thanks.
>
>Would you care to fix up the patch subject (it's a cleanup, not a fix)
>and send it on to Andrew Morton?  You can include
>
>Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>

Thanks, Johannes. I'll do it ;-)

>--
>To unsubscribe, send a message with 'unsubscribe linux-mm' in
>the body to majordomo@kvack.org.  For more info on Linux MM,
>see: http://www.linux-mm.org/ .
>Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
>Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>