Fix Zbus Benchmark Sample is smashing the stack and is too fast #53563

alpsayin · 2023-01-06T16:31:37Z

ZBus subscriber threads were smashing their threads. Upped their stack with something safe by utilising IDLE_STACK_SIZE for subscribers and MAIN_STACK_SIZE for producer.

Benchmark also finishes way too fast for most if not all targets. Used cycles instead of uptime as originally suggested by @rodrigopex and used nanosecond conversions to get around the duration==0 check.

Native posix still returns start and end times as 0 though, so with further commits utilised native_rtc with RTC_CLOCK_PSEUDOHOSTREALTIME to get as much resolution as possible.

alpsayin · 2023-01-06T18:17:24Z

@rodrigopex for reviews?

rodrigopex

Only small changes are necessary.

samples/subsys/zbus/benchmark/src/benchmark.c

rodrigopex · 2023-01-06T18:33:43Z

@alpsayin maybe we could leave the duration zero occurs. The only thing we need to do is to avoid the zero division and force fand i also to be zero. What do you think? And that would remove platform_exclude: native_posix from the sample yaml you are suggesting.

asayin-xlnx · 2023-01-06T23:32:25Z

@alpsayin maybe we could leave the duration zero occurs. The only thing we need to do is to avoid the zero division and force fand i also to be zero. What do you think? And that would remove platform_exclude: native_posix from the sample yaml you are suggesting.

I found a somewhat reliable way to measure time for native_posix. "Reliable" is when you up the BYTES_TO_BE_SENT to MBs then duration becomes consistently directly proportional (as one would expect) to BYTES_TO_BE_SENT. With 256kB, the deviation is about 30% on the machine that I'm testing (workstation with 0 load).

Eyeballed means/deviations are 30+/-10us for native_posix_64 and 35+/-5us for native_posix.

Let's see what CI/CD will end up showing.

rodrigopex

I would avoid abbreviations for one-time used variables and declare variables inside ifdefs.

samples/subsys/zbus/benchmark/src/benchmark.c

Stack sizes need to be dependent on something arch specific such as idle stack size because they're smashing their stacks. Signed-off-by: Alp Sayin <alp.sayin@amd.com>

…time Needed because benchmark finishes faster than a tick, especially on emulated platforms where tick rate is 100Hz. Output units are also updated to print numbers with less digits. Signed-off-by: Alp Sayin <alp.sayin@amd.com>

carlescufi · 2023-01-09T11:16:32Z

@alpsayin can you please fix the failing checks?

rodrigopex

Just waiting for @cfriedt to comment on the native_posix get-time approach.

rodrigopex · 2023-01-09T11:18:17Z

samples/subsys/zbus/benchmark/src/benchmark.c

+
+#if defined(CONFIG_ARCH_POSIX)
+#include "native_rtc.h"
+#define GET_ARCH_TIME_NS() (native_rtc_gettime_us(RTC_CLOCK_PSEUDOHOSTREALTIME) * NSEC_PER_USEC)


@cfriedt Could you please give us your opinion about line 17? Is this a good practice for native_posix?

alpsayin · 2023-01-09T11:22:48Z

Hi @carlescufi thanks for showing interest.
I think 1st failure here is a checkpatch bug, checkpatch either complains line's too long or I'm "unnecessarily" breaking it. Please see in fact our discussion on 2nd line. What should I do?
https://github.com/zephyrproject-rtos/zephyr/actions/runs/3871989306/jobs/6603001449#step:9:31
#53563 (comment)

K_THREAD_DEFINE(producer_thread_id, PRODUCER_STACK_SIZE, producer_thread, NULL, NULL, NULL, 5, 0, 0);

2nd failure I see here is just me being somehow very unlucky:
https://github.com/zephyrproject-rtos/zephyr/actions/runs/3871989309/jobs/6603001548#step:5:54

Err:1 http://azure.archive.ubuntu.com/ubuntu focal/universe amd64 ocaml-base-nox amd64 4.08.1-8
  503  Service Unavailable [IP: 20.106.104.242 80]
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/universe/o/ocaml/ocaml-base-nox_4.08.1-8_amd64.deb  503  Service Unavailable [IP: 20.106.104.242 80]

rodrigopex · 2023-01-09T11:25:19Z

2nd failure I see here is just me being somehow very unlucky: https://github.com/zephyrproject-rtos/zephyr/actions/runs/3871989309/jobs/6603001548#step:5:54
Err:1 http://azure.archive.ubuntu.com/ubuntu focal/universe amd64 ocaml-base-nox amd64 4.08.1-8
  503  Service Unavailable [IP: 20.106.104.242 80]
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/universe/o/ocaml/ocaml-base-nox_4.08.1-8_amd64.deb  503  Service Unavailable [IP: 20.106.104.242 80]

@alpsayin

Let's try it again in some minutes.

carlescufi · 2023-01-09T18:37:22Z

I think 1st failure here is a checkpatch bug, checkpatch either complains line's too long or I'm "unnecessarily" breaking it. Please see in fact our discussion on 2nd line. What should I do?

I think we can let this one go, but double-checking with @stephanosio

samples/subsys/zbus/benchmark/src/benchmark.c

keith-packard · 2023-01-09T20:45:51Z

samples/subsys/zbus/benchmark/src/benchmark.c

+#include "native_rtc.h"
+#define GET_ARCH_TIME_NS() (native_rtc_gettime_us(RTC_CLOCK_PSEUDOHOSTREALTIME) * NSEC_PER_USEC)
+#else
+#define GET_ARCH_TIME_NS() (k_cyc_to_ns_near32(sys_clock_cycle_get_32()))


I was seeing issues on the ARC QEMU platform where the tick count runs at 100Hz; does this API provide a higher resolution value for that case?

Seems so

-- west build: running target run_qemu [0/1] To exit from QEMU enter: 'CTRL+a, x'[QEMU] CPU: arcem *** Booting Zephyr OS build zephyr-v3.2.0-3158-g7bb6557ba710 *** I: Benchmark 1 to 8: Dynamic memory, SYNC transmission and message size 256 I: Bytes sent = 262144, received = 262144 I: Average data rate: 2.11MB/s I: Duration: 118061.400us @118061400

[0/1] To exit from QEMU enter: 'CTRL+a, x'[QEMU] CPU: archs *** Booting Zephyr OS build zephyr-v3.2.0-3158-g7bb6557ba710 *** I: Benchmark 1 to 8: Dynamic memory, SYNC transmission and message size 256 I: Bytes sent = 262144, received = 262144 I: Average data rate: 2.12MB/s I: Duration: 117709.400us @117709400

[0/1] To exit from QEMU enter: 'CTRL+a, x'[QEMU] CPU: hs6x *** Booting Zephyr OS build zephyr-v3.2.0-3158-g7bb6557ba710 *** I: Benchmark 1 to 8: Dynamic memory, SYNC transmission and message size 256 I: Bytes sent = 262144, received = 262144 I: Average data rate: 3.12MB/s I: Duration: 80011.600us @80011600

stephanosio · 2023-01-09T23:26:07Z

I think we can let this one go, but double-checking with @stephanosio

The package download failure has been fixed and can be ignored. The LINE_CONTINUATIONS failure, however, is valid -- it is warning that \ is not necessary and should be removed.

…POSIX Because posix emulated execution is significantly faster than posix emulated timer and benchmark finishes before even the first tick occurs. Signed-off-by: Alp Sayin <alp.sayin@amd.com>

The sample is now able to run on all previously excluded platforms. Signed-off-by: Alp Sayin <alp.sayin@amd.com>

alpsayin · 2023-01-10T07:34:02Z

Of course 🤦🏼 Dont need \ when inside parenthesis...
Thanks both!

alpsayin · 2023-01-10T13:01:33Z

@rodrigopex I have a feeling I should also compare ticks for good measure.
I couldn't trust that cycles isn't going to overflow.
Some numbers I saw are 100s of Mbytes/s whereas arc returned 2-3.
Can we really expect this much deviation?

carlescufi · 2023-01-10T13:42:25Z

Merging this now, additional improvements can be done in a subsequent PR.

rodrigopex · 2023-01-10T14:04:53Z

@rodrigopex I have a feeling I should also compare ticks for good measure. I couldn't trust that cycles isn't going to overflow. Some numbers I saw are 100s of Mbytes/s whereas arc returned 2-3. Can we really expect this much deviation?

@alpsayin I need to think more about it. We can discuss it in the following days. Ok?

keith-packard · 2023-01-10T22:23:25Z

This doesn't fix the problem on qemu_nios2 where the cycle counter resolution is the the same as the system clock (100Hz by default). Cranking up the tick rate (CONFIG_SYS_CLOCK_TICKS_PER_SEC) to 1kHz fixes the problem.

Output with minimal libc (with 1kHz tick):

*** Booting Zephyr OS build zephyr-v3.2.0-3310-g4ec18e5fbf52 ***
I: Benchmark 1 to 8: Dynamic memory, SYNC transmission and message size 256
I: Bytes sent = 262144, received = 262144
I: Average data rate: 7.35MB/s
I: Duration: 34000.0us

@34000000

Output with picolibc (with 1kHz tick):

*** Booting Zephyr OS build zephyr-v3.2.0-3310-g4ec18e5fbf52 ***
I: Benchmark 1 to 8: Dynamic memory, SYNC transmission and message size 256
I: Bytes sent = 262144, received = 262144
I: Average data rate: 62.50MB/s
I: Duration: 4000.0us

@4000000

As you can see, when using picolibc, the test completes in 4ms, well under the 10ms system tick.

You should be able to verify this using this command:

$ west build -b qemu_nios2 samples/subsys/zbus/benchmark -T sample.zbus.benchmark_sync -- -DCONFIG_PICOLIBC=y

I'll submit another PR that just increases the TICK value for these tests.

rodrigopex · 2023-01-10T22:49:07Z

That would be great.

alpsayin · 2023-01-12T09:10:26Z

@rodrigopex I have a feeling I should also compare ticks for good measure. I couldn't trust that cycles isn't going to overflow. Some numbers I saw are 100s of Mbytes/s whereas arc returned 2-3. Can we really expect this much deviation?

@alpsayin I need to think more about it. We can discuss it in the following days. Ok?

I checked, and we get away with (mostly by chance). In the slowest platforms, tests still finish before cycles can roll over.
It also seems nanoseconds was a bit of an overkill but oh well ¯\_(ツ)_/¯

alpsayin changed the title ~~Fix Zbus Sample is smashing the stack and too fast~~ Fix Zbus Sample is smashing the stack and is too fast Jan 6, 2023

alpsayin changed the title ~~Fix Zbus Sample is smashing the stack and is too fast~~ Fix Zbus Benchmark Sample is smashing the stack and is too fast Jan 6, 2023

alpsayin marked this pull request as ready for review January 6, 2023 18:13

alpsayin requested a review from nashif as a code owner January 6, 2023 18:13

rodrigopex requested changes Jan 6, 2023

View reviewed changes

samples/subsys/zbus/benchmark/src/benchmark.c Outdated Show resolved Hide resolved

samples/subsys/zbus/benchmark/src/benchmark.c Outdated Show resolved Hide resolved

asayin-xlnx force-pushed the zbus_fix branch from d125018 to d3b24fd Compare January 6, 2023 23:25

asayin-xlnx force-pushed the zbus_fix branch from d3b24fd to d5d46eb Compare January 6, 2023 23:38

alpsayin mentioned this pull request Jan 7, 2023

MicroBlaze Port #53576

Draft

rodrigopex requested changes Jan 7, 2023

View reviewed changes

samples/subsys/zbus/benchmark/src/benchmark.c Outdated Show resolved Hide resolved

samples/subsys/zbus/benchmark/src/benchmark.c Outdated Show resolved Hide resolved

samples/subsys/zbus/benchmark/src/benchmark.c Outdated Show resolved Hide resolved

alpsayin added 2 commits January 7, 2023 20:49

samples: subsys: zbus: benchmark: increase thread stack sizes

3edb17e

Stack sizes need to be dependent on something arch specific such as idle stack size because they're smashing their stacks. Signed-off-by: Alp Sayin <alp.sayin@amd.com>

asayin-xlnx force-pushed the zbus_fix branch from d5d46eb to 8d75060 Compare January 7, 2023 20:55

rodrigopex previously approved these changes Jan 7, 2023

View reviewed changes

alpsayin mentioned this pull request Jan 9, 2023

subsys/zbus: benchmark test runs "too fast" with picolibc #53589

Closed

asayin-xlnx dismissed rodrigopex’s stale review via 7bb6557 January 9, 2023 08:24

asayin-xlnx force-pushed the zbus_fix branch from 8d75060 to 7bb6557 Compare January 9, 2023 08:24

carlescufi requested review from rodrigopex and asayin-xlnx and removed request for asayin-xlnx January 9, 2023 11:14

