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 thread insertion in ready list when blocked by higher priority thread #1000

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

singds
Copy link

@singds singds commented Sep 23, 2020

Hi,
I encountered the following issue using rtx.
There is a situation in which i ended up with one thread not sharing time with threads with same priority, in round-robin scheduling.
This happens when the thread in question is frequently interrupted by a higher priority thread.
In this interrupt the thread is blocked and reinserted in the ready list before other processes with same priority.
In the end, if a thread is always interrupted before it ends its timeslice, it never gives other threads with same priority the chance to run.
I think this can easily be fixed inserting the blocked thread in the ready list after al threads with same priority already in.

@JonatanAntoni
Copy link
Member

Hi @singds,

this is a known limitation already discussed in #228.
While your patch resolves your immediate problem it has certain other drawbacks. We need to clearly think if the drawbacks caused by this minimal change might be acceptable.

Given the current limitation we recommend not mixing round robin and priority based scheduling.

Cheers,
Jonatan

@singds
Copy link
Author

singds commented Sep 24, 2020

Thank you for your rapid feedback,
Sorry i didn't notice the older issue.
I think the "fast" solution of putting blocked thread in the back of the ready list can be ok.
Maybe there are cases in which they don't equally share time but at least they share. This compromise is even easy to document.
The solution of a per thread counter is even better. I can try to implement this and update my pull request.
I think it's worth resolve the issue and i'm really happy if i can help.

About not mixing round robin with priority may be the case to write some line in the official documentation, because it shows actually an example of priority mixed with round-robin.
https://www.keil.com/pack/doc/CMSIS/RTOS2/html/theory_of_operation.html

@JonatanAntoni
Copy link
Member

Good catch.

Changing the implementation, even if its such a small change, might affect existing applications. Basically, I think you change has a chance to work and deliver some sort of advantage. But as we might inappropriately affect existing application I tend to be very conservative.

Cheers,
Jonatan

@singds
Copy link
Author

singds commented Sep 24, 2020

I totally agree with you. The change is too delicate to be silently included.
Maybe we can add a config (default disabled) that enables it. This config could also be hidden from the Configuration Wizard in RTX_Config.h if you don't like it to be so evident. What do you think ?
I wrote another solution with per thread robin counter. May be a little bit more fair although not perfect. I plan to test and submit it tomorrow, so you can give me your feedback.

@JonatanAntoni
Copy link
Member

Yes, probably we need to have some sort of config options here. We are due to enhance the scheduler, anyhow. This will take its time but we are happy to incorporate your proposals.

@singds
Copy link
Author

singds commented Sep 25, 2020

Hi,
I'm not happy with my solution with per thread counter.
There are still borderline cases i can not remove without modifying too much the code.
Moreover i think there is no need to overcomplicate, the "fast" way it's enough and do the job.
I'll open a case in keil to see if it helps speed the adoption of any fix.

Thanks

@JonatanAntoni
Copy link
Member

Can one of the admins verify this patch?

RobertRostohar added a commit that referenced this pull request May 17, 2021
Round-Robin timeout value is not reset any more when switching to higher priority threads.
@RobertRostohar
Copy link
Collaborator

Should be properly addressed with the patch ada4cde. Pull request #1000 should not be needed.

JiafeiPan pushed a commit to nxp-mcuxpresso/CMSIS_5 that referenced this pull request Aug 8, 2023
Round-Robin timeout value is not reset any more when switching to higher priority threads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants