I/O Performance Measurement: Add I/O Performance Monitor #9330

tobonex · 2024-07-25T15:44:01Z

Implements I/O performance measurement feature. It counts number of transmitted bytes or other events that happen in various interfaces. Currently measured interfaces: IPC, IDC, SNDW, HDA, SSP, DMIC.

There are some interfaces that are defined in Reference Firmware that apparently are not used directly here: USB, I2C, I3C, UART, CSI_2 , DTF. But I could not confirm this as of yet.

There are some other minor issues I will mention in comments.

tobonex

Some comments from me.

src/ipc/ipc-common.c

tobonex · 2024-07-25T15:49:50Z

src/debug/telemetry/performance_monitor.c

+		return;
+
+	struct io_perf_monitor_ctx *self = &perf_monitor_ctx;
+	k_spinlock_key_t key = k_spin_lock(&self->lock);


I'm not sure about this, this lock was there in reference (not sure what kind of a lock it was), but I think this has potential to disrupt the measured interfaces with a spinlock here. The entries to which we write are just statically defined memory, assigned to a module from performance monitor, and released by the module itself, so I don't think the global lock is needed.

CI answered my own question, smokes did not pass. Both this and release slot function cannot be locked. io_perf_monitor_release_slot has locks in the bitmap functions, so it should be safe.

lgirdwood

Mostly minor opens.

lgirdwood · 2024-08-05T12:22:15Z

src/debug/telemetry/performance_monitor.c

