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

Fix schedcoop idling and x86 timer interrupts #923

Closed
wants to merge 2 commits into from

Conversation

FedeParola
Copy link
Contributor

@FedeParola FedeParola commented May 31, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): x86_64
  • Platform(s): kvm
  • Application(s): N/A

Additional configuration

Description of changes

880f27c fixes a problem in ukschedcoop in which the idle thread keeps busy looping instead of halting the cpu when no wakeup time is set (i.e., wake_up_time == 0). To test the problem simply apply app-helloworld-diff.txt to the helloworld application (the diff causes the main thread to block) and run it. You will see the CPU usage at 100%.

655ea17 fixes a problem in which the i8254 timer doesn't switch completely to software-triggered strobe in the init function. To switch the i8254 to sw-triggered strobe mode a value must be written into the counter. Previously the timer remained in rate generator mode and kept generating interrupts until the counter was written (e.g., for a sleep()). To test add some debug message in the timer interrupt ISR and run the previous modified helloworld, you will see a lot of interrupts are triggered. E.g., time-diff.txt.

@FedeParola FedeParola requested review from a team as code owners May 31, 2023 23:50
@unikraft-bot unikraft-bot added area/lib Internal Unikraft Microlibrary area/plat Unikraft Patform lang/c Issues or PRs to do with C/C++ lib/ukschedcoop plat/kvm Unikraft for KVM labels Jun 1, 2023
@skuenzer skuenzer assigned skuenzer and marcrittinghaus and unassigned nderjung Jun 1, 2023
@skuenzer skuenzer requested a review from mschlumpp June 1, 2023 11:19
@razvand razvand requested review from mogasergiu and removed request for a team and dragosargint June 1, 2023 11:33
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jun 1, 2023
@razvand
Copy link
Contributor

razvand commented Jun 1, 2023

Nice fixes, @FedeParola. I will use your test programs to validate the solution, but, just by browsing the code, it makes sense.

I would ask you to start the commit messages with capital letter in the two commits of this PR, i.e. Fix idling without wakeup time (capital F) and Switch i8254 to sw-triggered strobe on init (capital S).

@mschlumpp, @mogasergiu, @skuenzer, @marcrittinghaus, please also take a look.

A wrong check caused the idle thread to keep busy looping instead of
halting the cpu when no wakeup time is set (i.e., wake_up_time == 0)

Signed-off-by: Federico Parola <federico.parola@polito.it>
To switch the i8254 to sw-triggered strobe mode a value must be
written into the counter. Previously the timer remained in rate
generator mode and kept generating interrupts until the counter
was written (e.g., for a sleep())

Signed-off-by: Federico Parola <federico.parola@polito.it>
@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
880f27c lib/ukschedcoop: Fix idling without wakeup time
655ea17 plat/kvm/x86/tscclock: Switch i8254 to sw-triggered strobe on init

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Hi, @FedeParola. I tested and, indeed, the problems are as you mentioned and the solutions as you provided. I'm approving this PR.

We'll wait for an approval step from @skuenzer. At that point, I will also add the reviewer tag, conforming to Unikraft's review process.

I've one question related to the i8254 sw-triggered configuration. Why are two writes (outb) required to TIMER_CNTR? Indeed, I tried only using on write (outb) but it did not work.

@FedeParola
Copy link
Contributor Author

Hi @razvand and thanks for the review. The i8254 has a 16 bit counter, but the register to program it is only 8 bit, so two outb are needed to perform a complete write. It is also possible to configure the timer to only input the high or low 8 bits of the counter (remaining bits are automatically set to 0), but keeping the two steps configuration allows later configurations to retain full resolution.

Copy link
Member

@skuenzer skuenzer left a comment

Choose a reason for hiding this comment

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

Dear @FedeParola,

a well explained PR with good code examples. Thanks for these fixes!

Approved-by: Simon Kuenzer simon@unikraft.io

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Jun 2, 2023
To switch the i8254 to sw-triggered strobe mode a value must be
written into the counter. Previously the timer remained in rate
generator mode and kept generating interrupts until the counter
was written (e.g., for a sleep())

Signed-off-by: Federico Parola <federico.parola@polito.it>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Simon Kuenzer <simon@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #923
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Jun 2, 2023
razvand pushed a commit to unikraft-upb/unikraft that referenced this pull request Jun 4, 2023
A wrong check caused the idle thread to keep busy looping instead of
halting the cpu when no wakeup time is set (i.e., wake_up_time == 0)

Signed-off-by: Federico Parola <federico.parola@polito.it>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Simon Kuenzer <simon@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#923
razvand pushed a commit to unikraft-upb/unikraft that referenced this pull request Jun 4, 2023
To switch the i8254 to sw-triggered strobe mode a value must be
written into the counter. Previously the timer remained in rate
generator mode and kept generating interrupts until the counter
was written (e.g., for a sleep())

Signed-off-by: Federico Parola <federico.parola@polito.it>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Simon Kuenzer <simon@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#923
StefanJum pushed a commit to unikraft-upb/unikraft that referenced this pull request Jun 5, 2023
A wrong check caused the idle thread to keep busy looping instead of
halting the cpu when no wakeup time is set (i.e., wake_up_time == 0)

Signed-off-by: Federico Parola <federico.parola@polito.it>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Simon Kuenzer <simon@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#923
StefanJum pushed a commit to unikraft-upb/unikraft that referenced this pull request Jun 5, 2023
To switch the i8254 to sw-triggered strobe mode a value must be
written into the counter. Previously the timer remained in rate
generator mode and kept generating interrupts until the counter
was written (e.g., for a sleep())

Signed-off-by: Federico Parola <federico.parola@polito.it>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Simon Kuenzer <simon@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#923
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary area/plat Unikraft Patform ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ lib/ukschedcoop plat/kvm Unikraft for KVM
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

6 participants