You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/07/22 07:41:45 UTC
[GitHub] [incubator-nuttx] Donny9 opened a new pull request, #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Donny9 opened a new pull request, #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685
## Summary
1. Output memory info about each task when malloc failed.
* I think outputting each task information directly is more useful than dumping the entire heap, because it can be clear know who is occupying the most memory blocks, who may leak, and if there is too much dump heap, it may trigger wdt interrupt.
example:
2. change the type config MM_BACKTRACE from bool to int.
Initially, the entire backtrace function relied on DEBUG_MM, but due to the conflict with memory-related log output control, it was replaced with a new config MM_BACKTRACE.
* Therefore, this config will control the trace function, including pid information and backtrace information, but some platforms have limited resources, and these two information need to be controlled separately, so thid PR will do this:
* when MM_BACKTRACE = -1, disable trace function
* when MM_BACKTRACE = 0, only enable pid information for trac function.
* when MM_BACKTRACE = 8, enbale entir trace function. 8 is the default backtrace stack depth, if you need a larger depth, you need to update MM_MIN_SHIFT in mm/mm_heap/mm.h synchronously.
## Impact
memory debug function
## Testing
Vlea CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
fjpanag commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1193962303
> If we use another option to enable the pid track, the code will become more complex, because user can enable backtrace, but disable pid.
I don't think this is a real problem.
You can do it like this:
```
config MM_OWNERSHIP
bool "Owner tracking"
default n
---help---
Enables ownership tracking in heap allocations.
config MM_BACKTRACE
bool "Backtrace"
default n
depends on MM_OWNERSHIP && DEBUG_MM
---help---
Enables backtrace printing.
```
The dependencies are resolved by Kconfig.
The code is much cleaner, I believe, and the features are enabled in *compile* time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1195225949
Unfortunately we do not have any board configuration with `CONFIG_MM_BACKTRACE`, so CI can't validate changes.
@xiaoxiang781216 do you know if we already have `CONFIG_MM_BACKTRACE` in 10.3 release? I mean do we need to add "breaking change" label or not?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1192479844
> @Donny9 I haven't have the chance to actually test this, but I have a proposition:
>
> `CONFIG_MM_BACKTRACE` enables two things now. It controls both whether we have information about the owner of a memory block (and thus enables the the relevant information in `ps`), and also produces the dump.
>
> I believe that these are different functions, and should be controlled independently.
>
> More specifically, the ownership information should _not_ depend on `CONFIG_DEBUG_MM`. This is useful information that can exist in `proc`, regardless of any debugging taking place. A task may need to know how much heap it consumes. I propose to make this a new Kconfig option.
>
> On the other hand, the backtrace _is_ a debugging feature, that may be enabled/disabled dependently, when actually needed. I believe that only this should depend on `CONFIG_DEBUG_MM`, and of course to the above new option.
>
> What do you think?
@fjpanag yes, This pr has modified COFNIG_MM_BACKTRACE not to depends on CONFIG_DEBUGMM, is a completely independent config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1193978412
> > If we use another option to enable the pid track, the code will become more complex, because user can enable backtrace, but disable pid.
>
> I don't think this is a real problem.
>
> You can do it like this:
>
> ```
>
> config MM_OWNERSHIP
> bool "Owner tracking"
> default n
> ---help---
> Enables ownership tracking in heap allocations.
>
> config MM_BACKTRACE
> bool "Backtrace"
> default n
> depends on MM_OWNERSHIP && DEBUG_MM
> ---help---
> Enables backtrace printing.
> ```
>
> The dependencies are resolved by Kconfig. The code is much cleaner, I believe, and the features are enabled in _compile_ time.
If using different config to enable pid and backtrace, we need to divide a lot of conditional macros for procfs and memdump functions in file:fs/procfs/fs_procfsmeminfo.c,fs/procfs/fs_procfsproc.c,mm/mm_heap/mm_memdump.c, that is, we need to replace #if CONFIG_MM_BACKTRACE >= 0 in this PR with two conditions, it is complicated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1192283673
Please @fjpanag @yamt @pkarashchenko review, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
fjpanag commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1192448593
@Donny9 I haven't have the chance to actually test this, but I have a proposition:
`CONFIG_MM_BACKTRACE` enables two things now. It controls both whether we have information about the owner of a memory block (and thus enables the the relevant information in `ps`), and also produces the dump.
I believe that these are different functions, and should be controlled independently.
More specifically, the ownership information should *not* depend on `CONFIG_DEBUG_MM`. This is useful information that can exist in `proc`, regardless of any debugging taking place. A task may need to know how much heap it consumes. I propose to make this a new Kconfig option.
On the other hand, the backtrace *is* a debugging feature, that may be enabled/disabled dependently, when actually needed. I believe that only this should depend on `CONFIG_DEBUG_MM`, and of course to the above new option.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#discussion_r929718076
##########
mm/mm_heap/mm_memdump.c:
##########
@@ -65,27 +65,31 @@ static void memdump_handler(FAR struct mm_allocnode_s *node, FAR void *arg)
if ((node->preceding & MM_ALLOC_BIT) != 0)
{
DEBUGASSERT(node->size >= SIZEOF_MM_ALLOCNODE);
-#ifndef CONFIG_MM_BACKTRACE
+#if CONFIG_MM_BACKTRACE < 0
if (info->pid == -1)
#else
if (info->pid == -1 || node->pid == info->pid)
#endif
{
-#ifndef CONFIG_MM_BACKTRACE
+#if CONFIG_MM_BACKTRACE < 0
syslog(LOG_INFO, "%12zu%*p\n",
(size_t)node->size, MM_PTR_FMT_WIDTH,
((FAR char *)node + SIZEOF_MM_ALLOCNODE));
#else
+# if CONFIG_MM_BACKTRACE > 0
int i;
FAR const char *format = " %0*p";
- char buf[MM_BACKTRACE_DEPTH * MM_PTR_FMT_WIDTH + 1];
+# endif
+ char buf[CONFIG_MM_BACKTRACE * MM_PTR_FMT_WIDTH + 1];
buf[0] = '\0';
- for (i = 0; i < MM_BACKTRACE_DEPTH && node->backtrace[i]; i++)
+# if CONFIG_MM_BACKTRACE > 0
+ for (i = 0; i < CONFIG_MM_BACKTRACE && node->backtrace[i]; i++)
{
sprintf(buf + i * MM_PTR_FMT_WIDTH, format,
MM_PTR_FMT_WIDTH - 1, node->backtrace[i]);
}
+# endif
syslog(LOG_INFO, "%6d%12zu%*p%s\n",
(int)node->pid, (size_t)node->size, MM_PTR_FMT_WIDTH,
Review Comment:
```suggestion
node->pid, (size_t)node->size, MM_PTR_FMT_WIDTH,
```
unrelated to this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1192289832
https://github.com/apache/incubator-nuttx-apps/pull/1231 is relate to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
fjpanag commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1192655903
> Because backtrace without pid is useless in many case.
I am talking about pid without backtrace, not the other way around.
Also, I think it would be best to have discreet and meaningful options (or just a choice item), instead of using some random numbers with no obvious meaning...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1195651446
> Unfortunately we do not have any board configuration with `CONFIG_MM_BACKTRACE`, so CI can't validate changes. @xiaoxiang781216 do you know if we already have `CONFIG_MM_BACKTRACE` in 10.3 release? I mean do we need to add "breaking change" label or not?
it is added by @yamt after 10.3 release: https://github.com/apache/incubator-nuttx/pull/6305.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1192552599
@fjpanag as @Donny9 said the useful tracking info is separated from the debug option, but pid and backtrace is still share the same option(MM_BACKTRACE) as below:
1. -1 mean disable both pid and backtrace
2. 0 track pid only
3. > 0 track pid and backtrace
Because backtrace without pid is useless in many case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#issuecomment-1192743311
> > Because backtrace without pid is useless in many case.
>
> I am talking about pid without backtrace, not the other way around.
>
you can get pid by set CONFIG_MM_BACKTRACE to 0.
> Also, I think it would be best to have discreet and meaningful options (or just a choice item), instead of using some random numbers with no obvious meaning...
If we use another option to enable the pid track, the code will become more complex, because user can enable backtrace, but disable pid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6685: mm/mm_heap: update MM_BACKTRACE and malloc failed output
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6685:
URL: https://github.com/apache/incubator-nuttx/pull/6685#discussion_r930129085
##########
mm/mm_heap/mm_memdump.c:
##########
@@ -65,27 +65,31 @@ static void memdump_handler(FAR struct mm_allocnode_s *node, FAR void *arg)
if ((node->preceding & MM_ALLOC_BIT) != 0)
{
DEBUGASSERT(node->size >= SIZEOF_MM_ALLOCNODE);
-#ifndef CONFIG_MM_BACKTRACE
+#if CONFIG_MM_BACKTRACE < 0
if (info->pid == -1)
#else
if (info->pid == -1 || node->pid == info->pid)
#endif
{
-#ifndef CONFIG_MM_BACKTRACE
+#if CONFIG_MM_BACKTRACE < 0
syslog(LOG_INFO, "%12zu%*p\n",
(size_t)node->size, MM_PTR_FMT_WIDTH,
((FAR char *)node + SIZEOF_MM_ALLOCNODE));
#else
+# if CONFIG_MM_BACKTRACE > 0
int i;
FAR const char *format = " %0*p";
- char buf[MM_BACKTRACE_DEPTH * MM_PTR_FMT_WIDTH + 1];
+# endif
+ char buf[CONFIG_MM_BACKTRACE * MM_PTR_FMT_WIDTH + 1];
buf[0] = '\0';
- for (i = 0; i < MM_BACKTRACE_DEPTH && node->backtrace[i]; i++)
+# if CONFIG_MM_BACKTRACE > 0
+ for (i = 0; i < CONFIG_MM_BACKTRACE && node->backtrace[i]; i++)
{
sprintf(buf + i * MM_PTR_FMT_WIDTH, format,
MM_PTR_FMT_WIDTH - 1, node->backtrace[i]);
}
+# endif
syslog(LOG_INFO, "%6d%12zu%*p%s\n",
(int)node->pid, (size_t)node->size, MM_PTR_FMT_WIDTH,
Review Comment:
@Donny9 please create a new patch to fix this, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org