Skip to content

Add used() API in AddressAllocator #95

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
## Upcoming version

### Added

- Added `used()` API in `AddressAllocator` to allow
getting the used memories after `allocate/free()`s

### Changed
### Fixed
### Removed
Expand Down
39 changes: 36 additions & 3 deletions src/address_allocator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ pub struct AddressAllocator {
// tree will represent a memory location and can have two states either
// `NodeState::Free` or `NodeState::Allocated`.
interval_tree: IntervalTree,
// Used memory space in the address space.
used: usize,
}

impl AddressAllocator {
Expand All @@ -43,6 +45,7 @@ impl AddressAllocator {
Ok(AddressAllocator {
address_space: aux_range,
interval_tree: IntervalTree::new(aux_range),
used: 0,
})
}

Expand All @@ -63,13 +66,30 @@ impl AddressAllocator {
policy: AllocPolicy,
) -> Result<RangeInclusive> {
let constraint = Constraint::new(size, alignment, policy)?;
self.interval_tree.allocate(constraint)
let allocated = self.interval_tree.allocate(constraint)?;
self.used = self
.used
.checked_add(allocated.len() as usize)
.expect("Failed to calculate used memory");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that expect is the best solution here. I think it would be better to use ok_or(Error::Overflow) and propagate the error with ?

Ok(allocated)
}

/// Deletes the specified memory slot or returns `ResourceNotAvailable` if
/// the node was not allocated before.
pub fn free(&mut self, key: &RangeInclusive) -> Result<()> {
self.interval_tree.free(key)
self.interval_tree.free(key)?;
self.used = self
.used
.checked_sub(key.len() as usize)
.expect("Failed to calculate used memory");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here but with ok_or(Error:Underflow)

Copy link
Member

@roypat roypat Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error conditions is impossible to hit unless the logic in the library is fundamentally broken. In that case, panicking is the correct thing to do I'd say - if we just propagate with ? then the allocate will be left in an inconsistent state anyway, which I don't think would be recoverable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here but with ok_or(Error:Underflow)

same thought as @roypat, it should be ok to panic here since the checked_add/sub() should never fail unless allocate/free() before these two calls went very wrong like it's capable of allocating some total >=usize:MAX memory or free some non-existing memory calling used < 0.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, panicking is the correct thing to do I'd say - if we just propagate with ? then the allocate will be left in an inconsistent state anyway, which I don't think would be recoverable

You are right that the interval tree will be likely messed up. But the reason for propagating the error is to allow the caller to clean up after themselves before exiting.

Copy link
Author

@ylzh10 ylzh10 Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, panicking is the correct thing to do I'd say - if we just propagate with ? then the allocate will be left in an inconsistent state anyway, which I don't think would be recoverable

You are right that the interval tree will be likely messed up. But the reason for propagating the error is to allow the caller to clean up after themselves before exiting.

i don't really expect caller to handle this error externally, since it's non-recoverable and the program should crash if it hits here.
How would you expect caller to handle/clean up this, like undo the allocate/free()? but the previous allocate/free on the interval tree has succeeded before the self.used.add/sub() and callers have no way to manipulate self.used w/o changing the interval tree again, in which case the interval tree and used() can never be "consistent" w/ each other. IMHO, panic inside allocator would give callers a signal that this is non-recoverable, they should seek help to fix the allocator rather than remediate on their side

Ok(())
}

/// Returns the used memory size in this allocator.
/// NOTE that due to fragmentations, not all unused memory may be available
/// for next `allocate()` call!
pub fn used(&self) -> usize {
self.used
}
}

Expand Down Expand Up @@ -158,20 +178,27 @@ mod tests {
#[test]
fn test_allocate_with_alignment_first_ok() {
let mut pool = AddressAllocator::new(0x1000, 0x1000).unwrap();
assert_eq!(pool.used(), 0);
// Allocate 0x110
assert_eq!(
pool.allocate(0x110, 0x100, AllocPolicy::FirstMatch)
.unwrap(),
RangeInclusive::new(0x1000, 0x110F).unwrap()
);
assert_eq!(pool.used(), 0x110);
// Allocate 0x100
assert_eq!(
pool.allocate(0x100, 0x100, AllocPolicy::FirstMatch)
.unwrap(),
RangeInclusive::new(0x1200, 0x12FF).unwrap()
);
assert_eq!(pool.used(), 0x110 + 0x100);
// Allocate 0x10
assert_eq!(
pool.allocate(0x10, 0x100, AllocPolicy::FirstMatch).unwrap(),
RangeInclusive::new(0x1300, 0x130F).unwrap()
);
assert_eq!(pool.used(), 0x110 + 0x100 + 0x10);
}

#[test]
Expand Down Expand Up @@ -230,18 +257,24 @@ mod tests {
#[test]
fn test_tree_allocate_address_free_and_realloc() {
let mut pool = AddressAllocator::new(0x1000, 0x1000).unwrap();
assert_eq!(pool.used(), 0);
// Allocate 0x800
assert_eq!(
pool.allocate(0x800, 0x100, AllocPolicy::FirstMatch)
.unwrap(),
RangeInclusive::new(0x1000, 0x17FF).unwrap()
);

assert_eq!(pool.used(), 0x800);
// Free 0x800
let _ = pool.free(&RangeInclusive::new(0x1000, 0x17FF).unwrap());
assert_eq!(pool.used(), 0);
// Allocate 0x800 again
assert_eq!(
pool.allocate(0x800, 0x100, AllocPolicy::FirstMatch)
.unwrap(),
RangeInclusive::new(0x1000, 0x17FF).unwrap()
);
assert_eq!(pool.used(), 0x800);
}

#[test]
Expand Down