Skip to content

Conversation

pussuw
Copy link
Contributor

@pussuw pussuw commented Jan 24, 2023

Summary

Implement task_create and task_delete for use with ostest. The functions are unavailable in kernel build mode but overload them here and create a pthread worker instead. Some tests should be re-visited because the intent is to user another task (in this case another process) to e.g. receive signals. That will require quite a bit of extra work.

Tests that had to be disabled:

  • waitpid: pthreads don't work with waitpid, but this can be fixed by starting a dummy process instead and using waitpid on that (NOT IMPLEMENTED)
  • restart: task_restart() does not work at all with kernel mode so it is disabled entirely
  • fpu: make sure the FPU test is not even attempted, because it will cause ASSERT() and stop the test
  • vfork: vfork() does not work for some reason in CONFIG_BUILD_KERNEL, there is something missing on the kernel side, so just disable the test for now

Tests that should be re-visited:

  • The signal tests, now they signal the process itself while before the signal was sent to another task. This will require building the part that receives the signal as a separate process
  • waitpid: Like stated above, waitpid does not work for pthreads
  • user_main: It might be a better idea to just call user_main() because the "test creating a task works"-part has already been tested; if ostest starts, then task starting and creation also works

Impact

Adds basic support for ostest with CONFIG_BUILD_KERNEL=y

Testing

icicle:knsh

Implement task_create and task_delete for use with ostest. The functions
are unavailable in kernel build mode but overload them here and create a
pthread worker instead. Some tests should be re-visited because the intent
is to user another task (in this case another process) to e.g. receive
signals. That will require quite a bit of extra work.

Tests that had to be disabled:
- waitpid: pthreads don't work with waitpid, but this can be fixed by
  starting a dummy process instead and using waitpid on that
  (NOT IMPLEMENTED)
- restart: task_restart() does not work at all with kernel mode so it is
  disabled entirely
- fpu: make sure the FPU test is not even attempted, because it will cause
  ASSERT() and stop the test
- vfork: vfork() does not work for some reason in CONFIG_BUILD_KERNEL,
  there is something missing on the kernel side, so just disable the test
  for now

Tests that should be re-visited:
- The signal tests, now they signal the process itself while before the
  signal was sent to another task. This will require building the part
  that receives the signal as a separate process
- waitpid: Like stated above, waitpid does not work for pthreads
- user_main: It might be a better idea to just call user_main() because
  the "test creating a task works"-part has already been tested; if ostest
  starts, then task starting and creation also works
@pussuw pussuw force-pushed the ostest_for_kernelmode branch from 2a5157e to a5f8061 Compare January 24, 2023 11:29
@xiaoxiang781216
Copy link
Contributor

Why not skip the test which involve task_xxx function instead simulating task by pthread?

@pussuw
Copy link
Contributor Author

pussuw commented Jan 24, 2023

Why not skip the test which involve task_xxx function instead simulating task by pthread?

Of course that is another way of doing it. I thought this would be better, many tests create "workers" via task_create and I thought it would be fine to use pthreads for that.

We currently do not have any kind of ostest for CONFIG_BUILD_KERNEL, I made this to have at least something. It is not a full implementation but IMO better than nothing.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 24, 2023

Why not skip the test which involve task_xxx function instead simulating task by pthread?

Of course that is another way of doing it. I thought this would be better, many tests create "workers" via task_create and I thought it would be fine to use pthreads for that.

If the case doesn't really depend on task_create, it's better to migrate to pthread or remove task_create calling directly.

We currently do not have any kind of ostest for CONFIG_BUILD_KERNEL, I made this to have at least something. It is not a full implementation but IMO better than nothing.

If the test invokes the function which is impossible to implement in kernel mode:

  1. Change to the similar function exist in kernel mode
  2. Skip the test by #ifdef CONFIG_BUILD_KERNEL if it indeed test the specific function

@pussuw
Copy link
Contributor Author

pussuw commented Jan 24, 2023

If the case doesn't really depend on task_create, it's better to migrate to pthread or remove task_create calling directly.

I would have to go them through one by one, because I don't know how each test is supposed to work. Instead, I made a kludge solution that allows the test to compile and run (most) tests.

@pussuw
Copy link
Contributor Author

pussuw commented Jan 24, 2023

If the test invokes the function which is impossible to implement in kernel mode:

1. Change to the similar function exist in kernel mode

The similar function is posix_spawn(), which requires:

  • The test case is split into another (or perhaps even multiple) process's, compiled into a file, which can be started with posix_spawn()
  • Potential communication between the two (or multiple) processes
    Achieving this kind of functionality is a lot of work which I'm not prepared to do at this point.
2. Skip the test by `#ifdef CONFIG_BUILD_KERNEL` if it indeed test the specific function

Skipping these kind of tests is fine by me, but I don't know which tests could be converted to use pthread and which must be skipped. I already disabled some of the tests that I know won't work in kernel mode.

@pussuw
Copy link
Contributor Author

pussuw commented Jan 25, 2023

Btw, I 100% agree with you that this incomplete implementation is not perfect and needs more work + clean up.

I have been using this version for some time now and have found it useful (better than nothing) and I decided to make this PR to at least offer the solution to the community.

However, if no extra value is seen in this partial implementation, I will just close the PR.

Maybe at some point I will make a proper implementation for this, but as I said, I don't have time to do that now.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 25, 2023

The patch is valuable. My suggestion is very simple that:

  1. Remove user_main, let's start the test from main directly

  2. Skip the test case which invoke the task_xxx function in kernel mode

    • Skip compiling from Make.defs
    • Skip inovation in ostest_main.c by #ifdef CONFIG_BUILD_KERNEL

Or provding a dummy function in the individual test source code

  1. Remove the task_xxx simulation layer

We can migrate task_create to posix_spawn or pthread_create as needed in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants