(sysman): Enabled multi process power get/set test #264
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR enables multi-process testing for power get/set operations in the sysman power test suite. The implementation allows testing power limit settings across separate processes using shared memory for inter-process communication.
- Added a helper process executable that validates power limits set by the parent process
- Implemented shared memory communication using Boost interprocess library to pass power limit data between processes
- Created a comprehensive test that sets power limits in the main process, launches a child process to verify those limits, and restores original settings
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test_sysman_power_process_helper.cpp | New helper executable that reads shared memory and validates power limits set by parent process |
| test_sysman_power.cpp | Adds multi-process test with shared memory setup, child process launch, and power limit validation |
| CMakeLists.txt | Configures build for the new helper process executable |
| } | ||
| } | ||
| } | ||
| struct powerInfo { |
There was a problem hiding this comment.
The 'powerInfo' struct is duplicated between files. Consider defining it in a shared header file to avoid code duplication and ensure consistency.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This seems valid comment. @Sarbojit2019 Could you please address this?
| zes_uuid_t uuid = lzt::get_sysman_device_uuid(devices[d]); | ||
|
|
||
| bi::shared_memory_object::remove("MultiProcPowerLimitSharedMemory"); | ||
| bi::shared_memory_object power_limit_shm(bi::create_only, "MultiProcPowerLimitSharedMemory", bi::read_write); |
There was a problem hiding this comment.
The shared memory object name 'MultiProcPowerLimitSharedMemory' is hardcoded in multiple places. Consider defining it as a constant to improve maintainability.
| bi::shared_memory_object power_limit_shm(bi::create_only, "MultiProcPowerLimitSharedMemory", bi::read_write); | |
| bi::shared_memory_object::remove(MULTI_PROC_POWER_LIMIT_SHARED_MEMORY_NAME); | |
| bi::shared_memory_object power_limit_shm(bi::create_only, MULTI_PROC_POWER_LIMIT_SHARED_MEMORY_NAME, bi::read_write); |
Copilot uses AI. Check for mistakes.
|
|
||
| int main(int argc, char **argv) { | ||
| if (argc != 2) { | ||
| LOG_INFO << "Insufficient argument count " <<argc; |
There was a problem hiding this comment.
Missing space before '<<argc' in the log message. Should be 'LOG_INFO << "Insufficient argument count " << argc;'
| LOG_INFO << "Insufficient argument count " <<argc; | |
| LOG_INFO << "Insufficient argument count " << argc; |
Copilot uses AI. Check for mistakes.
| power_limit_shm.truncate(ZES_MAX_UUID_SIZE+pd[d].power_info_list.size()*sizeof(powerInfo)); | ||
| bi::mapped_region mapped_region(power_limit_shm, bi::read_write); | ||
| std::memcpy(mapped_region.get_address(), uuid.id, ZES_MAX_UUID_SIZE); | ||
| std::memcpy(((char*)mapped_region.get_address())+ZES_MAX_UUID_SIZE, pd[d].power_info_list.data(), pd[d].power_info_list.size()*sizeof(powerInfo)); |
There was a problem hiding this comment.
[nitpick] The pointer arithmetic and casting make this line complex and hard to read. Consider using a more explicit approach with offset variables or helper functions.
| std::memcpy(((char*)mapped_region.get_address())+ZES_MAX_UUID_SIZE, pd[d].power_info_list.data(), pd[d].power_info_list.size()*sizeof(powerInfo)); | |
| size_t power_info_offset = ZES_MAX_UUID_SIZE; | |
| std::memcpy(static_cast<char*>(mapped_region.get_address()) + power_info_offset, | |
| pd[d].power_info_list.data(), | |
| pd[d].power_info_list.size() * sizeof(powerInfo)); |
Copilot uses AI. Check for mistakes.
| }; | ||
| LZT_TEST_F( | ||
| POWER_TEST, | ||
| MultiProcessTestSetValidPowerLimitInParentProcessAndReadInChildProcess) { |
There was a problem hiding this comment.
Could you please follow a Given..When..Then.. way of naming a test case?
| zes_power_limit_ext_desc_t power_peak_set = {}; | ||
| for (auto &power_limits_descriptor : power_limits_descriptors) { | ||
| power_limits_descriptors_initial[p].push_back(power_limits_descriptor); | ||
| if (power_limits_descriptor.level == ZES_POWER_LEVEL_PEAK) { |
There was a problem hiding this comment.
Any reason you're implementing this test only for PEAK power limit?
I believe this test should be implemented for all the available power limits.
| //step 1: i) Preserve initial power limit descriptors for restoration later | ||
| // ii) Set the power limit and verify the setting | ||
| std::vector<std::vector<zes_power_limit_ext_desc_t>> power_limits_descriptors_initial(p_power_handles.size()); | ||
| for (size_t p = 0; p < p_power_handles.size(); ++p) { |
There was a problem hiding this comment.
Unless you're modifying the elements of the vector power_handles, I think simple for loop declaration such as follow would be more concise just to access the elements.
for (auto &power_handle: power_handles)
| } | ||
| } | ||
| } | ||
| struct powerInfo { |
There was a problem hiding this comment.
This seems valid comment. @Sarbojit2019 Could you please address this?
JIRA: [VLCLJ-2456]