Skip to content

userspace: reduce CI userspace testing to tests that use it and fix some network tests #15230

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

Jump to bottom
Merged
merged 7 commits into from
Apr 6, 2019
Merged

userspace: reduce CI userspace testing to tests that use it and fix some network tests #15230

merged 7 commits into from
Apr 6, 2019

Conversation

andrewboie
Copy link
Contributor

  • CONFIG_TEST_USERSPACE is now off by default. It should be enabled for any test that tries to put a thread in user mode (K_USER option to k_thread_create()). A special assertion has been added to enforce this. This should greatly decrease CI build times.

  • Missing system call for gethostname() added.

  • The getnameinfo() test case now runs in user mode as intended.

  • tests/kernel/poll uses a lot less RAM by recycling stack/thread objects

Fixes: #15228
Fixes: #15227
Partial fix for: #8559 (we eventually need to look into making gen_kobject_list.py faster)
Fixes: #15103

Andrew Boie added 4 commits April 5, 2019 16:12
tests: poll: reduce memory usage
40cdd79 
We can re-use thread/stack objects between cases, no
need to have separate ones.

Signed-off-by: Andrew Boie <[email protected]>
tests: net: getnameinfo: run in user mode
e8ad181 
The test is designed to run in user mode, but was disabled
for some reason.

Re-enable, and add mps2_an385 to the whitelist as this is
the QEMU target that emulates an MPU.

Fixes: zephyrproject-rtos#15228

Signed-off-by: Andrew Boie <[email protected]>
net: add missing syscall for gethostname()
4a0cbd9 
We need all the socket APIs to work from user mode.
tests/net/socket/misc now runs in userspace.

Fixes: zephyrproject-rtos#15227

Signed-off-by: Andrew Boie <[email protected]>
tests: set userspace tag for all tests that use it
92af83d 
This lets us quickly filter tests that exercise userspace
when developing it.

Some tests had a whitelist with qemu_cortex_m3; change
this to mps2_an385, which is the QEMU target with an
MPU enabled.

Signed-off-by: Andrew Boie <[email protected]>
@andrewboie andrewboie force-pushed the userspace-testing-overhaul branch 2 times, most recently from e7579de to 8d99ea6 Compare April 5, 2019 23:22
@andrewboie andrewboie added the bug The issue is a bug, or the PR is fixing a bug label Apr 5, 2019
@andrewboie andrewboie added this to the v1.14.0 milestone Apr 5, 2019
andyross
andyross approved these changes Apr 5, 2019
View reviewed changes
Copy link
Collaborator

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Makes sense. A few nitpicks.