rodrigopex requested review from cfriedt and removed request for asayin-xlnx January 9, 2023 11:14

rodrigopex reviewed Jan 9, 2023

View reviewed changes

rodrigopex approved these changes Jan 9, 2023

View reviewed changes

rodrigopex requested changes Jan 9, 2023

View reviewed changes

samples/subsys/zbus/benchmark/src/benchmark.c Outdated Show resolved Hide resolved

keith-packard reviewed Jan 9, 2023

View reviewed changes

alpsayin added 2 commits January 10, 2023 07:32

samples: subsys: zbus: benchmark: use native posix host rtc for ARCH_…

d1b8f38

…POSIX Because posix emulated execution is significantly faster than posix emulated timer and benchmark finishes before even the first tick occurs. Signed-off-by: Alp Sayin <alp.sayin@amd.com>

samples: subsys: zbus: benchmark: remove all platform exclusions

ca04ea9

The sample is now able to run on all previously excluded platforms. Signed-off-by: Alp Sayin <alp.sayin@amd.com>

asayin-xlnx force-pushed the zbus_fix branch from 7bb6557 to ca04ea9 Compare January 10, 2023 07:32

rodrigopex approved these changes Jan 10, 2023

View reviewed changes

carlescufi requested review from asayin-xlnx and keith-packard and removed request for asayin-xlnx January 10, 2023 13:40

carlescufi approved these changes Jan 10, 2023

View reviewed changes

carlescufi assigned rodrigopex Jan 10, 2023

carlescufi merged commit 345b3bc into zephyrproject-rtos:main Jan 10, 2023

keith-packard mentioned this pull request Jan 10, 2023

sample/zbus: qemu_nios2 needs faster tick for zbus test with picolibc #53684

Closed

alpsayin deleted the zbus_fix branch January 11, 2023 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Zbus Benchmark Sample is smashing the stack and is too fast #53563

Fix Zbus Benchmark Sample is smashing the stack and is too fast #53563

alpsayin commented Jan 6, 2023 •

edited

Loading

alpsayin commented Jan 6, 2023

rodrigopex left a comment

rodrigopex commented Jan 6, 2023 •

edited

Loading

asayin-xlnx commented Jan 6, 2023

rodrigopex left a comment

carlescufi commented Jan 9, 2023

rodrigopex left a comment

rodrigopex Jan 9, 2023

alpsayin commented Jan 9, 2023

rodrigopex commented Jan 9, 2023

carlescufi commented Jan 9, 2023

keith-packard Jan 9, 2023

alpsayin Jan 10, 2023

stephanosio commented Jan 9, 2023

alpsayin commented Jan 10, 2023

alpsayin commented Jan 10, 2023

carlescufi commented Jan 10, 2023

rodrigopex commented Jan 10, 2023

keith-packard commented Jan 10, 2023

rodrigopex commented Jan 10, 2023

alpsayin commented Jan 12, 2023

Fix Zbus Benchmark Sample is smashing the stack and is too fast #53563

Fix Zbus Benchmark Sample is smashing the stack and is too fast #53563

Conversation

alpsayin commented Jan 6, 2023 • edited Loading

alpsayin commented Jan 6, 2023

rodrigopex left a comment

Choose a reason for hiding this comment

rodrigopex commented Jan 6, 2023 • edited Loading

asayin-xlnx commented Jan 6, 2023

rodrigopex left a comment

Choose a reason for hiding this comment

carlescufi commented Jan 9, 2023

rodrigopex left a comment

Choose a reason for hiding this comment

rodrigopex Jan 9, 2023

Choose a reason for hiding this comment

alpsayin commented Jan 9, 2023

rodrigopex commented Jan 9, 2023

carlescufi commented Jan 9, 2023

keith-packard Jan 9, 2023

Choose a reason for hiding this comment

alpsayin Jan 10, 2023

Choose a reason for hiding this comment

stephanosio commented Jan 9, 2023

alpsayin commented Jan 10, 2023

alpsayin commented Jan 10, 2023

carlescufi commented Jan 10, 2023

rodrigopex commented Jan 10, 2023

keith-packard commented Jan 10, 2023

rodrigopex commented Jan 10, 2023

alpsayin commented Jan 12, 2023

alpsayin commented Jan 6, 2023 •

edited

Loading

rodrigopex commented Jan 6, 2023 •

edited

Loading