M-Labs
/
zynq-rs
11
0
Fork 15
Code Issues 6 Pull Requests 1 Releases Wiki Activity

Allocator hacks #48

Merged
sb10q merged 1 commits from pca006132/zynq-rs:alloc-hack into master 2020-08-03 14:49:32 +08:00
Conversation 5 Commits 1 Files Changed 1 +4
pca006132 commented 2020-07-20 11:15:15 +08:00
Contributor
Copy Link

Hacks on the allocator for M-Labs/artiq-zynq#32

  • Split the allocator into two separate allocators for the two cores.
  • Allow user to do allocation in core1 heap using unsafe alloc_in_core1.
  • Force the core1 allocator mutex to unlock before init. It is possible that core1 got reset while doing allocation/deallocation, but the core1 states should all be cleaned up, and unlocking the mutex should be safe.
Hacks on the allocator for https://git.m-labs.hk/M-Labs/artiq-zynq/issues/32 * Split the allocator into two separate allocators for the two cores. * Allow user to do allocation in core1 heap using unsafe `alloc_in_core1`. * Force the core1 allocator mutex to unlock before init. It is possible that core1 got reset while doing allocation/deallocation, but the core1 states should all be cleaned up, and unlocking the mutex should be safe.
astro commented 2020-07-22 08:41:46 +08:00
Contributor
Copy Link

LGTM

If we're sure that one of the allocators is always used by only one core, we could get rid of the Mutex for just that one.

LGTM If we're sure that one of the allocators is always used by only one core, we could get rid of the Mutex for just that one.
pca006132 commented 2020-07-22 09:44:02 +08:00
Author
Contributor
Copy Link

LGTM

If we're sure that one of the allocators is always used by only one core, we could get rid of the Mutex for just that one.

For the core0 allocator, yes.

Actually I am considering doing a CPU ID check for core0 allocator deallocation, and panic/log when we try to deallocate from core1 to core0 allocator, to help pinpoint the possible location for memory leak, as we should not transfer something located in core0 to core1.

> LGTM > > If we're sure that one of the allocators is always used by only one core, we could get rid of the Mutex for just that one. For the core0 allocator, yes. Actually I am considering doing a CPU ID check for core0 allocator deallocation, and panic/log when we try to deallocate from core1 to core0 allocator, to help pinpoint the possible location for memory leak, as we should not transfer something located in core0 to core1.
👍 1
sb10q reviewed 2020-07-26 23:26:43 +08:00
libsupport_zynq/src/ram.rs Outdated
@ -10,0 +10,4 @@
static ALLOCATOR: CortexA9Alloc =
CortexA9Alloc(Mutex::new(Heap::empty()), Mutex::new(Heap::empty()));
static mut FORCE_CORE1: bool = false;
sb10q commented 2020-07-26 23:26:43 +08:00
Owner
Copy Link

We shouldn't need FORCE_CORE1 anymore.

We shouldn't need FORCE_CORE1 anymore.
sb10q reviewed 2020-07-26 23:29:17 +08:00
libsupport_zynq/src/ram.rs Outdated
@ -51,0 +91,4 @@
// It is possible for core1 to get reset while allocation/deallocation,
// as all core1 states should be cleared after reset,
// forcing the mutex to be unlocked should be safe.
ALLOCATOR.1.force_unlock();
sb10q commented 2020-07-26 23:29:17 +08:00
Owner
Copy Link

With core-specific allocators, we shouldn't need mutexes either.
Related note: Due to this problem of resets while holding a mutex, we probably want to prefer static mut over a mutex in code that is executed by core1 in artiq-zynq.

With core-specific allocators, we shouldn't need mutexes either. Related note: Due to this problem of resets while holding a mutex, we probably want to prefer ``static mut`` over a mutex in code that is executed by core1 in artiq-zynq.
sb10q reviewed 2020-07-26 23:30:16 +08:00
libsupport_zynq/src/ram.rs
@ -51,0 +101,4 @@
#[cfg(feature = "alloc_core")]
/// Allocate memory in core1 heap. Should only be invoked in core0.
/// Done by temporarily overriding a flag to change the global allocator used.
pub unsafe fn alloc_in_core1<F, T>(f: F) -> T
sb10q commented 2020-07-26 23:30:16 +08:00
Owner
Copy Link

should be unneeded, remove

should be unneeded, remove
sb10q reviewed 2020-07-26 23:32:46 +08:00
libsupport_zynq/src/ram.rs
@ -24,0 +44,4 @@
if start0 <= const_ptr && const_ptr < end0 {
self.0.lock()
} else if start1 <= const_ptr && const_ptr < end1 {
self.1.lock()
sb10q commented 2020-07-26 23:32:46 +08:00
Owner
Copy Link

We shouldn't need to deallocate things in the heap of the other core, so check for MPIDR and that address range matches.

We shouldn't need to deallocate things in the heap of the other core, so check for MPIDR and that address range matches.
sb10q commented 2020-08-03 14:01:54 +08:00
Owner
Copy Link

Not used in ARTIQ, but to make this crate more generic, we should be able to use the single-core allocator with the linker symbols that define the location of the heap.

Not used in ARTIQ, but to make this crate more generic, we should be able to use the single-core allocator with the linker symbols that define the location of the heap.
pca006132 commented 2020-08-03 14:04:34 +08:00
Author
Contributor
Copy Link

Not used in ARTIQ, but to make this crate more generic, we should be able to use the single-core allocator with the linker symbols that define the location of the heap.

Yes, there are conditions detecting cfg!(not(feature = "alloc_core")) and use the allocator 0.

> Not used in ARTIQ, but to make this crate more generic, we should be able to use the single-core allocator with the linker symbols that define the location of the heap. Yes, there are conditions detecting `cfg!(not(feature = "alloc_core"))` and use the allocator 0.
sb10q commented 2020-08-03 14:11:03 +08:00
Owner
Copy Link

Right, but this provides only init_alloc_ddr and not something that would use linker symbols.

Right, but this provides only ``init_alloc_ddr`` and not something that would use linker symbols.
sb10q closed this pull request 2020-08-03 14:49:32 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/zynq-rs#48
No description provided.
Delete Branch "pca006132/zynq-rs:alloc-hack"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?