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

ISR versions of mutex_try_enter and mutex_exit #1549

Closed
wants to merge 1 commit into from

Conversation

daveythacher
Copy link
Contributor

#1453 Extends mutex API to support holding the spin lock. This can be used to prevent deadlock from competing core.

@daveythacher
Copy link
Contributor Author

Note this does cause possible priority inversion, if used incorrectly.

@daveythacher
Copy link
Contributor Author

daveythacher commented Dec 3, 2023

Does disabling interrupts on core 1, disable them on core 0? This patch does not cover the case of core 0 ISR and core 1 ISR racing against each other. In which case the patch is not worth much. (Even though this is horrible practice.) This would be new behavior and is capable of creating an issue like 1453. Perhaps documentation could describe the differences between the ISR and non ISR version and which is correct for certain contexts.

Note the ISR version is capable of creating a slight blocking behavior in non ISR version, which may need to be documented. I do not believe this diminishes this request, however does ask a larger question about the SDK.

@daveythacher
Copy link
Contributor Author

See #1580

@daveythacher
Copy link
Contributor Author

daveythacher commented Jul 12, 2024

Note a queue makes more sense.

Edit:
I planned two things but only did one. This was originally done to prevent a race condition. The other change was to allow a try API, which I never added. This version would not use spin_lock_blocking. Overall this API change is questionable.

So I changed the mechanics and forgot to look at the request summary. Overall the symbol names need to be updated in the request. However I knew this would never make it thru. As it does not make sense on SMP. The second change is SMT feature, while the first is a correction for SMP RTOS.

Overall recommendation is ISRs using CPU affinity with short ISRs to notify tasks. Using critical sections within mutex is a finer point that was lost. My displeasure with SDK maintainers is becoming more apparent.

Edit 2:
What this issue actually does is takes ownership of the mutex. (It does not release the spin lock.)

@kilograham
Copy link
Contributor

if you want a mutex which holds the spin lock, use critical_section

@kilograham kilograham closed this Jul 12, 2024
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.

2 participants