Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add alignment block splitting to malloc_freelist #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaeljclark
Copy link

Description

The main allocator is modified to split blocks to align them, returning surplus to the free list.

I am sharing this because I think this approach is a little cleaner than the existing approach. It has several advantages and adds minimal complexity. It has been tested locally but it needs more testing and review. The primary code path should be unchanged for regular allocations besides dropping the minimum allocation size and calculating alignment_slack which should always be zero for regular allocations.

The fundamental algorithm is mostly unchanged although there is additional logic to calculate alignment_slack in the case that the alignment is greater than the alignment of the found block. If alignment_slack is non-zero it is used to split the block so that block is sufficiently aligned with the surplus returned to the free list. Some indent has been removed for readability.

aligned_free is now redundant but it is kept for compatibility. It is no longer necessary to unwrap an offset field in wrapper headers as the main allocation header block is aligned and free can be called directly on aligned allocations, so aligned_free now simply delegates to free(). Also, the surplus memory to align blocks is not wasted and can be allocated.

This changes needs rigorous testing and review. This is a heads up!

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update which has been made
    • The change is backwards compatible but it relaxes requirements for aligned_free.

How Has This Been Tested?

  • Test A - The internal libmemory_freelist_test test passes.
  • Test B - The code is used in a kernel that requires aligned allocations and appears to function correctly.

Test Configuration:

  • Project version/commit: f758bd4
  • Hardware: Intel Skylake i9
  • Toolchain: Ubuntu/GCC

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

The main allocator is modified to split blocks to align them.

The fundamental algorithm is mostly unchanged although there
is some additional logic to calculate alignment_slack in the
case that the alignment is greater than the alignment of the
found block. The alignment_slack is used to split the block
with the surplus returned to the freelist. Some indent has
been removed from the code for readability.

aligned_free is now redundant but it is kept for compatibility.
It is no longer necessary to unwrap an offset field in wrapper
headers as the main allocation header block is aligned and free
can be called directly on aligned allocations, so aligned_free
now simply delegates to free().

The documentation is updated to reflect these changes.
@phillipjohnston
Copy link
Member

phillipjohnston commented Feb 25, 2024 via email

@michaeljclark
Copy link
Author

Thanks! It's mostly just a heads up. Take your time...

It hadn't occurred to me but perhaps you indented code on the bodies of if conditions as a secure coding practice because if-not-clause-or-not-clause with an early return could be more prone to logic errors than an if-and-clause-and-clause with an indented block. In any case it should not be too hard to change the indentation if you prefer it the other way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants