Skip to content
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.

Sign up for GitHub

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

Jump to bottom

os/pm: Add PM Thread Metrics #6286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+196 −11
Open

os/pm: Add PM Thread Metrics #6286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions os/include/tinyara/pm/pm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,22 @@ int pm_changestate(enum pm_state_e newstate);

enum pm_state_e pm_querystate(void);

/****************************************************************************
* Name: pm_recover
*
* Description:
* This function is called from task_recover() when a task is deleted via
* task_delete() or via pthread_cancel(). It does pm cleanup.
*
* Inputs:
* tcb - The TCB of the terminated task or thread
*
Copy link
Collaborator

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.

* Return Value:
* None.
*
****************************************************************************/
void pm_recover(FAR struct tcb_s *tcb);

#ifdef CONFIG_PM_DVFS
/****************************************************************************
* Name: pm_dvfs
Expand Down Expand Up @@ -656,6 +672,7 @@ int pm_metrics(int milliseconds);
#define pm_checkstate() (0)
#define pm_changestate(state) (0)
#define pm_querystate() (0)
#define pm_recover(tcb) (0)

#endif /* CONFIG_PM */

Expand Down
3 changes: 3 additions & 0 deletions os/kernel/task/task_recover.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
#include <tinyara/arch.h>
#include <tinyara/wdog.h>
#include <tinyara/sched.h>
#include <tinyara/pm/pm.h>

#include "semaphore/semaphore.h"
#include "wdog/wdog.h"
Expand Down Expand Up @@ -114,6 +115,8 @@

