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

[Wei Yang <weiyang@linux.vnet.ibm.com>]

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>