[PATCH v2 2/2] lmb: Fix adjacent region merge in lmb_add_region_flags()

[Patrice CHOTARD]

On 4/14/24 13:10, Kumar, Udit wrote:
> Hello Patrice,
> 
> On 4/13/2024 1:54 PM, Patrice CHOTARD wrote:
>>
>> On 4/12/24 17:53, Patrice Chotard wrote:
>>> In case a new region is adjacent to a previous region with
>>> similar flag, this region is merged with its predecessor, but no
>>> check are done if this new added region is overlapping another region
>>> present in lmb (see reserved[3] which overlaps reserved[4]).
>>>
>>> [..]
>>> phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align)
>>>   {
>>>       return lmb_alloc_base(lmb, size, align, LMB_ALLOC_ANYWHERE);
>>
>> I think this series (v2) is not correct even if now the CI tests are OK.
>> After re-reading carefully the lib_test_lmb_overlapping_reserve() test
>> it appears to me there is a contradiction.
>>
>> It's indicating that "check that calling lmb_reserve with overlapping regions fails"
>>
>> but the very last test of lib_test_lmb_overlapping_reserve() has this comment :
>> /* allocate 3rd region, coalesce with first and overlap with second */
>> and this test allows this overlap case.
>>
>> It's not clear if LMB region can overlap each other or not ?
> 
> 
> I would say partial overlap and coalescing with before one
> 
> May be Below can help
> 
> /* allocate 2nd region , This should coalesced all region into one
> 
> you will get one region as
> 
> Address --- Size
> 
> 0x40010000 --- 0x30000
> 
> Next after this  /* allocate 2nd region, which should be added as first region */
> 
> we will have two region like
> 
> Address --- Size
> 
> (0x40000000 -- 0x8000)
> 
> (0x40010000 --- 0x30000)
> 
> Now third request comes in
> 
> /* allocate 3rd region, coalesce with first and overlap with second */
> 
> which is address of  0x40008000 and size of  0x10000, Now this region to be added
> 
> is coalescing with first (0x40000000 -- 0x8000) and part of this overlap with (0x40010000 --- 0x30000).
> 
> So, what this patch does , merge all these into one region
> 
> as (0x40000000 -- 0x40000)

Hi Udit

Ok, but why the second test done in test/lib/lmb.c test should be considered as failed ?


	ret = lmb_reserve(&lmb, 0x40010000, 0x10000);
	ut_asserteq(ret, 0);
	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
		   0, 0, 0, 0);
	/* allocate overlapping region should fail */
	ret = lmb_reserve(&lmb, 0x40011000, 0x10000);
	ut_asserteq(ret, -1);


The 2 area 0x40010000 -- 0x10000   and 0x40011000 -- 0x10000 area overlaps each other and 
should be merged in one 0x40010000 -- 11000 ?

What is the criteria to merge or not 2 overlapping areas ?

For me overlapping shouldn't be authorized.

Patrice

> 
>> Udit, your patch edb5824be17f ("lmb: remove overlapping region with next range")
>> is authorizing LMB overlapping right ?
> 
> As said before this is checking overlap and coalescing and acting accordingly.
> 
>> Patrice
>>
>>
>>
>>
>>