Re: [RFC] memblock; Properly handle overlaps

[Yinghai Lu <yinghai@kernel.org>]

On 03/05/2011 01:37 PM, Benjamin Herrenschmidt wrote:
> On Sat, 2011-03-05 at 11:14 -0800, Yinghai Lu wrote:
>> On 03/04/2011 11:56 PM, Benjamin Herrenschmidt wrote:
>>>>
>>>> did you try remove and add tricks?
>>>
>>> Yes, and it's a band-wait on top of a wooden leg... (didn't even work
>>> properly for some real cases I hit with bad FW data, ended up with two
>>> regions once reserving a portion of the previous one). It doesn't take
>>> long starting at the implementation of remove() to understand why :-)
>>>
>>> Also, if something like that happens, you expose yourself to rampant
>>> corruption and other very hard to debug problems, because nothing will
>>> tell you that the array is corrupted (no longer a monotonic progression)
>>> and you might get overlapping allocations, allocations spanning reserved
>>> regions etc... all silently.
>>>
>>> I think the whole thing was long overdue for an overhaul. Hopefully, my
>>> new code is -much- more robust under all circumstances of full overlap,
>>> partial overlap, freeing entire regions with multiple blocks in them or
>>> reserving regions with multiple holes, etc...
>>>
>>> Note that my patch really only rewrite those two low level functions
>>> (add and remove of a region to a list), so it's reasonably contained and
>>> should be easy to audit.
>>>
>>> I want to spend a bit more time next week throwing at my userspace
>>> version some nasty test cases involving non-coalesce boundaries, and
>>> once that's done, and unless I have some massive bug I haven't seen, I
>>> think we should just merge the patch.
>>
>> please check changes on top your patch regarding memblock_add_region
> 
> Can you reply inline next to the respective code ? It would make things
> easier :-)
> 
>> 1. after check with bottom, we need to update the size. otherwise when we
>> checking with top, we could use wrong size, and increase to extra big.
> 
> You mean adding this ?
> 
> 			/* We continue processing from the end of the
> 			 * coalesced block.
> 			 */
> 			base = rgn->base + rgn->size;
> + 			size = end - base;
> 
> I suppose you are right. Interestingly enough I haven't trigged that in
> my tests, I'll add an specific scenario to trigger that problem.
> 

yes. in addition to that, still need to move in base >= end into the previous if block.
because only that place upste base, and also me need to make sure end >= start before using
them to get fize.

>> @@ -330,11 +321,17 @@ static long __init_memblock memblock_add
>>  			 * coalesced block.
>>  			 */
>>  			base = rgn->base + rgn->size;
>> -		}
>>  
>> -		/* Check if e have nothing else to allocate (fully coalesced) */
>> -		if (base >= end)
>> -			return 0;
>> +			/*
>> +			 * Check if We have nothing else to allocate
>> +			 * (fully coalesced)
>> +			 */
>> +			if (base >= end)
>> +				return 0;
>> +
>> +			/* Update left over size */
>> +			size = end - base;
>> +		}
>>  
>>  		/* Now check if we overlap or are adjacent with the
>>  		 * top of a block

Thanks

Yinghai