Skip to content

Conversation

cvinayak
Copy link
Contributor

@cvinayak cvinayak commented May 3, 2019

Add stack usage logging in idle thread. Enabled by
CONFIG_INIT_STACKS and CONFIG_THREAD_MONITOR.

Signed-off-by: Vinayak Kariappa Chettimada vich@nordicsemi.no

Add stack usage logging in idle thread. Enabled by
CONFIG_INIT_STACKS and CONFIG_THREAD_MONITOR.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak cvinayak added the RFC Request For Comments: want input from the community label May 3, 2019
@cvinayak cvinayak requested review from carlescufi and jhedberg May 3, 2019 13:36
@jhedberg jhedberg requested review from andrewboie, ceolin and nashif May 3, 2019 13:38
STACK_ANALYZE("interrupt stack", _interrupt_stack);
STACK_ANALYZE("sys work q stack", sys_work_q_stack);
STACK_ANALYZE("main stack", _main_stack);
STACK_ANALYZE("idle stack", _idle_stack);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do the above four threads get special treatment? Wont they be included in the k_thread_foread()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, its not included... that what needs to be debugged. May the code owner for the thread monitor feature needs to intervene here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above four threads might be "special" to the kernel? They are in any case essential to the RTOS so it's critical that we get their info printed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is worth to check why they are not included in k_treadh_foreach()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvinayak @ceolin Not sure why they weren't included when you tried this. Could it have been related to this: #23196?

I do get them now without that change.

#else
for (;;) {
#if defined(CONFIG_INIT_STACKS)
if (k_uptime_get_32() - idle_ts > K_SECONDS(5)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this kind of periodic logging the period should probably be made configurable through Kconfig. E.g. if the shell is enabled one might want to disable this completely and just rely on the explicit "kernel stacks" command.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Will try to remember to fix once its figured out how to get all the threads enumerated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, must be possible to configure it in build time. Make clear that this is an idle thread and there is not time guarantee.

@carlescufi carlescufi requested a review from ioannisg May 3, 2019 13:51
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I like the idea, I am not sure we should pollute the idle thread with this. Maybe it would be better to provide a thread-dumping kernel call that the user can then call at will from main()? perhaps even a variant that never returns and uses k_sleep(), so that the user can write:

main()
{
   init_my_app()

   k_stacks();
   /* unreachable */
}

and then in k_stacks():

while(1) {
  print_stack_info()
  k_sleep(K_STACKS_FREQUENCY);
}

@carlescufi
Copy link
Member

Or maybe even a Kconfig option that automatically calls k_stacks() at the end of main:
CONFIG_LOG_STACKS or similar.

@nashif
Copy link
Member

nashif commented May 3, 2019

#3082 for reference

@nashif
Copy link
Member

nashif commented May 3, 2019

While I like the idea, I am not sure we should pollute the idle thread with this.

agree.

We probably should have a stack monitoring thread that can be a bit more intelligent and send notifications only when things above a certain threshold . Does not mynewt have something like that btw?

LOG_MODULE_DECLARE(kernel, CONFIG_KERNEL_LOG_LEVEL);

const char *name = k_thread_name_get((struct k_thread *)thread);
unsigned int size = thread->stack_info.size;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use the same type of stack_info u32_t. int size can change ...

@ceolin
Copy link
Member

ceolin commented May 17, 2019

Or maybe even a Kconfig option that automatically calls k_stacks() at the end of main:
CONFIG_LOG_STACKS or similar.

agree, both options are better than introduce a new feature under the INIT_STACKS option.

@cvinayak
Copy link
Contributor Author

Nothing has happened since May 2019, closing this until this is picked up again for implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RFC Request For Comments: want input from the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants