Skip to content
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.

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

Test SCHED0021 has a race condition #42

Closed
axel-h opened this issue Jul 17, 2021 · 0 comments · Fixed by #105
Closed

Test SCHED0021 has a race condition #42

axel-h opened this issue Jul 17, 2021 · 0 comments · Fixed by #105
Labels

Comments

@axel-h
Copy link
Member

axel-h commented Jul 17, 2021

    /* Configure all of the threads */
    for (size_t thread = 0; thread < PREEMPTION_THREADS; thread += 1) {
[...]
        /* Thread must run at same prio as monitor */
        set_helper_priority(env, &threads[thread], OUR_PRIO);
[...]
    }
[...]
    /* Start executing other threads */
    ZF_LOGD("Releasing Threads");
    test_simple_preempt_start = 1;
    /* Yield should cause all other threads to execute before returning
     * to the current thread. */
    seL4_Yield();

Isn't there a tiny chance that the preemptive scheduler will kick in after test_simple_preempt_start = 1? So all threads get their time slice to run. Then seL4_Yield() is executed and each thread again gets a time slice to run?
Wouldn't is be better to do this:

    /* Start executing other threads */
    ZF_LOGD("Releasing Threads");
    /* Release our time slice now to get a new one. That should ensure we are
     * not interrupted due to bad luck by the preemptive scheduler right after 
     * setting test_simple_preempt_start to 1 and then voluntarily yielding. For
     * the same reason we are also printing the doing the ZF_LOGD() above 
     * already.
     */
    seL4_Yield();
    /* Set a timeout for the test. Each thread should run for one tick. */
    uint64_t start = time_now(env);
    test_simple_preempt_start = 1;
    /* Yield should cause all other threads to execute before returning
     * to the current thread. 
     */
    seL4_Yield();
    test_simple_preempt_start = 0;

There is still a risk because we can't be really sure what time_now(env) does and how long this may take.

Aside from this, the line

    uint64_t now = start;

is the original code seems quite useless to me, as nobody is using the value assigned to now, it is overwritten a few lines later. This might be a left over from a time where another debug message was printed with it?

@axel-h axel-h added the bug label Jul 17, 2021
lsf37 added a commit that referenced this issue Aug 21, 2021
These are currently BREAKPOINT_002 and SCHED0021. The corresponding
issues #43 and #42 remain open, and when resolved positively, these
tests should be enabled again for simulation runs.

Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
lsf37 added a commit that referenced this issue Aug 21, 2021
These are currently BREAKPOINT_002 and SCHED0021. The corresponding
issues #43 and #42 remain open, and when resolved positively, these
tests should be enabled again for simulation runs.

Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
lsf37 added a commit that referenced this issue Aug 21, 2021
These are currently BREAKPOINT_002 and SCHED0021. The corresponding
issues #43 and #42 remain open, and when resolved positively, these
tests should be enabled again for simulation runs.

Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
lsf37 added a commit that referenced this issue Aug 23, 2021
These are currently BREAKPOINT_002 and SCHED0021. The corresponding
issues #43 and #42 remain open, and when resolved positively, these
tests should be enabled again for simulation runs.

Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
lsf37 added a commit that referenced this issue Aug 23, 2021
These are currently BREAKPOINT_002 and SCHED0021. The corresponding
issues #43 and #42 remain open, and when resolved positively, these
tests should be enabled again for simulation runs.

Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
lsf37 added a commit that referenced this issue Oct 27, 2023
Decrease the chance that the monitor thread is preempted just when it
has released the preemption threads, but before it has yielded itself.
(If that happens, the preemption threads get two time slices.)

See also #42

Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
@lsf37 lsf37 linked a pull request Oct 27, 2023 that will close this issue
Indanz pushed a commit that referenced this issue Oct 27, 2023
Decrease the chance that the monitor thread is preempted just when it
has released the preemption threads, but before it has yielded itself.
(If that happens, the preemption threads get two time slices.)

See also #42

Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant