Fix block allocation size (IDFGH-13453) #2
Conversation
It needs to include the rounding done by `mapping_search()` otherwise we'll trim the block too much and not be able to re-allocate the same amount of space after the block is freed. In the paper's version of mapping_search, this is subtle because r is marked as "in out". http://www.gii.upv.es/tlsf/files/papers/jrts2008.pdf To reproduce: * Allocate an amount that is rounded up. * Allocate a second block that takes up the remaining space. This prevents the second allocation from occurring in a new spot. * Free the first block. * Allocate the first amount a second time. It will fail without this change even though the requested space is actually available because the free block was added to the non-rounded segmented list.
The original code missed a * to check the value of size for non-zero. This corrects that error and adds an additional check after the first alignment.
|
Hi @tannewt, |
|
Any update for reviewing this PR? BTW, I noteice the tlsf in esp-idf-v5.2 branch is quite old. |
@SoucheSouche |
@SoucheSouche @igrr |
|
Hi @tannewt, @AxelLin, sorry for not taking a look at this earlier. I will get started on it this week. @AxelLin concerning v5.2 and the TLSF, it is a bit tricky since some target have the option to use the ROM version of the TLSF. So for a given change in the TLSF submodule we have to make sure it can be patched to the existing TLSF in the ROM. |
|
Hi @tannewt, I had a look at your proposed changes. For small allocations this difference is quite minimal but once you start getting into allocations of a few KB it gets less neglectable. (e.g, for 3000 bytes allocations on esp32s3 using the default capability, 3072 bytes end up being allocated from which 60 bytes are related to your changes). However, I can see that when freeing the memory, instead of recalculating the rounded size from the size of the block to get the fl, sl value pair that was actually used in the allocation to mark the block as used in the bitmap, we just calculate the fl, sl value pair based on the size of the block without rounding it and this leads to a different pair of fl, sl being calculated, hence your issue. I updated the tlsf_free function to use the rounded size of the block instead of the size only and it fixes the problem, without having to allocate more memory than necessary. I will look more into it to make sure it doesn't create any problem but if not, then I would suggest using this strategy instead. |
|
That’s the intended design of the tlsf paper I think. It marks that variable as in-out. Those unused bytes are deliberately considered in the overhead calculation.
The reason I changed it was that I couldn’t free a 3000 byte allocation and then allocate back into that spot because it was looking for 3072. This matters when that’s the only place that the allocation can fit.
…On Thu, Aug 1, 2024, at 11:59 PM, Guillaume Souchere wrote:
Hi @tannewt <https://github.com/tannewt>, I had a look at your proposed changes.
By returning the rounded size used to find a suitable block instead of the requested size, you prevent the algorithm from merging the difference between the requested size and the suitable block size (if any) back to the next free block. Meaning that those bytes will be unusable.
For small allocations this difference is quite minimal but once you start getting into allocations of a few KB it gets less neglectable.
(e.g, for 3000 bytes allocations on esp32s3 using the default capability, 3072 bytes end up being allocated from which 60 bytes are related to your changes)
—
Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAM3KMWPPV6TFPJTWCKGKTZPMU5TAVCNFSM6AAAAABEGE5JNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRUG4YDCMBVGE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@tannewt, yes I totally understand the reason for your changes. As I mentioned in the last 2 paragraphs of my last message, instead of including this rounded size overhead in the allocation, we can calculate the rounded size of the allocated memory in the free. This will ensure that we get the same sl, fl value pair that was calculated in the alloc, so we change the right bitmap value back to free. E.g., (not shure the fl, sl values are actually right but this is just for demonstration purposes)
So either we include the rounded value in the allocation so that when we free, the FL/SL value calculated will be matching the one calculated in the allocation |
This would be incorrect because a following allocation of 20001 would round to the same 20523 but the free space wouldn't be enough. The list that the fl/sl values points to is of blocks with a minimum size at least 20523. That way you don't need to transverse the list to find one that actually works. You just take the 20523 block and mark any more extra as free. |
|
@tannewt, you are right. It does seem to be the only way to resolve the issue. Great job with this find. I will submit the PR for review. |
|
Thanks! I only found it because we had a large allocation that we couldn’t reallocate.
…On Wed, Aug 7, 2024, at 1:28 AM, Guillaume Souchere wrote:
@tannewt <https://github.com/tannewt>, you are right. It does seem to be the only way to resolve the issue. Great job with this find. I will submit the PR for review.
—
Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAM3KNJBJZLFHCQAKTEE5LZQHLEJAVCNFSM6AAAAABEGE5JNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZSHEYTIOJWGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Hi @tannewt, I just merged your fix. Thanks again for the contribution and sorry it took so long to get to it. |
|
Where did you merge it? I don't see it. No problem! I like TLSF and am happy to see it fixed. (We use it on all CircuitPython ports now.) |
|
It's been merged on our internal gitlab server. It should sync your contribution in GitHub shortly. If not, I'll take a look at it on Monday. |
|
I think the last commit breaks this on ESP. It works on RP2350 but I may have adjusted the size so it does. |
…y be out of bounds
github-actions
bot
changed the title
|
@tannewt, I already fixed the next_free issue before merging the PR. |
|
Done in ea82911 |
It needs to include the rounding done by
mapping_search()otherwise we'll trim the block too much and not be able to re-allocate the same amount of space after the block is freed.In the paper's version of mapping_search, this is subtle because r is marked as "in out". http://www.gii.upv.es/tlsf/files/papers/jrts2008.pdf
To reproduce: