Virtual Memory Heaps boot testing improvements #8775

dabekjakub · 2024-01-23T11:54:41Z

Change boot test trigger from ipc to zephyr API call.
Change existing vmh tests and expand it.

lyakh

would be better as a change, not a rewrite

zephyr/boot_test.c

zephyr/test/vmh.c

lyakh

extensive testing is good, but I think this is taking it a bit too far. And many indentation and curly brace issues

lyakh · 2024-01-24T08:32:17Z

zephyr/test/vmh.c

+	test_vmh_alloc_free(true);
+	test_vmh_alloc_free(false);
+	test_vmh_alloc_multiple_times(true);
+	test_vmh_alloc_multiple_times(false);


This is quite a bit of testing, taking into account that many of these functions test multiple allocations too. Do we know how long this takes? I'm not sure we need that much testing - I'd basically test each (or most) configuration once - once in contiguous mode, once without, once with success, once with failure. Something rather lightweight for regular debug-overlay testing, possibly add a new Kconfig for extensive VMH testing. So do all these tests now pass? Can we re-enable boot-time testing by default?

Overall I assumed that this boot test feature is not intended to be enabled in the productized version. So we will only use the CONFIG_SOF_BOOT_TEST flag for testing only and are not that much impacted by test time.

However, for allocator testing, this is quite concise. Should not take that much time - will have better data on it as soon as I clean up all the issues those tests found.

Amount of tests: This test approach allows for clear identification of the issues. I tried to add tests for each case I encountered. For example, multiple allocations failed in my internal testing during development so now I want it tested extensively. Writing data to the allocated space also checks for correctly assigned physical pages.

Can we enable boot testing? No - I am still working out the issues in vmh API #8772 Once this is done It can be pushed.

Good, that this helps you find issues with the VMH! Maybe we should first have all the issues fixed and then merge and enable this?

I am working on it so I see no problem In pushing both fixes and this at the same time.
Linked pr is for fixes I will do.

It does not matter how long the tests take to run as they should not be used in a production mode. i.e. we are not going to add latency to audio use case.
CI should be able to build this Kconfig and all tests should run for all subsystems.

I'd prefer to enable this in debug overlay, so this would get tested automatically with the current infra we have. That would require the tests to pass. Otherwise the code will very easily get broken (like it has when we temporarily disabled it in the debug overlay).

zephyr/test/vmh.c

lgirdwood

@dabekjakub is this bisect-able with the remove and add patches being different i.e. will it build/run ?
Also please state in commit message why new test are better etc, i.e. what are you adding that makes the test better.

zephyr/boot_test.c

lgirdwood

@dabekjakub commit message should say why we remove old test (i.e. what the gaps were) and why the new tests are better (what we test now and not before etc).

dabekjakub · 2024-02-19T18:20:38Z

@lgirdwood @lyakh After zephyr update there is a build error in the current implementation.

I will update PR tomorrow with both review fixes and new zephyr ztest suite run implementation.

Boot tests were dependent on ipc. Change to run theme as part of zephyr boot process. Calls to zephyr api were deprecated and would not build with current zephyr version. Update the call. Signed-off-by: Jakub Dabek <[email protected]>

Remove current tests for vmh api. To be replaced by new implementation. Old implementation is not parametrized and only checks one scenario: create heap and allocate on it. New implementation will cover multiple heap creation, multiple allocations, checking allocated memory for physical page allocation among other scenarios. Remove whole implementation since there is no code reuse. Signed-off-by: Jakub Dabek <[email protected]>

Add enhanced vmh testing. Add tests for: heap creation, heap creation with config, multiple allocation tests for each heap mode. Signed-off-by: Jakub Dabek <[email protected]>

dabekjakub · 2024-02-24T00:33:15Z

Fixed review issues and some test issues I have found.
Tested with #8849 worked.
As soon as this and #8849 is merged we can reenable boot tests. However, they do take a lot of time so if we enhance those we should think about separating it as a test-only build.

lyakh · 2024-02-26T08:54:39Z

However, they do take a lot of time so if we enhance those we should think about separating it as a test-only build.

@dabekjakub maybe we could have a define (Kconfig?) to select between a short and a long VMH test, a short one would skip most configurations and only test a couple of them

dabekjakub · 2024-02-26T11:14:40Z

It is a solution, however, this is outside of the scope and we would have to discuss it on an open forum, with people managing the validation of prs at least.

lgirdwood · 2024-02-26T16:36:19Z

zephyr/test/vmh.c

+	test_vmh_alloc_free(true);
+	test_vmh_alloc_free(false);
+	test_vmh_alloc_multiple_times(true);
+	test_vmh_alloc_multiple_times(false);


It does not matter how long the tests take to run as they should not be used in a production mode. i.e. we are not going to add latency to audio use case.
CI should be able to build this Kconfig and all tests should run for all subsystems.

kv2019i

Given the existing code doesn't build anymore, I'm ok to merge this version.