tests/kernel/poll/src/test_poll.c
@@ -15,11 +15,16 @@ struct fifo_msg {

#define SIGNAL_RESULT 0x1ee7d00d
#define FIFO_MSG_VALUE 0xdeadbeef
#define STACK_SIZE (1024 + CONFIG_TEST_EXTRA_STACKSIZE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've done similar stuff in other tests to fold stacks together. Maybe we should have some kind of ztest framework for this or something.

kernel/thread.c
/* Special case, only for unit tests */
#if defined(CONFIG_TEST) && defined(CONFIG_ARCH_HAS_USERSPACE) && !defined(CONFIG_USERSPACE)
__ASSERT((options & K_USER) == 0,
"Platform is capable of user mode, and test thread created with K_USER option, but CONFIG_TEST_USERSPACE or CONFIG_USERSPACE is not set\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be cleaner to just make it a general assertion if the caller asks for K_USER but the kernel can't accommodate? Or do we document that as a valid noop? Seems like there's nothing really test-specific about this validation other than the reason it was added.

Copy link
Contributor Author

@andrewboie andrewboie Apr 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

For general usage, it's valid to disable userspace and still use things like K_USER or other related APIs. They are no-ops.

However, just for tests, we want to tighten this up as if someone is dropping a test to user mode in a test case, but forgets to set CONFIG_TEST_USERSPACE, it will not silently just run everything in supervisor mode.

We could consider making this a general assertion, but I am hesitant to do this here because we are so close to release. Re-visit for 1.15?

Andrew Boie added 3 commits April 5, 2019 17:06
tests: bluetooth: tester: workaround build error
f81e03d 
For mysterious reasons this test fails cmake if
CONFIG_TEST_USERSPACE=n.

Enable it for now, bug tracked in zephyrproject-rtos#15232

Signed-off-by: Andrew Boie <[email protected]>
tests: CONFIG_TEST_USERSPACE now off by default
71689d2 
Unlike CONFIG_HW_STACK_PROTECTION, which greatly helps
expose stack overflows in test code, activating
userspace without putting threads in user mode is of
very limited value.

Now CONFIG_TEST_USERSPACE is off by default. Any test
which puts threads in user mode will need to set
CONFIG_TEST_USERSPACE.

This should greatly increase sanitycheck build times
as there is non-trivial build time overhead to
enabling this feature. This also allows some tests
which failed the build on RAM-constrained platforms
to compile properly.

tests/drivers/build_all is a special case; it doesn't
put threads in user mode, but we want to ensure all
the syscall handlers compile properly.

Fixes: zephyrproject-rtos#15103 (and probably others)

Signed-off-by: Andrew Boie <[email protected]>
kernel: enforce usage of CONFIG_TEST_USERSPACE
8363eeb 
If a test tries to create a user thread, and the platform
suppors user mode, and CONFIG_TEST_USERSPACE has not been
enabled, fail an assertion.

Signed-off-by: Andrew Boie <[email protected]>
@andrewboie andrewboie force-pushed the userspace-testing-overhaul branch from 8d99ea6 to 8363eeb Compare April 6, 2019 00:06
@andrewboie andrewboie changed the title userspace: overhaul userspace testing and fix some network tests userspace: reduce CI userspace testing to tests that use it and fix some network tests Apr 6, 2019
jukkar
jukkar approved these changes Apr 6, 2019
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

LGTM

@nashif nashif merged commit 9f04c74 into zephyrproject-rtos:master Apr 6, 2019
@andrewboie andrewboie deleted the userspace-testing-overhaul branch April 10, 2019 14:54
@marc-hb marc-hb requested review from mped-oticon and removed request for ramakrishnapallala May 14, 2019 01:00
@marc-hb
Copy link
Collaborator

marc-hb commented May 14, 2019

Some tests have CONFIG_TEST_USERSPACE=y but no userspace sanitycheck tag.
For other tests it's the other way round.
Should they be in sync?

Because they're in unrelated locations (prj.conf versus sanitycheck's .yaml file) it's unrealistic to keep them in sync all the time. On the other hand it should be easy for sanitycheck to automagically set CONFIG_TEST_USERSPACE=y when the userspace tag is set. Would that be acceptable? "Reverse" demo:

   sanitycheck --extra-args=CONFIG_TEST_USERSPACE=n \
    -p qemu_x86 -T tests/kernel/mem_protect/stackprot/

Random "out of sync" examples found with git grep + a fair amount of elbow grease:

test_build_gpio
test_build_sensors_*
getaddrinfo/net.socket
tests/kernel/lifo/lifo_usage/kernel.lifo.usage
tests/kernel/mem_protect/mem_protect/kernel.memory_protection
...

@andrewboie
Copy link
Contributor Author

CONFIG_TEST_USERSPACE=y when the userspace tag is set. Would that be acceptable?

NACK

the test will not function if you run it outside of sanitycheck, do not split the the configuration like this

@pizi-nordic pizi-nordic mentioned this pull request May 17, 2019
Closed
@marc-hb
Copy link
Collaborator

marc-hb commented May 29, 2019

I found another, semi-automated way to check for inconsistencies between CONFIG_TEST_USERSPACE and "userspace". See PR #16466

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulfalizer ulfalizer ulfalizer approved these changes

@nashif nashif nashif approved these changes

@andyross andyross andyross approved these changes

@jukkar jukkar jukkar approved these changes

@ceolin ceolin Awaiting requested review from ceolin

@jhedberg jhedberg Awaiting requested review from jhedberg

@pfalcon pfalcon Awaiting requested review from pfalcon

@sjanc sjanc Awaiting requested review from sjanc

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@Vudentz Vudentz Awaiting requested review from Vudentz

@carlescufi carlescufi Awaiting requested review from carlescufi

@mped-oticon mped-oticon Awaiting requested review from mped-oticon

Assignees
No one assigned
Labels
bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Milestone
v1.14.0
6 participants
@andrewboie @marc-hb @ulfalizer @nashif @andyross @jukkar