Skip to content

performance: perf cnt bugfix and add more macros for message print #6353

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

Merged
kv2019i merged 1 commit into from
Oct 18, 2022
Merged

performance: perf cnt bugfix and add more macros for message print #6353

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: 11 additions & 6 deletions xtos/include/sof/lib/perf_cnt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ struct perf_cnt_data {
#define perf_trace_simple(pcd, arg) perf_cnt_trace(arg, pcd)

#if CONFIG_PERFORMANCE_COUNTERS_RUN_AVERAGE

/* perf measurement windows size 2^x */
#define PERF_CNT_CHECK_WINDOW_SIZE 10

Expand All @@ -95,6 +94,8 @@ struct perf_cnt_data {
trace_m(pcd, arg); \
(pcd)->cpu_delta_sum = 0; \
(pcd)->sample_cnt = 0; \
(pcd)->plat_delta_peak = 0; \
(pcd)->cpu_delta_peak = 0; \
} \
} while (0)

Expand All @@ -115,12 +116,16 @@ struct perf_cnt_data {
(uint32_t)sof_cycle_get_64(); \
uint32_t cpu_ts = \
(uint32_t)perf_cnt_get_cpu_ts(); \
if ((pcd)->plat_ts) { \
if (plat_ts > (pcd)->plat_ts) \
(pcd)->plat_delta_last = plat_ts - (pcd)->plat_ts; \
(pcd)->cpu_delta_last = cpu_ts - (pcd)->cpu_ts; \
} \
(pcd)->plat_ts = plat_ts; \
(pcd)->cpu_ts = cpu_ts; \
Copy link
Collaborator

@lyakh lyakh Oct 10, 2022

Choose a reason for hiding this comment

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

lines 122 and 123 aren't needed? Are .plat_ts and .cpu_ts updated elsewhere now?

Copy link
Contributor Author

@btian1 btian1 Oct 10, 2022

Choose a reason for hiding this comment

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

line 122 and 123 will be updated with perf_cnt_init before each module beginning, then after each module, will calculate the diff and decide the data.

Copy link
Contributor Author

@btian1 btian1 Oct 10, 2022

Choose a reason for hiding this comment

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

this already split from original #6322 , how about keep it, too much prs will make upstream more difficult? it is not complicate actually, regarding change to inline, due to still pass function into the macro, how about keep for now?
I even want to rewrite perf_cnt, but due to I am new, and don't know too much about history, so I change based on current, in future, if there is new request, I may rewrite this module.

Copy link
Collaborator

@lyakh lyakh Oct 11, 2022

Choose a reason for hiding this comment

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

sorry, I don't understand. perf_cnt_init() initialises .plat_ts and .cpu_ts but when calculating the current deltas, you have to compare to the previous call, not to the initialisation values? What am I missing?

Copy link
Contributor Author

@btian1 btian1 Oct 11, 2022

Choose a reason for hiding this comment

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

in: perf_cnt_init()
get initial clock for plat_ts and cpu_ts.
then followed by function calling.
....
then get current clock with local plat_ts and cpu_ts
then get the delta and peak for this function calling.
record the delta and accumulated it with 1024 times to get average.

it will loop around per 1 seconds.

Copy link
Collaborator

@lyakh lyakh Oct 11, 2022

Choose a reason for hiding this comment

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

Ok, I see now, so it looks like it is redundant in the current implementation? Would be good to have that change - just removing those two lines as a separate commit, but well, at least I know now why they are removed, thanks

Copy link
Collaborator

@kv2019i kv2019i Oct 11, 2022

Choose a reason for hiding this comment

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

Hmm, sorry, I don't understand how this works. At every function we calculate "(pcd)->plat_delta_last = plat_ts - (pcd)->plat_ts;" , but if "(pcd)->plat_ts" is not update (as removed in this patch), the delta is not correctly calculated (it's delta to initial value, not delta to previous call). Did I miss where a new place where "(pcd)->plat_ts" is updated?

Copy link
Collaborator

@kv2019i kv2019i Oct 11, 2022

Choose a reason for hiding this comment

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

Thanks @btian1 , so the old code really was wrong.

else \
(pcd)->plat_delta_last = UINT32_MAX - (pcd)->plat_ts \
+ plat_ts; \
if (cpu_ts > (pcd)->cpu_ts) \
(pcd)->cpu_delta_last = cpu_ts - (pcd)->cpu_ts; \
else \
(pcd)->cpu_delta_last = UINT32_MAX - (pcd)->cpu_ts \
+ cpu_ts;\
if ((pcd)->plat_delta_last > (pcd)->plat_delta_peak) \
(pcd)->plat_delta_peak = (pcd)->plat_delta_last; \
if ((pcd)->cpu_delta_last > (pcd)->cpu_delta_peak) { \
Expand Down