I do think we should enable this in some configuration CI tests (or add one, either in Intel internal/quickbuild). Otherwise the code will get broken and we won't know e.g. regressions with Zephyr. Having this enabled in CI would be very powerful e.g. to test Zephyr baseline updates.

kv2019i · 2024-03-05T09:55:42Z

zephyr/test/vmh.c

+	test_vmh_alloc_free(true);
+	test_vmh_alloc_free(false);
+	test_vmh_alloc_multiple_times(true);
+	test_vmh_alloc_multiple_times(false);


I'd prefer to enable this in debug overlay, so this would get tested automatically with the current infra we have. That would require the tests to pass. Otherwise the code will very easily get broken (like it has when we temporarily disabled it in the debug overlay).

dabekjakub requested review from kv2019i, lyakh, iuliana-prodan, dbaluta, abonislawski, lgirdwood, plbossart, mmaka1 and lbetlej as code owners January 23, 2024 11:54

dabekjakub force-pushed the vmh_tests_upstream branch from f5f7beb to 09dd76e Compare January 23, 2024 12:09

lyakh reviewed Jan 24, 2024

View reviewed changes

zephyr/boot_test.c Outdated Show resolved Hide resolved

zephyr/boot_test.c Outdated Show resolved Hide resolved

zephyr/test/vmh.c Outdated Show resolved Hide resolved

zephyr/test/vmh.c Show resolved Hide resolved

lyakh reviewed Jan 24, 2024

View reviewed changes

zephyr/test/vmh.c Outdated Show resolved Hide resolved

lyakh reviewed Jan 24, 2024

View reviewed changes

dabekjakub force-pushed the vmh_tests_upstream branch 7 times, most recently from 8102942 to 8d37d79 Compare January 26, 2024 00:51

lgirdwood reviewed Jan 26, 2024

View reviewed changes

dabekjakub force-pushed the vmh_tests_upstream branch 3 times, most recently from 0a3f7a9 to ae4d3cc Compare February 9, 2024 16:07

lyakh reviewed Feb 12, 2024

View reviewed changes

zephyr/boot_test.c Outdated Show resolved Hide resolved

lgirdwood reviewed Feb 14, 2024

View reviewed changes

dabekjakub mentioned this pull request Feb 19, 2024

Virtual memory heaps fixes #8849

Merged

dabekjakub force-pushed the vmh_tests_upstream branch 2 times, most recently from ddd8ae1 to 5b01043 Compare February 23, 2024 18:06

dabekjakub added 2 commits February 23, 2024 19:12

boot_test: change call of boot tests

ce824b5

Boot tests were dependent on ipc. Change to run theme as part of zephyr boot process. Calls to zephyr api were deprecated and would not build with current zephyr version. Update the call. Signed-off-by: Jakub Dabek <[email protected]>

dabekjakub force-pushed the vmh_tests_upstream branch 2 times, most recently from 2849279 to fec9649 Compare February 24, 2024 00:27

vmh_tests: Add set of parametrized tests and test suit

64bcd75

Add enhanced vmh testing. Add tests for: heap creation, heap creation with config, multiple allocation tests for each heap mode. Signed-off-by: Jakub Dabek <[email protected]>

dabekjakub force-pushed the vmh_tests_upstream branch from fec9649 to 64bcd75 Compare February 24, 2024 00:29

lgirdwood approved these changes Feb 26, 2024

View reviewed changes

kv2019i approved these changes Mar 5, 2024

View reviewed changes

mwasko merged commit dc8b367 into thesofproject:main Mar 5, 2024
35 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Virtual Memory Heaps boot testing improvements #8775

Virtual Memory Heaps boot testing improvements #8775

dabekjakub commented Jan 23, 2024

lyakh left a comment •

edited

Loading

lyakh left a comment

lyakh Jan 24, 2024

dabekjakub Jan 24, 2024

lyakh Jan 25, 2024

dabekjakub Jan 25, 2024

lgirdwood Feb 26, 2024

kv2019i Mar 5, 2024

lgirdwood left a comment

lgirdwood left a comment •

edited

Loading

dabekjakub commented Feb 19, 2024

dabekjakub commented Feb 24, 2024 •

edited

Loading

lyakh commented Feb 26, 2024

dabekjakub commented Feb 26, 2024

lgirdwood Feb 26, 2024

kv2019i left a comment

kv2019i Mar 5, 2024

Virtual Memory Heaps boot testing improvements #8775

Virtual Memory Heaps boot testing improvements #8775

Conversation

dabekjakub commented Jan 23, 2024

lyakh left a comment • edited Loading

Choose a reason for hiding this comment

lyakh 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

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood left a comment • edited Loading

Choose a reason for hiding this comment

dabekjakub commented Feb 19, 2024

dabekjakub commented Feb 24, 2024 • edited Loading

lyakh commented Feb 26, 2024

dabekjakub commented Feb 26, 2024

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

lyakh left a comment •

edited

Loading

lgirdwood left a comment •

edited

Loading

dabekjakub commented Feb 24, 2024 •

edited

Loading