Attention is currently required from: Lance Zhao, Konrad Adamczyk, Jakub Czapiga, Tim Wawrzynczak, Grzegorz Bernacki, Karthik Ramasubramanian.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74715 )
Change subject: acpi: Add missing cbfs_unmap() ......................................................................
Patch Set 3:
(1 comment)
File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/74715/comment/a9fb534a_724b2b5e PS2, Line 1934: cbfs_unmap(dsdt_file);
As long as you don't overflow your heap, there aren't any problems with this approach.
I think the expectation is that being a good citizen and freeing your memory actually results in the memory being freed. Then, if we run out of memory, the first step is seeing who allocated without a corresponding free, and cleaning that up. However, the (hidden) gotcha is that freeing is only part of the fix, we also need to free in the correct order.
We aren't hitting the memory limit today, but not being able to free memory due to order of operations issues could potentially cause that in the future, and would be a pain to debug and fix, since memory lifetime issues can be tricky without being familiar with how the overall system operates. It may be the case that a more complex allocator is still simpler to maintain than juggling when to free memory to make sure we don't hit the limit.
All that being said, I agree with you and I don't think we need to fix the allocator yet, since we still have plenty of room and things are working fine. However, it would be good to track this somehow, or at least make the memory freeing requirements more visible somehow, so we don't need to rediscover what we've learned here again if/when we do run out of space in the future.
-
Tim Van Patten (Code Review)