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 used() API in AddressAllocator #95

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

ylzh10
Copy link

@ylzh10 ylzh10 commented Feb 19, 2025

Summary of the PR

bit dumb here, but I thought it should be a common use case to get the available memories in AddressAllocator, while i don't see a way on how to get this info today.
Or there is already a way to get this info from allocator and somehow i missed it? (please lmk)

So add a new private var used

  • init it w/ 0
  • inc/dec it when allocate/free()
  • expose it in a new public API used() so we can infer how many available memories are left (We don't want to expose available() directly since it can be confusing/inaccurate due to fragmentations most times)

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Merge branch 'main' of https://github.com/ylzh10/vm-allocator

add `available()` to AddressAllocator

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Create rust.yml

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update CHANGELOG.md

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Delete .github/workflows directory

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update CHANGELOG.md

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

update changelog.md

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

update format

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

update format

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

update changelog.md

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update CHANGELOG.md

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Delete .github/workflows directory

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update address_allocator.rs

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Update CHANGELOG.md

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>

Create rust.yml

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>
@ylzh10 ylzh10 changed the title Add available() API in AddressAllocator #94 Add available() API in AddressAllocator Feb 19, 2025
@ylzh10 ylzh10 marked this pull request as ready for review February 19, 2025 08:14
@ylzh10
Copy link
Author

ylzh10 commented Feb 19, 2025

dup of #94

@roypat thanks for reviewing my PR.
My usecase is simply to allocate/free some memory and then check the rest available memories in the addr space to account for the memory usage and potential mem leaks.
yes i understand that due to fragmentation, available() may return more than the actual "available" memory spaces, but in our use case, when we do allocate(size, alignment), size is always aligned with alignment as well, not just the start addr, so say alignment == 4k, we always allocate a memory block of multiple of 4k size and since its start addr is also aligned w/ 4k, this available info should be "more accurate" (tho still has fragmentations), and we'd like to use this info to as a indicator to tell API users the available memory spaces (best effort tho : )).

@AlexandruCihodaru
Copy link
Collaborator

AlexandruCihodaru commented Feb 19, 2025

dup of #94

@roypat thanks for reviewing my PR. My usecase is simply to allocate some memory and then check the rest available memories in the addr space to account for the memory usage and potential mem leaks. yes i understand that due to fragmentation, available() may return more than the actual "available" memory spaces, but in our use case, when we do allocate(size, alignment), size is always aligned with alignment as well, not just the start addr, so say alignment == 4k, we always allocate a memory block of multiple of 4k size and since its start addr is also aligned w/ 4k, this available info should be accurate, right?

You can have a memory pool from 0 to 12K. You can allocate a block with ExactMatch from 4k to 8k. Now you have 8k of available memory but you can not allocate a 8k blob. I think that in this scenario "available" as it is implemented is misleading.

My usecase is simply to allocate some memory and then check the rest available memories in the addr space to account for the memory usage and potential mem leaks

Maybe you could check that the memory that has been freed is the same as memory that has been allocated.

@ylzh10
Copy link
Author

ylzh10 commented Feb 19, 2025

dup of #94
@roypat thanks for reviewing my PR. My usecase is simply to allocate some memory and then check the rest available memories in the addr space to account for the memory usage and potential mem leaks. yes i understand that due to fragmentation, available() may return more than the actual "available" memory spaces, but in our use case, when we do allocate(size, alignment), size is always aligned with alignment as well, not just the start addr, so say alignment == 4k, we always allocate a memory block of multiple of 4k size and since its start addr is also aligned w/ 4k, this available info should be accurate, right?

You can have a memory pool from 0 to 12K. You can allocate a block with ExactMatch from 4k to 8k. Now you have 8k of available memory but you can not allocate a 8k blob. I think that in this scenario "available" as it is implemented is misleading.

My usecase is simply to allocate some memory and then check the rest available memories in the addr space to account for the memory usage and potential mem leaks

Maybe you could check that the memory that has been freed is the same as memory that has been allocated.

You can have a memory pool from 0 to 12K. You can allocate a block with ExactMatch from 4k to 8k. Now you have 8k of available memory but you can not allocate a 8k blob. I think that in this scenario "available" as it is implemented is misleading.

yeah, i understand this fragmentation and available can never be accurate due to this. It's expected in our use case and would give some signal on the max available free memory and i can add more documentation in this api for this behavior.
The issue we have today when using the AddressAllocator is it doesn't have any public API to allow callers to peek into the any free memory size info. Alternatively, exposing the interval tree and let users count the free memory blocks would let users ctrl how they want to count total "available" memory (or the max avail memory chunk size), but i feel it might be too much.

Maybe you could check that the memory that has been freed is the same as memory that has been allocated.

the alloc/free/get_avail calls happen asynchronously and some allocations may persist a longer time than others before they're freed, so we typically check the memory usage when at the beginning/end of the program/func to make sure they match, no mem leaks rather than accumulate each alloc/free calls.

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>
@roypat
Copy link
Collaborator

roypat commented Feb 20, 2025

maybe we can have the inverse of this API instead? Something that returns how many bytes are used from the allocator range? That should cover what you want to do, and be less misleading, API-wise.

@ylzh10
Copy link
Author

ylzh10 commented Feb 20, 2025

maybe we can have the inverse of this API instead? Something that returns how many bytes are used from the allocator range? That should cover what you want to do, and be less misleading, API-wise.

sg, that works too, any API that can expose the available/used memory in the address space/interval tree would be helpful. let me update the code.

Signed-off-by: ylzh10 <46687083+ylzh10@users.noreply.github.com>
@ylzh10 ylzh10 changed the title Add available() API in AddressAllocator Add used() API in AddressAllocator Feb 20, 2025
@ylzh10
Copy link
Author

ylzh10 commented Feb 25, 2025

@roypat @AlexandruCihodaru can you help review the changes again and see if you have any further comments?

facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this pull request Feb 26, 2025
Summary:
follow wiki (https://www.internalfb.com/wiki/Rust/Third_Party_Libraries/Adding_or_Updating_Libraries/#maintaining-local-change) to patch the pkg w/ my PR.
* rust-vmm/vm-allocator#95
  * Added a `used()` API in `AddressAllocator` so in runtime we can report DRAM/SRAM usage.

NOTE to myself on updating rust 3rd party lib
* update https://www.internalfb.com/code/fbsource/[eae01930e4f97fad5f6f03341bbbdc0b62636187]/third-party/rust/Cargo.toml?lines=1483
* `cd ~/fbsource && fbcode/common/rust/tools/reindeer/vendor && arc autocargo`

Reviewed By: diliop

Differential Revision: D69826054

fbshipit-source-id: c712d57a6e74398b61e8288c6fae5d50954a967f
facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request Feb 26, 2025
Summary:
follow wiki (https://www.internalfb.com/wiki/Rust/Third_Party_Libraries/Adding_or_Updating_Libraries/#maintaining-local-change) to patch the pkg w/ my PR.
* rust-vmm/vm-allocator#95
  * Added a `used()` API in `AddressAllocator` so in runtime we can report DRAM/SRAM usage.

NOTE to myself on updating rust 3rd party lib
* update https://www.internalfb.com/code/fbsource/[eae01930e4f97fad5f6f03341bbbdc0b62636187]/third-party/rust/Cargo.toml?lines=1483
* `cd ~/fbsource && fbcode/common/rust/tools/reindeer/vendor && arc autocargo`

Reviewed By: diliop

Differential Revision: D69826054

fbshipit-source-id: c712d57a6e74398b61e8288c6fae5d50954a967f
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.

3 participants