Updated to memmove support #18
Conversation
…ns with overlapping buffers are performed entirely on CPU or on DSA Modified support for memmove to disregard memmove operations with overlapping buffers because they are not split between CPU and DSA Modified dsa_execute function to not call dsa_wait_and_adjust since it is called when the operation wasn't split and the autotune algorithm should not be used. Signed-off-by: Sydir, Jerry <jerry.sydir@intel.com>
There was a problem hiding this comment.
Pull Request Overview
This PR updates memmove support to allow users to select whether overlapping buffer operations run on the CPU or on DSA. Key changes include the introduction of a new enum and environment variable for overlapping memmove actions; adjustments in statistics collection macros and calls to include the overlapping flag; and corresponding README updates to document the new behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| dto.c | Adds new enum and configuration for overlapping memmove actions and updates functions to handle the new logic and statistics. |
| README.md | Updates documentation to include the new DTO_OVERLAPPING_MEMMOVE_ACTION variable. |
Comments suppressed due to low confidence (2)
dto.c:1255
- Consider validating that 'dto_overlapping_memmove_action' falls within the defined enum range (0 to OVERLAPPING_LAST_ENTRY - 1) after parsing the environment variable, and fall back to OVERLAPPING_CPU if the value is out of bounds.
dto_overlapping_memmove_action = strtoul(env_str, NULL, 10);
README.md:59
- [nitpick] Consider reformatting the description for DTO_OVERLAPPING_MEMMOVE_ACTION to improve readability, for example by clearly separating the default value and possible options with punctuation or bullet points.
DTO_OVERLAPPING_MEMMOVE_ACTION=0/1 0 (default) DTO submits memmove operations with overlapping buffers entirely to CPU, 1 - entirely to DSA
dto.c
Outdated
| uint64_t elapsed_ns, int group, int error_code) | ||
| { | ||
| // dto_memcpymove didn't actually submit the request to DSA, so there is nothing to log. This will be captured by a second call | ||
| if(op==MEMMOVE && overlapping && dto_overlapping_memmove_action==OVERLAPPING_CPU && group==DSA_CALL_SUCCESS) |
There was a problem hiding this comment.
minor: the spacing in the if statement is a bit smushed, was it necessary?
dto.c
Outdated
| { | ||
| struct dto_wq *wq = get_wq(dest); | ||
| size_t cpu_size, dsa_size; | ||
| uint8_t is_overlapping = 0; |
There was a problem hiding this comment.
minor: I think this should be a bool to convey the is_overlapping is either true or false
There was a problem hiding this comment.
agree. I've changed it
dto.c
Outdated
| cpu_size = 0; | ||
| else | ||
| is_overlapping = 1; |
There was a problem hiding this comment.
can we have a check here that says
if (dto_overlapping_memmove_action == OVERLAPPING_CPU) { return true; }
then we don't need the if statement in line 1530
There was a problem hiding this comment.
better yet - we can move this check before we even call get_wq() to avoid atomic instruction if we are just going to default to CPU
There was a problem hiding this comment.
Agree. I've rearranged the code mostly as you suggested. We first check if its an overlapping move to be performed on CPU and return, otherwise we get the WQ, fill in the descriptor, etc.
dto.c
Outdated
| cpu_size = n * cpu_size_fraction / 100; | ||
| } | ||
|
|
||
| dsa_size = n - cpu_size; | ||
|
|
||
| thr_bytes_completed = 0; |
There was a problem hiding this comment.
we can set the_bytes_completed = 0 up with the completion_addr, and then we don't have to worry about setting it to zero later.
There was a problem hiding this comment.
I've modified it to set thr_bytes_completed=0 once at the top of the function.
…he case where the overlapping memmove is performed entirely on CPU Signed-off-by: Sydir, Jerry <jerry.sydir@intel.com>
Modified support for memmove to allow user to select whether operations with overlapping buffers are performed entirely on CPU or on DSA
Modified support for memmove to disregard memmove operations with overlapping buffers because they are not split between CPU and DSA Modified dsa_execute function to not call dsa_wait_and_adjust since it is called when the operation wasn't split and the autotune algorithm should not be used.