[mrajwa]

This patch adds another argument to allocator functions
which defines if allocation must succeed or can fail.
Such addition is needed as in some cases components
"probe" memory caps if they ara capable of storing
certain amount of data. If they ain't, nothing wrong
happens in functional regards since components either
decrease is original request or use different memory
region. However with current implementation you will
see error message saying that allocation has failed.
This is very misleading if success of allocation wasn't
necessary.

Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com

[dbaluta]

@mrajwa But we will still see errors logged here right?

[mrajwa]

@dbaluta, no you will see only regular log message which shall be treated as a warning. Previously it was error message and people were confused, linking almost every WoV issue with this error message like "Ohh of course it happens because there was not enough memory".

[dbaluta]

Instead of adding a new parameter I think we can extend the caps field to also include a flag named SOF_MEM_NOWARN (similar with GFP_NOWARN from Linux kernel).

When this flag is set it has the same meaning as your can_fail parameter.

Also, it might worth renaming caps parameter to flags.

[mrajwa]

I was thinking about that too. However I am afraid it may confuse people more than extra single argument. Anyway it is worth discussing - if more people like this idea too I am more than happy to go with this direction.

[jajanusz]

caps is for capabilities, not error reporting so it would be confusing, I'd go for another function

[dbaluta]

@jajanusz will rename parameter from caps to flags and so that we can avoid confusion. Have a look at GFP_ macros in Linux kernel (GFP_NOWARN).

[jajanusz]

@dbaluta If you change it to flags then it's fine for me

[lgirdwood]

I think what you want to do is to check whether X bytes of type Y are available in heap and if so allocate them ? This needs an additional heap API call to return avail bytes of type Y that can then be used to allocate. Note. This get free bytes and the allocate sequence should be atomic.

[paulstelian97]

I think what you want to do is to check whether X bytes of type Y are available in heap and if so allocate them ? This needs an additional heap API call to return avail bytes of type Y that can then be used to allocate. Note. This get free bytes and the allocate sequence should be atomic.

Doesn't the approach of allowing the allocation to fail without an error actually make the call atomic,
without a requirement to make everything else atomic? I honestly doubt you could make this atomic with separate API calls, hence I agree with the approach of renaming "caps" to "flags" and adding a NOWARN flag.

[mrajwa]

I am also inclined towards re-designing CAPS flag to be more like generic allocation flag keeping information about memory regions requested + additional options like NO_WARN for example.

[lgirdwood]

@paulstelian97 @mrajwa seriously, where have you ever seen disable warning flags in posix like malloc(). Atmoc context locking is common place so really easy to lock if we have too. I'm not puting any garbage in to manage log level to alloc APIs for callers.

[paulstelian97]

@paulstelian97 @mrajwa seriously, where have you ever seen disable warning flags in posix like malloc(). Atmoc context locking is common place so really easy to lock if we have too. I'm not puting any garbage in to manage log level to alloc APIs for callers.

I'd argue that locking the whole allocator from code that isn't part of the allocator would seem out of place, and if you lock only inside the component who says that nobody unrelated will allocate from the same heap? I think any such locking to allow atomicity between test and actual allocation would be a lock of the entire heap, and I don't think there is any dedicated heap for such use cases which can be locked individually by non-allocator code.

Also, as @dbaluta said, GFP_NOWARN anyone?

[lgirdwood]

@paulstelian97 @mrajwa seriously, where have you ever seen disable warning flags in posix like malloc(). Atmoc context locking is common place so really easy to lock if we have too. I'm not puting any garbage in to manage log level to alloc APIs for callers.

I'd argue that locking the whole allocator from code that isn't part of the allocator would seem out of place, and if you lock only inside the component who says that nobody unrelated will allocate from the same heap? I think any such locking to allow atomicity between test and actual allocation would be a lock of the entire heap, and I don't think there is any dedicated heap for such use cases which can be locked individually by non-allocator code.

Also, as @dbaluta said, GFP_NOWARN anyone?

NAK. This is garbage and can lead to misleading trace/debug. If you dont want a warning then you must make sure the resource is available first.

[mrajwa]

@lgirdwood but this approach opens resource locking / atomic requests issue - you ask for XXXX bytes, get back positive response but in the meantime some higher priority requests kicks in and takes your memory pool. Same thing with multi core case. In the GFP_NOWARN you probe and allocate in one step.

[lgirdwood]

@mrajwa spinlock_irq() APIs prevents premption.

[paulstelian97]

@mrajwa spinlock_irq() APIs prevents premption.

What about multicore? How can we ensure ALL clients of the allocator will use the same spin lock and thus prevent each other from intervening?

[lgirdwood]

@mrajwa spinlock_irq() APIs prevents premption.

What about multicore? How can we ensure ALL clients of the allocator will use the same spin lock and thus prevent each other from intervening?

/* disables all IRQ sources and takes lock - enter atomic context */
#define spin_lock_irq(lock, flags) \
	do { \
		flags = interrupt_global_disable(); \
		spin_lock(lock); \
	} while (0)

This disables the IRQ premption on all cores but would also need to be embedded within malloc APIs i.e.

1. acquire global memory lock - IRQs now also OFF
2. get heap free/used data for type X
3. locally calculate and allocate heap type X
4. release global memory lock. - IRQs back on.

[paulstelian97]

@lgirdwood Well so you say that the memory allocator itself should expose either a global memory lock or a per-heap lock which you can lock externally, and that any client that wants to check if it can allocate a size, possibly optionally, should ask whether the allocation is possible then do it.

This is more complex and prone to bugs. Simply adding a flag to the allocator to not log as an error (it could still log as a regular trace! not all warnings are errors) is much simpler and with basically no risk of introducing new bugs with every such usage.

[paulstelian97]

@lgirdwood I believe you actually worry about Zephyr. Can you introduce me to the mechanisms within Zephyr which would allow implementing such a thing in either of Marcin or your proposed ways of handling this? You did mention POSIX but the reality is that we shouldn't sacrifice practicality just to be POSIX. The Linux kernel is as far from POSIX as it could ever be just to be able to implement a POSIX userspace, just saying.

[lgirdwood]

@paulstelian97 error messages must be deterministic so that integrators/debugers can easily scan the logs. Randomly switcing some messages on/off at runtime based on caller state will simply lead to confusion. Adding trace configuration data to memory APIs is out of context.
Please do suggest alternative solutions though, this is low priority non issue.

[paulstelian97]

@paulstelian97 error messages must be deterministic so that integrators/debugers can easily scan the logs. Randomly switcing some messages on/off at runtime based on caller state will simply lead to confusion. Adding trace configuration data to memory APIs is out of context.
Please do suggest alternative solutions though, this is low priority non issue.

Well that NOWARN flag would have been used only where failure is acceptable. Right now, failure
is never acceptable and is shown as an error, even if the component is not actually going to fail.
In my opinion, error traces should only appear when they are a reason of failure of some component or driver. Using such a flag is in fact the way to do this or, at least, the simplest way.

Are memory allocation failures even logged at all in POSIX, since you mention it? I guess not, but since we work with bare metal code many of these failures are non-recoverable so that's why we log. But the idea is to only log the failures that can't be recovered from, that are unusual, not those that are part of normal functioning (an allocation that can be done without).

I'd argue that having error messages enabled for stuff that isn't an error, a cause for an error return, is also confusing. So I'd still go with this, perhaps still having a trace for failure even when that flag
is set but no longer wording it as an error (allocation failure being still visible in the logs, but only
if you enable the regular traces instead of just the error traces)

[lgirdwood]

I'd argue that having error messages enabled for stuff that isn't an error, a cause for an error return, is also confusing. So I'd still go with this, perhaps still having a trace for failure even when that flag
is set but no longer wording it as an error (allocation failure being still visible in the logs, but only
if you enable the regular traces instead of just the error traces)

Why dont you submit a PR for multi-level log messages (like Linux) ? This sounds like it will fix the problem in a generic manner. Memory faiure could be a warn level ? i.e.

<Linux printk.h>
/*
 * These can be used to print at the various log levels.
 * All of these will print unconditionally, although note that pr_debug()
 * and other debug macros are compiled out unless either DEBUG is defined
 * or CONFIG_DYNAMIC_DEBUG is set.
 */
#define pr_emerg(fmt, ...) \
	printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
#define pr_alert(fmt, ...) \
	printk(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
#define pr_crit(fmt, ...) \
	printk(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
#define pr_err(fmt, ...) \
	printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
#define pr_warning(fmt, ...) \
	printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
#define pr_warn pr_warning
#define pr_notice(fmt, ...) \
	printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
#define pr_info(fmt, ...) \
	printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)```

[mrajwa]

@lgirdwood, what about such approach:

1. User wants to allocate 268 880 bytes from either memory pool. He starts with LPSRAM, so he locks this memory pool first and check if there is enough free memory there -> lock_lpsram(); free_size = get_free_size();
2. He then takes what is free and unlock this memory pool -> allocate_lpsram(free_size); unlock_lpsram()
3. He continues in the same way with other memory pools until he allocated enough or log and error message otherwise.

Would that be OK for you? Note it requires new interface for memory allocation as well as global locking mechanism.

Memory faiure could be a warn level ? i.e.

<Linux printk.h>
/*
 * These can be used to print at the various log levels.
 * All of these will print unconditionally, although note that pr_debug()
 * and other debug macros are compiled out unless either DEBUG is defined
 * or CONFIG_DYNAMIC_DEBUG is set.
 */
#define pr_emerg(fmt, ...) \
	printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
#define pr_alert(fmt, ...) \
	printk(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
#define pr_crit(fmt, ...) \
	printk(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
#define pr_err(fmt, ...) \
	printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
#define pr_warning(fmt, ...) \
	printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
#define pr_warn pr_warning
#define pr_notice(fmt, ...) \
	printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
#define pr_info(fmt, ...) \
	printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)

This would be an overkill and not even the solution to this issue - we need to decide under what circumstances log an error not what error/warning to log.

[kv2019i]

Hmm, I see merit on both approaches. On one hand, the NOWARN type of flag is alien to POSIX stacks, and e.g. Zephyr seems to follow this (emitting error messages is left for the client, not the allocator). For locking, POSIX has _try() variants, but this is not used for allocs. OTOH, GFP_NOWARN seems well established in Linux, and its use has stayed in hand. Documentation/core-api/memory-allocation.rst states that ""Allocations which have a reasonable fallback should be using GFP_NOWARN.""

But in any case, vast majority of allocs will be of the type "should-not-fail", so the interface should be sensible for this scenario. Adding mandatory locking to the caller for every alloc does not seem sane.
Having more fine-detailed log levels is probably somethign we should do, but seems orthogonal to this topic.

Do we have any other known needs to add a "flags" field. Zephyr btw does not have one, but Linux obviously has one. Just to add this one purpose, I don't think it makes sense to change the allocator API, but if we have other, maybe that helps to decide the best approach here.

[lgirdwood]

@mrajwa There is a feature issue now for multiple trace levels. Caller should use this to emit any warnings if allocations fail. I do not want to see any client code setting or changing log levels at runtime, this should all be driven by the user/integrator (via some SW IPC commands).

[jajanusz]

Topic stalled. If you'd like to come back to it please create new PR/Issue.