Optimize mutex usage to minimize hold time and prevent priority inversion#45
Conversation
…sion
This commit implements three key optimizations for the mutex protecting
image table access in Python plugins:
1. Priority Inheritance for Mutex (C core)
- Modified plugin_driver_create() to use pthread_mutexattr_setprotocol()
with PTHREAD_PRIO_INHERIT
- Prevents priority inversion when lower-priority plugin threads hold
the mutex while the real-time PLC scan cycle thread is waiting
2. Move Conversion Logic Outside Critical Section (modbus_master_memory.py)
- Added new optimized functions that separate data conversion from
buffer access:
* convert_modbus_data_to_iec_values() - pre-convert before mutex
* write_preconverted_iec_values() - write under mutex
* read_raw_iec_values() - read under mutex
* convert_raw_iec_to_modbus() - convert after mutex release
- Legacy functions kept for backward compatibility
3. Batch Write Preparations (modbus_master_plugin.py)
- READ operations: Pre-convert all Modbus data to IEC values before
acquiring mutex, then write all pre-converted values under single
mutex acquisition
- WRITE operations: Collect all due write points, read all raw IEC
values under single mutex acquisition, then convert and perform
Modbus writes outside the mutex
These optimizations reduce mutex hold time by ensuring only actual buffer
access operations occur within the critical section, while CPU-intensive
conversion work happens outside.
Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
Pull request overview
This PR optimizes mutex usage in the Modbus master plugin to reduce latency impact on the real-time PLC core by minimizing mutex hold time and preventing priority inversion. The key changes include:
- Implementing priority inheritance for the mutex in C to prevent lower-priority plugin threads from blocking the real-time PLC scan cycle
- Refactoring Python code to separate CPU-intensive data conversion from mutex-protected buffer access
- Batching operations to reduce the number of mutex acquisitions per cycle
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/src/drivers/plugin_driver.c | Added priority inheritance mutex initialization and code formatting improvements |
| core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py | Refactored READ/WRITE operations to pre-convert data outside mutex and batch buffer access |
| core/src/drivers/plugins/python/modbus_master/modbus_master_memory.py | Added optimized functions that separate conversion from buffer access, preserved legacy functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| iec_addr, modbus_data, length | ||
| ) | ||
| if converted_values is not None and details is not None: | ||
| preconverted_updates.append((converted_values, details)) |
There was a problem hiding this comment.
Failed conversions are silently skipped without logging. Consider adding error logging when convert_modbus_data_to_iec_values() returns None to aid debugging when data conversion fails.
| preconverted_updates.append((converted_values, details)) | |
| preconverted_updates.append((converted_values, details)) | |
| else: | |
| print( | |
| f"[{self.name}] (FAIL) Data conversion failed for IEC address {iec_addr}, " | |
| f"Modbus data: {modbus_data}, length: {length}" | |
| ) |
| print( | ||
| f"[{self.name}] (FAIL) Invalid offset " | ||
| f"'{point.offset}' for FC {point.fc}: {ve}" | ||
| ) |
There was a problem hiding this comment.
The loop continues after catching ValueError but doesn't break when stop event is set between iterations. The stop check at line 259 should be moved after line 272 to ensure the loop can exit promptly during shutdown even when processing exceptions.
| ) | |
| ) | |
| if self._stop_event.is_set(): | |
| break |
There was a problem hiding this comment.
The stop event is already checked at the top of each loop iteration. Since this loop is purely CPU-bound (scanning a Python list, doing integer operations, and calling parse_modbus_offset), there are no blocking calls or sleeps inside. If _stop_event becomes set while inside the loop, it will be checked at the top of the next iteration and exit promptly.
Adding an extra check after the exception handler would only shave off at most a single iteration of this fast loop, which doesn't provide meaningful improvement to shutdown responsiveness. It would also be inconsistent with the analogous read loop pattern earlier in the same method.
The existing check at the top of the loop is sufficient for this non-blocking, cheap operation.
Addresses Copilot review comment: Log when convert_modbus_data_to_iec_values() returns None to aid debugging. Uses a concise message with IEC address and length without dumping the full modbus_data list to avoid log noise. Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
Optimize mutex usage to minimize hold time and prevent priority inversion
Summary
This PR implements three optimizations for the mutex protecting image table access in Python plugins, addressing concerns about latency impact in the real-time PLC core:
Priority Inheritance for Mutex (C core): Modified
plugin_driver_create()to usePTHREAD_PRIO_INHERITprotocol, preventing priority inversion when lower-priority plugin threads hold the mutex while the real-time PLC scan cycle thread is waiting.Move Conversion Logic Outside Critical Section: Added new optimized functions in
modbus_master_memory.pythat separate CPU-intensive data conversion from buffer access:convert_modbus_data_to_iec_values()- pre-convert before mutexwrite_preconverted_iec_values()- write under mutexread_raw_iec_values()- read under mutexconvert_raw_iec_to_modbus()- convert after mutex releaseBatch Write Preparations: Updated
modbus_master_plugin.pyto:Updates since last revision
convert_modbus_data_to_iec_values()returns None, a concise error message is now logged with the IEC address and length.Review & Testing Checklist for Human
PTHREAD_PRIO_INHERITbehavior can vary across Linux kernels. Test on actual deployment hardware.Recommended Test Plan:
plc_timing_statslogs to verify scan cycle timing is not degradedNotes
modbus_master_memory.pyare preserved for backward compatibilityLink to Devin run: https://app.devin.ai/sessions/6d0c0c27083448438f6879d3f09c5266
Requested by: Thiago Alves (@thiagoralves)