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

Support notify hook for thread resume and suspend #426

Open
greglink opened this issue Dec 10, 2024 · 6 comments
Open

Support notify hook for thread resume and suspend #426

greglink opened this issue Dec 10, 2024 · 6 comments
Labels
feature New feature or enhancement request

Comments

@greglink
Copy link

greglink commented Dec 10, 2024

Problem Description
I'd like to monitor/observe (at runtime) the latency from a thread becoming ready for any reason (timer, semaphore, etc) until it begins execution, and then voluntarily suspends itself. These metrics are useful for ensuring a responsive real time operating system, and can be emitted as telemetry in formats like a histogram or high water mark to indicate potential delays or timing issues.

Describe the solution you'd like
I'd like to add/install optional callback/notify hooks in a similar fashion to the tx_thread_entry_exit_notify_ hooks, only this time for suspend and resume.

Describe alternatives you've considered
I could theoretically instrument all potential resume initiators (e.g. queues, semaphores, ...) but it seems like instrumenting at tx_thread_resume or tx_thread_system_resume might be the best/simplest location. I also considered emitting trace information, and scanning the trace information at runtime, but this seemed very heavy, as I couldn't restrict trace emission to only those events.

Additional context
I'm volunteering to write the PR, if someone can re-assure me that this won't be a rejected request for performance reasons, and inform me of whether _tx_thread_resume, _tx_thread_system_resume, or some other location is the best place to install the hook.

@greglink greglink added the feature New feature or enhancement request label Dec 10, 2024
@fdesbiens
Copy link

Hi @greglink.

Thank you for opening this issue. I appreciate your offer to implement this. Let's see what the project team thinks of your idea.

@eclipse-threadx/iot-threadx-committers Please review the proposal and provide your feedback.

@billlamiework
Copy link

Most developers would not find this helpful, so anything added must be under conditional compilation. This is also a critical real-time code path, so we must be extra cautious. That said, perhaps extension macros (defaulted to white space but able to be overridden by the developer) might be safer than function pointers and result in less overhead. You could define these as function calls or directly toggle pins from inside these routines. Interested to hear your thoughts....

@greglink
Copy link
Author

I agree with your assessment - I had planned to include it behind conditional compilation to match the style of other such hooks in the code, but I'm also open to extension macros. The only downside to extension macros (which I agree would be faster - no need to check for a non-NULL function pointer, nor need to even store it in the thread block) is that it doesn't seem to match the way other similar notify hooks are handled.

The entry_exit hook is evaluated (and optionally called) every time a thread enters and leaves the CPU, which can occur multiple times if said thread is pre-empted repeatedly. My belief is that a suspend/resume hook would likely be called much less frequently, and thus, while it would be in the critical execution path, it should also be lower impact than the entry_exit hook already in the code, and therefore, not 'overly much'.

@billlamiework
Copy link

I agree Greg that the entry/exit function is more overhead-wise since it happens at every context switch. However, the historical reason for the function pointer in this case was because not all developers had access to the source code for rebuilding/overriding. That is moot now given the source is in the public domain. Hence, my recommendation for a couple of extension macros....

@greglink
Copy link
Author

That makes good sense. It'll take me a bit to get a first draft PR up, then (I actually have to do the work), but I'm happy to use that technique, and expect and appreciate any feedback on improving it, as I won't "just" be mimicking the existing callback techniques.

@billlamiework
Copy link

I suggest a couple macros in tx_api.h defined to whitespace:

#ifndef TX_THREAD_SYSTEM_RESUME_EXTENSION
#define TX_THREAD_SYSTEM_RESUME_EXTENSION
#endif

#ifndef TX_THREAD_SYSTEM_SUSPEND_EXTENSION
#define TX_THREAD_SYSTEM_SUSPEND_EXTENSION
#endif

And then simply drop in TX_THREAD_SYSTEM_RESUME_EXTENSION and TX_THREAD_SYSTEM_SUSPEND_EXTENSION in the actual C code where you want to add the instrumentation. For resumption, I recommend sometime after the thread is completely on the ready list and for suspension sometime after the thread is completely removed from the ready list.

I hope that helps!

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

No branches or pull requests

3 participants