Logging: switch to delayed work #9929
Conversation
Make the IPC sending work queue available to other IPC parts by moving it to the global IPC context. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
requested review from
bardliao,
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
marcinszkudlinski,
mmaka1,
pblaszko and
plbossart
as code owners
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
A few notes/questions, otherwise looks good.
src/ipc/ipc4/logging.c
Outdated
| #define IPC4_MTRACE_AGING_TIMER_MIN_MS 100 | ||
|
|
||
| SOF_DEFINE_REG_UUID(mtrace_task); | ||
| //SOF_DEFINE_REG_UUID(mtrace_task); |
There was a problem hiding this comment.
@kv2019i left-over, sorry. Looks like this only was needed for logging / the EDF task - if at all? But then we'd also remove it from the UUID registry? Or is it needed for anything?
src/ipc/ipc4/logging.c
Outdated
| #include <ipc4/base_fw.h> | ||
| #include <ipc4/error_status.h> | ||
| #include <ipc4/logging.h> | ||
| #include <zephyr/kernel.h> |
There was a problem hiding this comment.
Not 100% sure this is ok for all built targest. In theory IPC4 could be used with XTOS, but in practise very unlikely.
src/ipc/ipc4/logging.c
Outdated
| */ | ||
| if (arch_proc_id() != MTRACE_IPC_CORE) | ||
| if (arch_proc_id() != MTRACE_IPC_CORE) { | ||
| k_mutex_unlock(&log_mutex); |
There was a problem hiding this comment.
Isn't this a double unlock as the parent call unlock as well?
There was a problem hiding this comment.
argh, right, another one
| mtrace_notify_last_sent = k_uptime_get(); | ||
| mtrace_bytes_pending = 0; | ||
| } else { | ||
| k_work_schedule_for_queue(&ipc_get()->ipc_send_wq, &log_work, |
There was a problem hiding this comment.
This looks good. The ipc send_wq is pinned to core 0, so this does indeed work.
| return k_uptime_ticks() + k_ms_to_ticks_ceil64(mtrace_aging_timer); | ||
| k_mutex_lock(&log_mutex, K_FOREVER); | ||
|
|
||
| if (k_uptime_get() - mtrace_notify_last_sent >= mtrace_aging_timer && |
There was a problem hiding this comment.
Why you need to check this? Shouldn't Zephyr's Delayable Work call this handler no sooner than a timeout?
There was a problem hiding this comment.
.. and there is yet another check in mtrace_log_hook_unlocked(). This check look quite redundant to me too.
There was a problem hiding this comment.
I think both are needed. This one is needed because it could happen that after the work was queued another log was issued and then the space was tight so an IPC got sent out and the time updated, so now the time isn't larger than the threshold. And the other one is needed for normal hook operation - if it isn't called from the delayed work, but enough time has passed since the last notification, we'll notify now.
No need for an EDF task to schedule delayed logging notification IPC, a one-shot delayed work is enough. Also make sure to protect the hook execution against potential preemption. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
CI:
|
|
SOFCI TEST |
|
The HDA/SDW DUTs not available in https://sof-ci.01.org/sofpr/PR9929/build11893/devicetest/index.html . Let me rekick the tests once more. |
|
SOFCI TEST |
|
CI:
all known and unrelated |
5 participants
Switch from an EDF task to a delayed task for logging notification IPC