-
Notifications
You must be signed in to change notification settings - Fork 573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
os/pm: Add PM Thread Metrics #6286
base: master
Are you sure you want to change the base?
Conversation
|
os/pm/pm_metrics.c
Outdated
} | ||
} | ||
pmdbg("\n"); | ||
pmdbg("\n"); | ||
pmdbg(" BOARD STATE | PM STATE | TIME \n"); | ||
pmdbg("-------------|----------|------------------------\n"); | ||
pmdbg(" %11s | %8s | %10dms (%6.2f%%) \n", "WAKEUP", "NORMAL", TICK2MSEC(g_pm_metrics->state_metrics.state_accum_ticks[0]),\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to change "WAKEUP" -> "ACTIVE"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pmdbg(" %11s | %8s | %10dms (%6.2f%%) \n", "SLEEP", "SLEEP", TICK2MSEC(g_pm_metrics->board_sleep_ticks),\
((double)g_pm_metrics->board_sleep_ticks) * 100.0 / total_time);
Suggest to change "SLEEP" -> "INACTIVE"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, system active corresponds one of 3 PM states.
System inactive corresponds to PM_SLEEP
* | ||
* Inputs: | ||
* tcb - The TCB of the terminated task or thread | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gSahitya-samsung use code-i feature to generate the doxzgen documentation for the interfaces and update the description if it not suiting better.(/docstring) which is very productive.
@@ -56,7 +56,7 @@ void pm_wakehandler(clock_t missing_tick, pm_wakeup_reason_code_t wakeup_src) | |||
{ | |||
irqstate_t flags = enter_critical_section(); | |||
#ifdef CONFIG_PM_METRICS | |||
pm_metrics_update_wakehandler(missing_tick, wakeup_src); | |||
pm_metrics_update_wakehandler((uint32_t)missing_tick, wakeup_src); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do missing_tick is typecased to uint32_t?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is missing_tick
is of type uint64
while pm_metrics
support only uint32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to organize the type in pm_metrics.
please check and let me know the maximum time supported by pm_metrics.
You can limit all actions to the maximum time and change the data type correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ewoodev We can scale pm_metrics
to UINT64_MAX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, currently our pm_metrics use pm_sleep. This is the maximum int max of delay.
Therefore, the pm_metrics input is int max tick.
is it necessary for the count and ticks of pm_metrics to maintain int max tick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it is highly unlikely board will sleep continuously for INT32_MAX
i.e. 24 days
.Since we have UART
and Wifi
Interrupts, it guarantee the board will wakeup at least one time within 24 days sleep. So we can scale our pm_metrics
to UINT64_MAX
.
@@ -56,7 +56,7 @@ void pm_wakehandler(clock_t missing_tick, pm_wakeup_reason_code_t wakeup_src) | |||
{ | |||
irqstate_t flags = enter_critical_section(); | |||
#ifdef CONFIG_PM_METRICS | |||
pm_metrics_update_wakehandler(missing_tick, wakeup_src); | |||
pm_metrics_update_wakehandler((uint32_t)missing_tick, wakeup_src); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to organize the type in pm_metrics.
please check and let me know the maximum time supported by pm_metrics.
You can limit all actions to the maximum time and change the data type correctly.
void pm_recover(FAR struct tcb_s *tcb) | ||
{ | ||
#ifdef CONFIG_PM_METRICS | ||
pm_metrics_update_recover(tcb->pid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can task exited using PIDHASH is same
For only pm_metrics, do we need to add operation in task exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but in future we can use pm_recover
for other pm cleanup activities which may be require after task termination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, when add new future, add this also is better.
for pm_metrics side, It seems better to keep the data until the next thread is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this way, then we may need to keep thread_name
in memory. Can we do this?
This commit extends the functionality of pm_metrics. Now PM Metrics show the thread specific pm metrics. It shows which process causes minimum actual sleep time (in ms) and how frequently the thread wakes up the board.
int hash_pid; | ||
if (g_pm_metrics_running) { | ||
hash_pid = PIDHASH(pid); | ||
g_pm_metrics->thread_metrics.min_sleep_ticks[hash_pid] = UINT32_MAX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can modify this data when the next pid comes in.
For this, do not add action to task end.
{ | ||
#ifdef CONFIG_PM_METRICS | ||
pm_metrics_update_sleep((pid_t)pid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please find wd_setwakeupsource()
we can use timer as wakeup source using api rather than pm_sleep.
Could you consider how to add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently whole pm_metrics
logic is inside PM module, if we want to move logic into wd_setwakeupsource
then we need to change pm interface and give pm_metrics
API.
@@ -80,6 +91,17 @@ static void pm_print_metrics(double total_time, int n_domains) | |||
for (index = 0; index < PM_WAKEUP_SRC_COUNT; index++) { | |||
pmdbg(" %14s | %6d \n", wakeup_src_name[index], g_pm_metrics->wakeup_src_counts[index]); | |||
} | |||
if (g_pm_metrics->wakeup_src_counts[PM_WAKEUP_HW_TIMER]) { | |||
pmdbg("\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding new definition for pmdbg_noarg (based on dbg_noarg) and use that to print these mertrics? When we are printing a table, then we dont need the api name at the beginning of each line of the table.
This commit extends the functionality of pm_metrics. Now PM Metrics show the thread specific pm metrics. It shows which process causes minimum actual sleep time (in ms) and how frequently the thread wakes up the board.