void task_recover(FAR struct tcb_s *tcb)
{
/* This task is being deleted. Do PM cleanup for this thread */
pm_recover(tcb);
gSahitya-samsung marked this conversation as resolved.
Show resolved Hide resolved
/* The task is being deleted. Cancel in pending timeout events. */

wd_recover(tcb);
Expand Down
1 change: 1 addition & 0 deletions os/pm/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ CSRCS += pm_changestate.c pm_checkstate.c pm_initialize.c pm_idle.c
CSRCS += pm_register.c pm_domain_register.c pm_unregister.c pm_procfs.c
CSRCS += pm_suspend.c pm_resume.c pm_timedsuspend.c
CSRCS += pm_suspendcount.c
CSRCS += pm_recover.c

ifeq ($(CONFIG_PM_TIMEDWAKEUP),y)
CSRCS += pm_sleep.c
Expand Down
35 changes: 34 additions & 1 deletion os/pm/pm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,40 @@ void pm_metrics_update_idle(void);
* None
*
****************************************************************************/
void pm_metrics_update_wakehandler(clock_t missing_tick, pm_wakeup_reason_code_t wakeup_src);
void pm_metrics_update_wakehandler(uint32_t missing_tick, pm_wakeup_reason_code_t wakeup_src);

/****************************************************************************
* Name: pm_metrics_update_sleep
*
* Description:
* This function is called inside pm_sleep's callback. It counts the frequency of
* thread which wakeup the board. It also checks the minimum amount of time board
* was in sleep because of given thread.
*
* Input parameters:
* pid - the ID of thread
*
* Returned value:
* None
*
****************************************************************************/
void pm_metrics_update_sleep(pid_t pid);

/****************************************************************************
* Name: pm_metrics_update_recover
*
* Description:
* This function is called inside pm_recover. It resets the wakeup_counts and
* sleep_ticks of given thread for consistent PM Metrics result.
*
* Input parameters:
* pid - the ID of thread
*
* Returned value:
* None
*
****************************************************************************/
void pm_metrics_update_recover(pid_t pid);
#endif

#undef EXTERN
Expand Down
88 changes: 81 additions & 7 deletions os/pm/pm_metrics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include <tinyara/config.h>
#include <tinyara/pm/pm.h>
#include <tinyara/sched.h>
#include <time.h>
#include <queue.h>
#include <debug.h>
Expand All @@ -39,9 +40,19 @@ struct pm_metric_state_s {

typedef struct pm_metric_state_s pm_metric_state_t;

struct pm_metric_thread_s {
pm_wakeup_reason_code_t wakeup_src; /* The wakeup source */
uint32_t wakeup_counts[CONFIG_MAX_TASKS]; /* Wakeup timer counts for each thread */
uint32_t min_sleep_ticks[CONFIG_MAX_TASKS]; /* Minimum actual sleep time allowed by each thread */
uint32_t delay; /* Wakeup timer delay (in ticks) */
};

typedef struct pm_metric_thread_s pm_metric_thread_t;

struct pm_metric_s {
pm_metric_domain_t domain_metrics; /* The domain metrics */
pm_metric_state_t state_metrics; /* The power management state metrics */
pm_metric_thread_t thread_metrics; /* The power management thread metrics */
uint32_t board_sleep_ticks; /* The amount of time (in ticks) board was in sleep */
uint32_t wakeup_src_counts[PM_WAKEUP_SRC_COUNT]; /* It counts the frequency of wakeup sources */
uint32_t total_try_ticks; /* Total duration of time pm tries to make board sleep */
Expand Down Expand Up @@ -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");
Copy link
Contributor

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.

pmdbg("\n");
pmdbg(" PROCESS NAME | WAKEUP COUNTS | MIN SLEEP TIME \n");
pmdbg("--------------|---------------|----------------\n");
for (index = 0; index < CONFIG_MAX_TASKS; index++) {
if (g_pm_metrics->thread_metrics.wakeup_counts[index]) {
pmdbg(" %12s | %13d | %12dms \n", sched_gettcb(index)->name, g_pm_metrics->thread_metrics.wakeup_counts[index], TICK2MSEC(g_pm_metrics->thread_metrics.min_sleep_ticks[index]));
}
}
}
pmdbg("\n");
pmdbg("\n");
pmdbg(" BOARD STATE | PM STATE | TIME \n");
Expand Down Expand Up @@ -117,7 +139,6 @@ void pm_metrics_update_domain(int domain_id)
{
if (g_pm_metrics_running) {
g_pm_metrics->domain_metrics.stime[domain_id] = clock_systimer();
g_pm_metrics->domain_metrics.suspend_ticks[domain_id] = 0;
}
}

Expand Down Expand Up @@ -230,14 +251,68 @@ void pm_metrics_update_changestate(void)
* None
*
****************************************************************************/
void pm_metrics_update_wakehandler(clock_t missing_tick, pm_wakeup_reason_code_t wakeup_src)
void pm_metrics_update_wakehandler(uint32_t missing_tick, pm_wakeup_reason_code_t wakeup_src)
{
if (g_pm_metrics_running) {
g_pm_metrics->thread_metrics.wakeup_src = wakeup_src;
g_pm_metrics->thread_metrics.delay = missing_tick;
g_pm_metrics->wakeup_src_counts[wakeup_src]++;
g_pm_metrics->board_sleep_ticks += missing_tick;
}
}

/****************************************************************************
* Name: pm_metrics_update_sleep
*
* Description:
* This function is called inside pm_sleep's callback. It counts the frequency of
* thread which wakeup the board. It also checks the minimum amount of time board
* was in sleep because of given thread.
*
* Input parameters:
* pid - the ID of thread
*
* Returned value:
* None
*
****************************************************************************/
void pm_metrics_update_sleep(pid_t pid)
{
int hash_pid;
if (g_pm_metrics_running && (g_pm_metrics->thread_metrics.wakeup_src == PM_WAKEUP_HW_TIMER)) {
hash_pid = PIDHASH(pid);
if (g_pm_metrics->thread_metrics.delay < g_pm_metrics->thread_metrics.min_sleep_ticks[hash_pid]) {
g_pm_metrics->thread_metrics.min_sleep_ticks[hash_pid] = g_pm_metrics->thread_metrics.delay;
}
g_pm_metrics->thread_metrics.wakeup_counts[hash_pid]++;
g_pm_metrics->thread_metrics.wakeup_src = PM_WAKEUP_UNKNOWN;
}
}

/****************************************************************************
* Name: pm_metrics_update_recover
*
* Description:
* This function is called inside pm_recover. It resets the wakeup_counts and
* sleep_ticks of given thread for consistent PM Metrics result.
*
* Input parameters:
* pid - the ID of thread
*
* Returned value:
* None
*
****************************************************************************/
void pm_metrics_update_recover(pid_t pid)
{
int hash_pid;
if (g_pm_metrics_running) {
hash_pid = PIDHASH(pid);
g_pm_metrics->thread_metrics.min_sleep_ticks[hash_pid] = UINT32_MAX;
Copy link
Contributor

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.

g_pm_metrics->thread_metrics.wakeup_counts[hash_pid] = 0;
}
}

/****************************************************************************
* Name: pm_metrics
*
Expand Down Expand Up @@ -280,16 +355,15 @@ int pm_metrics(int milliseconds)
return ERROR;
}
/* PM Metrics Initialization */
for (index = 0; index < PM_COUNT; index++) {
g_pm_metrics->state_metrics.state_accum_ticks[index] = 0;
}
flags = enter_critical_section();
start_time = clock_systimer();
g_pm_metrics->state_metrics.stime = start_time;
for (index = 0; (index < CONFIG_PM_NDOMAINS) && pm_domain_map[index]; index++) {
pm_metrics_update_domain(index);
g_pm_metrics->domain_metrics.stime[index] = start_time;
}
for (index = 0; index < CONFIG_MAX_TASKS; index++) {
g_pm_metrics->thread_metrics.min_sleep_ticks[index] = UINT32_MAX;
}
gSahitya-samsung marked this conversation as resolved.
Show resolved Hide resolved
g_pm_metrics_running = true;
leave_critical_section(flags);
/* Resume Board Sleep */
Expand Down Expand Up @@ -319,10 +393,10 @@ int pm_metrics(int milliseconds)
}
n_domains = index;
g_pm_metrics->state_metrics.state_accum_ticks[g_pmglobals.state] += end_time - g_pm_metrics->state_metrics.stime;
leave_critical_section(flags);
/* Show PM Metrics Results */
pm_print_metrics((double)(end_time - start_time), n_domains);
/* Free allocated memory */
leave_critical_section(flags);
free(g_pm_metrics);
g_pm_metrics = NULL;
/* Resume Board Sleep */
Expand Down
54 changes: 54 additions & 0 deletions os/pm/pm_recover.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/****************************************************************************
*
* Copyright 2024 Samsung Electronics All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
* either express or implied. See the License for the specific
* language governing permissions and limitations under the License.
*
****************************************************************************/
/****************************************************************************
* Included Files
****************************************************************************/

#include <tinyara/config.h>
#include <tinyara/pm/pm.h>

#include "pm.h"

#ifdef CONFIG_PM

/****************************************************************************
* Public Functions
****************************************************************************/

/****************************************************************************
* Name: pm_recover
*
* Description:
* This function is called from task_recover() when a task is deleted via
* task_delete() or via pthread_cancel(). It does pm cleanup.
*
* Inputs:
* tcb - The TCB of the terminated task or thread
*
* Return Value:
* None.
*
****************************************************************************/
void pm_recover(FAR struct tcb_s *tcb)
{
#ifdef CONFIG_PM_METRICS
pm_metrics_update_recover(tcb->pid);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

#endif
}

#endif /* CONFIG_PM */
7 changes: 5 additions & 2 deletions os/pm/pm_sleep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,11 @@
* Public Functions
************************************************************************/

static void pm_timer_callback(int argc, uint32_t sem)
static void pm_timer_callback(int argc, uint32_t sem, uint32_t pid)
{
#ifdef CONFIG_PM_METRICS
pm_metrics_update_sleep((pid_t)pid);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

#endif
/* As the timer is expired, give back the semaphore to unlock the thread */
sem_post((sem_t*)sem);
}
Expand Down Expand Up @@ -121,7 +124,7 @@ int pm_sleep(int milliseconds)
return ERROR;
}

ret = wd_start(wdog, MSEC2TICK(milliseconds), (wdentry_t)pm_timer_callback, 1, (uint32_t)&pm_sem);
ret = wd_start(wdog, MSEC2TICK(milliseconds), (wdentry_t)pm_timer_callback, 2, (uint32_t)&pm_sem, (uint32_t)getpid());
if (ret != OK) {
pmdbg("pm_sleep: wd_start failed\n");
wd_delete(wdog);
Expand Down
2 changes: 1 addition & 1 deletion os/pm/pm_wakehandler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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.

#endif
pmllvdbg("wakeup source code = %d\n", wakeup_src);
pmllvdbg("missing_tick: %llu\n", missing_tick);
Expand Down