Arduino
Arduino copied to clipboard
esp8266
Public umm_*_core and the rest of UMM functions for context
Looking at the implementation of the UMM heaps & different accessors... While we can manipulate stack of the heap contexts, we can't get the specific one via ID directly but only after changing it globally https://github.com/esp8266/Arduino/blob/7356cd1ef13c7a7da4631be01060f6f40806fa23/cores/esp8266/umm_malloc/umm_malloc.h#L38
Should this be public function? https://github.com/esp8266/Arduino/blob/7356cd1ef13c7a7da4631be01060f6f40806fa23/cores/esp8266/umm_malloc/umm_malloc.cpp#L155-L159
@mhightower83
Maybe, I thought about it. I didn't see a normal use case for it. Though I have done some diagnostic stuff that could use it; however, it also required access to the structure for umm_heap_context_t, which is also private.
I don't think there is an exported function that can accept the result of umm_get_heap_by_id.
From the sketch side, the umm_heap_context_t return value is used when testing for success.
Hmm, looking through these exports, I don't see how umm_get_current_heap() can be used by the sketch.
I had this q bookmarked, but now I also noticed I've been looking at some of my own modifications...
Right now, only umm_info() getHeapStats() implementations is accepting ctx pointer, and that had been my use-case initially to check stats of each heap. We just pass along the ctx pointer, so we don't specifically need the user to have the ctx struct publicized as well.
What may be another use-case - public umm_malloc(ctx), umm_free(ctx) etc. and the rest of the allocation routines. I'd should probably rename the issue as 'public functions to interact with the umm context'
so, in general, since everything essentially happens through the context
- have umm_info() to accept pointer as well, so there's no need to get 'current' context
- public umm_*_core, so we don't have to manipulate context stack before accessing them
- clean-up internals that accept 'current' pointer and accept ptr instead
thus, allow heap class as allocator, something like
class DramAlloc {
DramAlloc(umm_context_t* ctx) : _ctx(ctx) {}
void* allocate(size_t size) { return umm_malloc_core(_ctx); }
void deallocate(void* ptr) { umm_free_core(_ctx, ptr); }
private:
umm_context_t* _ctx;
};
Right now, only ~umm_info()~ getHeapStats() implementations is accepting ctx pointer, ...
Ahh, that's where those went. Those did not show up on my search, because the current changes I have to sync with upstream needed to refactor those function names. And, I gave up with the issue of internal things that were not exposed and moved getHeapStats into umm_local.c. I also abused the _core (umm_max_block_size_core,...) suffix, which should be reserved for functions that have to be called from within a critical section. The upstream now have names for some of these functions so I plan to sync with those.
The current umm_malloc_core(ctx, ...) requires a critical section. To expose these will require a alternative version of the umm_malloc() function.
This was written before your last post.
void deallocate(void* ptr) { umm_free_core(_ctx, ptr); }
For free, it may be easier and safer to just let it select the Heap free context; however, providing the context in the call would speed things up. I guess we can add debug build code to check that the pointer falls in the context provided. Maybe validating the context is really a context.
For malloc, we currently override with DRAM when in an IRQ .
For realloc, do you want to allow a force reselection of Heap context and copy, when the context is changed?
Other inquiry-type APIs should be straightforward. (I think - maybe)
... providing the context in the call would speed things up.
This would become a problem with free-ing an allocation that was made from ISR and was forced to use DRAM. The safest approach for free is to stay in control and ~extract~ determine the correct context for the free. I think I may have traveled this road before.
This would become a problem with free-ing an allocation that was made from ISR and was forced to use DRAM. The safest approach for free is to stay in control and extract determine the correct context for the free. I think I may have traveled this road before.
For malloc, we currently override with DRAM when in an IRQ .
True, but... it can also just fail for everything that isn't dram ctx and not automatically substitute it? Also, crash, which may be a pretty good sign to not do certain things (...and another variable for us to track and to try to make it happen, additional code, and additional maintenance)
(edit: but, I'd guess SDK may do something in ISR, but I have thought pvPortMalloc and etc. handle that case :/)
For free, it may be easier and safer to just let it select the Heap free context; however, providing the context in the call would speed things up. I guess we can add debug build code to check that the pointer falls in the context provided. Maybe validating the context is really a context.
Just a debug check may be better? Given the heapselect (or explicit dramalloc) idea, we would expect API user to stay sane and within limits
For realloc, do you want to allow a force reselection of Heap context and copy, when the context is changed?
I have not though that far, but I suppose it should not try to select some other heap unless we told it to. Either with an explicit ctx list of possible heaps, or IDs.
True, but... it can also just fail for everything that isn't dram ctx ...
I think part of the concern here was a class being instantiated from within an IRQ. Not a great idea; however, it worked before and I didn't want to make multiple heap support break things. In general, I tried to avoid breaking things that were working.
I think we have the SDK covered through pvPort... and switching back to DRAM in: https://github.com/esp8266/Arduino/blob/15e7d35d6e7c430915a5a86d854ae855d10a8da5/cores/esp8266/core_esp8266_main.cpp#L254-L263
BTW, should the 'lock' be on and for the structure itself, not the global system level for every allocator ctx? Thinking on it again, might not be the best idea to expose non-locking functions, while we still could expose upper level one including the ctx pointer?
I am not sure I follow your line of thinking.
Because of the potential of Heap allocating/freeing during an IRQ, INTLEVEL is raised to protect against changes while accessing Heap umm_block structures.
For umm_info, it is in ICACHE and must not be called from an IRQ. Since it will not be called from ISRs, this allows its structure to be accessed w/o any locking after the data is gathered from the Heap umm_block structures.
Yes, but that is not related to the global wrappers for the malloc. Ignoring those completely would be the goal, explicitly using the specific region in a container / object / poll of things / etc. through the umm functions
Unless I am missing something, the main idea behind isr lock is to protect from isr code calling malloc, and since our structures are effectively. Just considering that the code that does not do that, ctx can hold some minimal set of checks for reentry protection and simply assume we are doing everything safe? (...timing, probably too, since we
Sorry, I am still having trouble following.
For most of the data in _context, it is all read safe. When updating, global locks are usually in place because we are walking the Heap (umm_block) to perform some operation, malloc, free, or realloc. This often requires rewriting umm_blocks to create or free an allocation, the free heap variable is updated at that time to stay current. (I don't want to get too deep into realloc(); however, there are locations when doing memory copy or move that we can release locks for the copy then resume after.)
Maybe I can follow your concern better with a specific example?
Sorry for the delay, just going over the main thwarts a) it would be nice to have function to request specific callocation context, using either ctx or its tag. the OT meant that expose ctx pointer b) dependency on internal 'stack', forcing us to use ESP methods to switch 'contexts'. pretty confusing, tbh
Main application example would be your original PR for String https://github.com/esp8266/Arduino/pull/7769/files
Suppose, instead we would have something close to the C++ allocator design
struct String {
String(const Allocator& allocator) :
_allocator(allocator)
{}
~String() {
_allocator.deallocate(_buf);
}
void setLen(size_t n) {
_buf = _allocator.allocate(n);
}
private:
const char* _buf;
const Allocator& _allocator;
};
struct Allocator {
Allocator(int ctx) :
_ctx(ctx)
{}
void* allocate(size_t n) {
return umm_somehow_allocate(_ctx, n);
}
void deallocate(void* ptr) {
return umm_somehow_deallocte(_ctx, ptr);
}
private:
int _ctx; // or umm ctx struct ptr
};