+struct io_perf_monitor_ctx {
+	struct k_spinlock lock;
+	enum ipc4_perf_measurements_state_set state;
+	struct perf_bitmap io_performance_data_bitmap;//??????


Probably need to explain this i.e. its it unused but needed for backwards compat we should say so.

Ah, I do use it. It may be unnecessary though, as this only points to static memory.

src/debug/telemetry/performance_monitor.c

lgirdwood · 2024-08-05T12:25:37Z

src/debug/telemetry/performance_monitor.c

+int io_perf_monitor_init_data(struct io_perf_data_item **slot_id,
+			      struct io_perf_data_item *init_data)
+{
+	if (!slot_id)


I would put a log() here since its coming from host SW.

lgirdwood · 2024-08-05T12:26:01Z

src/debug/telemetry/performance_monitor.c

+	struct io_perf_monitor_ctx *io_perf_monitor = &perf_monitor_ctx;
+	struct io_perf_data_item *new_slot = io_perf_monitor_get_next_slot(io_perf_monitor);
+
+	if (!new_slot)


same here for log(), anything from host could be wrong.

lgirdwood · 2024-08-05T12:27:34Z

src/debug/telemetry/performance_monitor.c

+		return;
+
+	struct io_perf_monitor_ctx *self = &perf_monitor_ctx;
+	//k_spinlock_key_t key = k_spin_lock(&self->lock);


locking needed ? If multicore you could use an atomic add increment ?

Given "data" is 64bit, atomics won't work, but it would seem locking is probably not needed given a single slot is only used for one interface.

Yes, if only one instance uses a specific slot this should be ok. I'm not 100% sure if this was the original design but it is a thing now. I did add a comment in .h now to make this apparent.

src/include/sof/debug/telemetry/performance_monitor.h

src/audio/base_fw.c

lgirdwood · 2024-08-05T12:32:23Z

src/idc/zephyr_idc.c

@@ -88,6 +90,11 @@ static void idc_handler(struct k_p4wq_work *work)
 	idc->received_msg.header = msg->header;
 	idc->received_msg.extension = msg->extension;

+#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS


I would avoid the #ifdefs in client to maintain readability, i.e. the telemetry header could have the #ifdef and just declare some empty static inline stubs if its not enabled in Kconfig

src/ipc/ipc-common.c

kv2019i

Multiple minor comments. The locking is a bit hard to follow, but otherwise looks ok.

src/debug/telemetry/Kconfig

kv2019i · 2024-08-06T15:20:53Z

src/debug/telemetry/performance_monitor.c

+		return;
+
+	struct io_perf_monitor_ctx *self = &perf_monitor_ctx;
+	//k_spinlock_key_t key = k_spin_lock(&self->lock);


Given "data" is 64bit, atomics won't work, but it would seem locking is probably not needed given a single slot is only used for one interface.

kv2019i · 2024-08-06T15:23:46Z

src/debug/telemetry/performance_monitor.c

+
+	item->is_removed = true;
+
+	int idx = (item - self->io_perf_data) / sizeof(*item);


Wouldn't this be one place where locking is needed?

I added lock for this line alone, we'll see if we get any problems in smoke tests. This function did deadlock when i locked all the code here, but it's probably because bitmap has it's own locks too.

app/boards/intel_adsp_ace15_mtpm.conf

lyakh · 2024-08-12T11:54:23Z

src/debug/telemetry/performance_monitor.c

+
+	ret = perf_bitmap_setbit(&self->io_performance_data_bitmap, idx);
+	if (ret < 0)
+		return NULL;


free the bitmap?

If setbit fails, free also fails (wrong idx)

lyakh · 2024-08-12T11:56:21Z

src/debug/telemetry/performance_monitor.c

+	item->is_removed = true;
+
+	key = k_spin_lock(&self->lock);
+	idx = (item - self->io_perf_data) / sizeof(*item);


this looks wrong - the subtraction returns the number of structures, not bytes

lyakh · 2024-08-12T11:57:38Z

src/debug/telemetry/performance_monitor.c

+
+	ret = perf_bitmap_clearbit(&self->io_performance_data_bitmap, idx);
+	if (ret < 0)
+		return ret;


this is questionable - whether you should still try to free the bitmap

This only fails if idx is wrong - "free" will also fail in this case.

and idx can only be wrong if item is wrong, and you've already used it above to set item->is_removed = true. So, if item can really be wrong, can we check it first before modifying anything? And if it cannot be wrong, then we should probably handle "impossible" cases differently here - either just print a big scary error message or even use an assert()

Yeah, there's no check for null., and there should be one, added. As item is within a certain memory range we could check if it is within it also.

lyakh · 2024-08-12T12:00:53Z

src/debug/telemetry/performance_monitor.c

+}
+
+static int
+io_perf_monitor_stopped(struct io_perf_monitor_ctx *self)


this never returns errors, so can be void. Besides the name sounds like it's checking whether the monitor has stopped, while this isn't what this function is doing

lyakh · 2024-08-12T12:21:56Z

src/audio/base_fw.c

+#endif
+}
+
+static int io_global_perf_data_get(uint32_t *data_off_size, char *data)


why char? what does it have to do with characters / strings?

A better question would be: why all the other handlers have this kind of interface

lyakh · 2024-08-12T12:26:04Z

src/ipc/ipc-common.c

@@ -317,6 +336,11 @@ static enum task_state ipc_do_cmd(void *data)
 {
 	struct ipc *ipc = data;

+#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
+	/* Increment performance counters */
+	io_perf_monitor_update_data(ipc->io_perf_in_msg_count, 1);


why IPC commands are counted for performance? In some use-cases there are many IPCs, in others none, like plain playback or capture?

In ref. FW this feature did also count some events like IPC IDC and some interrupts.

lyakh · 2024-08-12T12:28:49Z

src/idc/idc.c

@@ -481,6 +483,11 @@ static void idc_complete(void *data)
 	uint32_t type = iTS(idc->received_msg.header);
 	k_spinlock_key_t key;

+#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
+	/* Increment performance counters */
+	io_perf_monitor_update_data(idc->io_perf_out_msg_count, 1);


same here, how does the number of IDCs describe IO performance?

src/audio/dai-zephyr.c

lyakh · 2024-08-12T12:33:58Z

src/audio/dai-zephyr.c

+		io_perf_monitor_init_data(&dd->io_perf_bytes_count, &init_data);
+	}
+#endif
+
 	return 0;


