buffer_manager: Implement Generic Buffer Management with Ownership Validation #844
Conversation
bhargavshah1988
changed the title
| @@ -0,0 +1,259 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
Looks like our formatter doesn't quite catch duplicate copyright directives. Please remove one :)
There was a problem hiding this comment.
Can never be too safe when it comes to IP.
| } | ||
| } | ||
|
|
||
| pub struct BufferManager { |
There was a problem hiding this comment.
Given that this is a new generic utility component, please add usage documentation for the BufferManager class.
Some things that are useful to include are any intended limitations, performance characteristics, etc. Please also include comments when a routine would PANIC (for example: in the case of external corruptions)
| impl BufferManager { | ||
| pub fn new(size: usize) -> Self { | ||
| let mut signature: u64 = 0; | ||
| getrandom::getrandom(signature.as_mut_bytes()).expect("crng failure"); |
There was a problem hiding this comment.
Could you just use let signature = getrandom::u64().expect("failed to allocate random signature"); instead? That would avoid this taking a depending on zerocopy, and is simpler.
There was a problem hiding this comment.
It looks like getrandom does not have u64(). Do you have any example for this ?
There was a problem hiding this comment.
Please don't use getrandom for this, it's expensive. If you really need this, then using a static AtomicU64 to generate unique values.
|
|
||
| pub fn free(&mut self, buffer: BufferBlock) { | ||
| if buffer.signature != self.signature { | ||
| panic!("Attempted to free a buffer from a different allocator"); |
There was a problem hiding this comment.
Would it instead be preferable for each BufferBlock to maintain a pointer to the BufferManager, and then have the buffer simply remove itself from the correct buffer block?
There was a problem hiding this comment.
In addition, it seems that we allow for safe double frees (that is: you can free a buffer as many times as you want). Is that your intention? What's the use case for that?
There was a problem hiding this comment.
In addition, it seems that we allow for safe double frees (that is: you can
freeabufferas many times as you want). Is that your intention? What's the use case for that?
There is no usecase for double free at hand. Can make it more strict on double free.
| } | ||
|
|
||
| pub fn malloc(&mut self, size: usize) -> Option<BufferBlock> { | ||
| if let Some(pos) = self.free_list.iter().position(|block| block.size >= size) { |
There was a problem hiding this comment.
So, we've arrived at a general purpose heap implementation, and this one is pretty slow.
For the use cases we have, do we need a general purpose heap, i.e., do we need to return contiguous buffers? Or can we just have a page allocator?
There was a problem hiding this comment.
(Even if we do need a general-purpose heap, this O(N) scanning is going to be quite expensive.)
This PR introduces a generic buffer management system that ensures memory safety and proper ownership tracking. It prevents accidental or malicious freeing of buffers allocated by a different instance using a signature-based validation mechanism. The design maintains a sorted free list, minimizes fragmentation by merging adjacent free blocks, and ensures unique allocation IDs.