[PATCH] mm/memblock: WARN_ON when nid differs from overlap region

[PATCH] mm/memblock: WARN_ON when nid differs from overlap region

[Wei Yang]

Each memblock_region has nid to indicates the Node ID of this range. For
the overlap case, memblock_add_range() inserts the lower part and leave the
upper part as indicated in the overlapped region.

If the nid of the new range differs from the overlapped region, the
information recorded is not correct.

This patch adds a WARN_ON when the nid of the new range differs from the
overlapped region.

---

I am not familiar with the lower level topology, maybe this case will not
happen. 

If current implementation is based on the assumption, that overlapped
ranges' nid and flags are the same, I would suggest to add a comment to
indicates this background.

If the assumption is not correct, I suggest to add a WARN_ON or BUG_ON to
indicates this case.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 mm/memblock.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/memblock.c b/mm/memblock.c
index 9318b56..09efe70 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -540,6 +540,9 @@ repeat:
 		 * area, insert that portion.
 		 */
 		if (rbase > base) {
+#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
+			WARN_ON(nid != memblock_get_region_node(rgn));
+#endif
 			nr_new++;
 			if (insert)
 				memblock_insert_region(type, i++, base,
-- 
1.7.9.5

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memblock: WARN_ON when nid differs from overlap region

[David Rientjes]

On Wed, 8 Jul 2015, Wei Yang wrote:

> Each memblock_region has nid to indicates the Node ID of this range. For
> the overlap case, memblock_add_range() inserts the lower part and leave the
> upper part as indicated in the overlapped region.
> 
> If the nid of the new range differs from the overlapped region, the
> information recorded is not correct.
> 
> This patch adds a WARN_ON when the nid of the new range differs from the
> overlapped region.
> 
> ---
> 
> I am not familiar with the lower level topology, maybe this case will not
> happen. 
> 
> If current implementation is based on the assumption, that overlapped
> ranges' nid and flags are the same, I would suggest to add a comment to
> indicates this background.
> 
> If the assumption is not correct, I suggest to add a WARN_ON or BUG_ON to
> indicates this case.
> 
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
>  mm/memblock.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 9318b56..09efe70 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -540,6 +540,9 @@ repeat:
>  		 * area, insert that portion.
>  		 */
>  		if (rbase > base) {
> +#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> +			WARN_ON(nid != memblock_get_region_node(rgn));
> +#endif
>  			nr_new++;
>  			if (insert)
>  				memblock_insert_region(type, i++, base,

I think the assertion that nid should match memblock_get_region_node() of 
the overlapped region is correct.  It only functionally makes a difference 
if insert == true, but I don't think there's harm in verifying it 
regardless.

Acked-by: David Rientjes <rientjes@google.com>

I think your supplemental to the changelog suggests that you haven't seen 
this actually occur, but in the off chance that you have then it would be 
interesting to see 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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memblock: WARN_ON when nid differs from overlap region

[Wei Yang]

On Wed, Jul 08, 2015 at 05:54:18PM -0700, David Rientjes wrote:
>On Wed, 8 Jul 2015, Wei Yang wrote:
>
>> Each memblock_region has nid to indicates the Node ID of this range. For
>> the overlap case, memblock_add_range() inserts the lower part and leave the
>> upper part as indicated in the overlapped region.
>> 
>> If the nid of the new range differs from the overlapped region, the
>> information recorded is not correct.
>> 
>> This patch adds a WARN_ON when the nid of the new range differs from the
>> overlapped region.
>> 
>> ---
>> 
>> I am not familiar with the lower level topology, maybe this case will not
>> happen. 
>> 
>> If current implementation is based on the assumption, that overlapped
>> ranges' nid and flags are the same, I would suggest to add a comment to
>> indicates this background.
>> 
>> If the assumption is not correct, I suggest to add a WARN_ON or BUG_ON to
>> indicates this case.
>> 
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  mm/memblock.c |    3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index 9318b56..09efe70 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -540,6 +540,9 @@ repeat:
>>  		 * area, insert that portion.
>>  		 */
>>  		if (rbase > base) {
>> +#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
>> +			WARN_ON(nid != memblock_get_region_node(rgn));
>> +#endif
>>  			nr_new++;
>>  			if (insert)
>>  				memblock_insert_region(type, i++, base,
>
>I think the assertion that nid should match memblock_get_region_node() of 
>the overlapped region is correct.  It only functionally makes a difference 
>if insert == true, but I don't think there's harm in verifying it 
>regardless.
>
>Acked-by: David Rientjes <rientjes@google.com>
>
>I think your supplemental to the changelog suggests that you haven't seen 
>this actually occur, but in the off chance that you have then it would be 
>interesting to see it.

Hi David,

Thanks for your comments.

Yes, I don't see this actually occur. This is a guard to indicates if the
lower level hardware is not functioning well.

Also, as the supplemental in the change log mentioned, the flags of the
overlapped region needs to be checked too. If you think this is proper, I
would like to form a patch to have the assertion on the flags. 

-- 
Richard Yang
Help you, Help me

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm/memblock: WARN_ON when flags differs from overlap region

[Wei Yang]

Each memblock_region has flags to indicates the Node ID of this range. For
the overlap case, memblock_add_range() inserts the lower part and leave the
upper part as indicated in the overlapped region.

If the flags of the new range differs from the overlapped region, the
information recorded is not correct.

This patch adds a WARN_ON when the flags of the new range differs from the
overlapped region.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 mm/memblock.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memblock.c b/mm/memblock.c
index 95ce68c..bde61e8 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -569,6 +569,7 @@ repeat:
 #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
 			WARN_ON(nid != memblock_get_region_node(rgn));
 #endif
+			WARN_ON(flags != rgn->flags);
 			nr_new++;
 			if (insert)
 				memblock_insert_region(type, i++, base,
-- 
1.7.9.5

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memblock: WARN_ON when flags differs from overlap region

[David Rientjes]

On Sat, 11 Jul 2015, Wei Yang wrote:

> Each memblock_region has flags to indicates the Node ID of this range. For
> the overlap case, memblock_add_range() inserts the lower part and leave the
> upper part as indicated in the overlapped region.
> 

Memblock region flags do not specify node ids, so this is somewhat 
misleading.

> If the flags of the new range differs from the overlapped region, the
> information recorded is not correct.
> 
> This patch adds a WARN_ON when the flags of the new range differs from the
> overlapped region.
> 
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
>  mm/memblock.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 95ce68c..bde61e8 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -569,6 +569,7 @@ repeat:
>  #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
>  			WARN_ON(nid != memblock_get_region_node(rgn));
>  #endif
> +			WARN_ON(flags != rgn->flags);
>  			nr_new++;
>  			if (insert)
>  				memblock_insert_region(type, i++, base,

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memblock: WARN_ON when flags differs from overlap region

[Wei Yang]

On Wed, Jul 15, 2015 at 05:19:39PM -0700, David Rientjes wrote:
>On Sat, 11 Jul 2015, Wei Yang wrote:
>
>> Each memblock_region has flags to indicates the Node ID of this range. For
>> the overlap case, memblock_add_range() inserts the lower part and leave the
>> upper part as indicated in the overlapped region.
>> 
>
>Memblock region flags do not specify node ids, so this is somewhat 
>misleading.
>

Thanks for pointing out, the commit message is not correct.

It should be "type" instead of "Node ID".

>> If the flags of the new range differs from the overlapped region, the
>> information recorded is not correct.
>> 
>> This patch adds a WARN_ON when the flags of the new range differs from the
>> overlapped region.
>> 
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  mm/memblock.c |    1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index 95ce68c..bde61e8 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -569,6 +569,7 @@ repeat:
>>  #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
>>  			WARN_ON(nid != memblock_get_region_node(rgn));
>>  #endif
>> +			WARN_ON(flags != rgn->flags);
>>  			nr_new++;
>>  			if (insert)
>>  				memblock_insert_region(type, i++, base,

-- 
Richard Yang
Help you, Help me

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH V2] mm/memblock: WARN_ON when flags differs from overlap region

[Wei Yang]

Each memblock_region has flags to indicates the type of this range. For
the overlap case, memblock_add_range() inserts the lower part and leave the
upper part as indicated in the overlapped region.

If the flags of the new range differs from the overlapped region, the
information recorded is not correct.

This patch adds a WARN_ON when the flags of the new range differs from the
overlapped region.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>

---
v2:
    * change the commit log to be more accurate.
---
 mm/memblock.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memblock.c b/mm/memblock.c
index 95ce68c..bde61e8 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -569,6 +569,7 @@ repeat:
 #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
 			WARN_ON(nid != memblock_get_region_node(rgn));
 #endif
+			WARN_ON(flags != rgn->flags);
 			nr_new++;
 			if (insert)
 				memblock_insert_region(type, i++, base,
-- 
1.7.9.5

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>