also in the INVALID case?

Yes, we should treat performance as something optional. Perf setup failing should not make everything else fail

ok, then maybe at least print a warning in the default case at line 509 above - not a blocker, can be a follow up patch

lyakh · 2024-08-30T06:19:47Z

src/debug/telemetry/performance_monitor.c

+
+	ret = perf_bitmap_clearbit(&self->io_performance_data_bitmap, idx);
+	if (ret < 0)
+		return ret;


and idx can only be wrong if item is wrong, and you've already used it above to set item->is_removed = true. So, if item can really be wrong, can we check it first before modifying anything? And if it cannot be wrong, then we should probably handle "impossible" cases differently here - either just print a big scary error message or even use an assert()

lyakh · 2024-08-30T06:22:16Z

src/debug/telemetry/performance_monitor.c

+	if (ret < 0)
+		return ret;
+
+	return 0;


btw, this should've been

return perf_bitmap_free(&performance_data_bitmap, idx);

but if you agree to use asserts, then something like

ret = perf_bitmap_free(&performance_data_bitmap, idx); assert(!ret); return ret;

You could also consider return 0 but then if built with asserts disabled, the compiler might complain about assigned but not used ret

lgirdwood

@tobonex I assume when IO is enabled, the the data will be gathered on every IO action and will be reset when D3. Btw, it occurs to me now we are gathering a lot of good data than we can also additionally now dump via the Zephyr shell. @kv2019i

src/debug/telemetry/performance_monitor.c

src/include/sof/debug/telemetry/performance_monitor.h

lgirdwood · 2024-09-02T13:13:34Z

src/debug/telemetry/performance_monitor.c

@@ -433,7 +433,8 @@ int io_perf_monitor_init(void)
 }

 /* init performance monitor using Zephyr */
-SYS_INIT(io_perf_monitor_init, APPLICATION, CONFIG_APPLICATION_INIT_PRIORITY);
+/* IMPORTANT , this init is now in IPC init, temporarily */


btw, we should also state the transition plan in the comment so that everyone know when this will be resolved.

Well, this is connected to the io_perf_monitor_init comment at the top, not sure if we have a nice place for the init. Leaving it near ipc_init works, but it's a bit ugly.

If we don't have a better place for this init, I will just leave it near ipc_init for now, and merge this temporary commit to another, or rename it.

kv2019i

A few minor comments, but nothing blocking merge.

src/debug/telemetry/Kconfig

src/debug/telemetry/performance_monitor.c

src/ipc/ipc-common.c

lgirdwood · 2024-09-10T14:34:59Z

Lot of failures, could have been lab power off event. rerun CI.

lgirdwood · 2024-09-10T14:35:06Z

SOFCI TEST

tobonex · 2024-09-11T10:07:57Z

Some CI fails are due to newly added asserts failing. I'll take a look.

Adds config option for enabling I/O performance. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

Implements I/O performance measurement feature. It counts number of transmitted bytes or other events that happen in various interfaces. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

Adds IO_PERF_MEASUREMENTS_STATE and IO_GLOBAL_PERF_DATA IPCs. Those can be used to change state of I/O performance monitor and extract the measured data. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

Set up a measurement of number of input and output IPCs. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

Set up a counter of input and output IDCs. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

Adds I/O performance measurement for audio unterfaces in DAI (SNDW, DMIC, SSP, HDA). Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

Enable I/O performance for mtl and lnl Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

Zephyr's bitarray had a problem when clearing the bit before freeing it. Not clearing it changes nothing in overall logic so we can just not do it. Also fixes and issue with computing bitmap idx. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

tobonex commented Jul 25, 2024

View reviewed changes

tobonex force-pushed the telemetry_global_io_perf_final branch 5 times, most recently from 48f43da to 55dab6c Compare July 26, 2024 17:23

tobonex marked this pull request as ready for review July 30, 2024 15:40

tobonex requested review from kv2019i, lyakh, iuliana-prodan, dbaluta, abonislawski, bardliao, marcinszkudlinski, pblaszko, lgirdwood, plbossart, mmaka1 and lbetlej as code owners July 30, 2024 15:40

lgirdwood reviewed Aug 5, 2024

View reviewed changes

kv2019i reviewed Aug 6, 2024

View reviewed changes

tobonex force-pushed the telemetry_global_io_perf_final branch from 55dab6c to dea7611 Compare August 8, 2024 14:24

lyakh reviewed Aug 12, 2024

View reviewed changes

tobonex force-pushed the telemetry_global_io_perf_final branch from dea7611 to 2cf375d Compare August 29, 2024 13:24

lyakh reviewed Aug 30, 2024

View reviewed changes

tobonex force-pushed the telemetry_global_io_perf_final branch from 2cf375d to a611cc2 Compare August 30, 2024 15:22

lgirdwood reviewed Sep 2, 2024

View reviewed changes

kv2019i approved these changes Sep 3, 2024

View reviewed changes

src/debug/telemetry/Kconfig Outdated Show resolved Hide resolved

src/debug/telemetry/performance_monitor.c Show resolved Hide resolved

src/ipc/ipc-common.c Outdated Show resolved Hide resolved

tobonex force-pushed the telemetry_global_io_perf_final branch from a611cc2 to 08b6164 Compare September 4, 2024 14:52

gbernatxintel approved these changes Sep 10, 2024

View reviewed changes

softwarecki approved these changes Sep 10, 2024

View reviewed changes

tobonex force-pushed the telemetry_global_io_perf_final branch from 08b6164 to 38c8379 Compare September 13, 2024 14:35

tobonex added 7 commits September 16, 2024 18:14

I/O performance monitor: add config for io performance measurements

9227d6f

Adds config option for enabling I/O performance. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

I/O performance monitor: Add I/O performance monitor backend

007c4d9

Implements I/O performance measurement feature. It counts number of transmitted bytes or other events that happen in various interfaces. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

I/O performance monitor: Add I/O performance related IPCs

620c69b

Adds IO_PERF_MEASUREMENTS_STATE and IO_GLOBAL_PERF_DATA IPCs. Those can be used to change state of I/O performance monitor and extract the measured data. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

I/O performance monitor: add I/O performance counters to IPC interface

2a1689c

Set up a measurement of number of input and output IPCs. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

I/O performance monitor: Add I/O measurements for IDC interface

300d56e

Set up a counter of input and output IDCs. Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

I/O performance monitor: Add I/O measurement for DAI

ef30a1d

Adds I/O performance measurement for audio unterfaces in DAI (SNDW, DMIC, SSP, HDA). Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

I/O performance monitor: enable I/O performance measurements

aee690a

Enable I/O performance for mtl and lnl Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>

tobonex force-pushed the telemetry_global_io_perf_final branch from 38c8379 to f7d2315 Compare September 17, 2024 07:35

tobonex force-pushed the telemetry_global_io_perf_final branch from f7d2315 to cade98a Compare September 17, 2024 08:46

lgirdwood approved these changes Sep 17, 2024

View reviewed changes

kv2019i merged commit 1fe942f into thesofproject:main Sep 18, 2024
44 of 47 checks passed


		item->is_removed = true;

		int idx = (item - self->io_perf_data) / sizeof(*item);

I/O Performance Measurement: Add I/O Performance Monitor #9330

I/O Performance Measurement: Add I/O Performance Monitor #9330

Conversation

tobonex commented Jul 25, 2024

tobonex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobonex Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

lgirdwood commented Sep 10, 2024

lgirdwood commented Sep 10, 2024

tobonex commented Sep 11, 2024

tobonex Aug 8, 2024 •

edited

Loading