- 
                Notifications
    You must be signed in to change notification settings 
- Fork 354
Perftest: Add support for Send with Immediate verb #345
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
base: master
Are you sure you want to change the base?
Conversation
| Generally looks good to me but I think we should add a check in run_iter_bw* and run_iter_lat that completion really has imm data. Something like: Also can you please describe your changes in the commit message? | 
| 
 No problem, I will push a new version later | 
6f13593    to
    870ebda      
    Compare
  
    | @mrgolin I've updated the code and the commit message with your suggestion, could you please take a look at the new version? Thanks! | 
        
          
                src/perftest_resources.c
              
                Outdated
          
        
      | ctx_post_send_work_request_func_pointer(ctx, user_param); | ||
| #endif | ||
|  | ||
| if (user_param->verb == SEND_IMM || user_param->verb == WRITE_IMM) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be always true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed, thanks a lot!
        
          
                src/perftest_resources.c
              
                Outdated
          
        
      | ctx_post_send_work_request_func_pointer(ctx, user_param); | ||
| #endif | ||
|  | ||
| if (user_param->verb == SEND_IMM || user_param->verb == WRITE_IMM) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't get here with user_param->verb == WRITE_IMM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
91a7185    to
    2164eb2      
    Compare
  
    | Our team would like to leverage Perftest test suite to test send scenarios while using the immediate field. | 
Currently, perftest supports RDMA Write / Write-with-Immediate / Send / Read and Atomic operations, but lacks Send-with-Immediate support. This commit adds Send-with-Immediate functionality, allowing users to test it via ib_send_bw with --send_with_imm flag. Additionally, to verify the completion logic for operations with immediate data is correct, completion events are now validated to ensure the IBV_WC_WITH_IMM flag is set. Signed-off-by: Zelong Yue <[email protected]>
2164eb2    to
    760fa10      
    Compare
  
    | 
 Thanks! I've noticed that your PR was checking against imm data correctness, but mine is not, should I add this to my code? Do you have any suggestion? | 
| 
 Yes I noticed that too. It could be nice to have it, but since it is a hardcoded constant I'm not sure it gives so much added value in comparison to validating the imm flags (which you are doing in your PR). | 
| 
 OK, I'll skip implementing imm data checking in this PR. I think perftest is mainly aimed at providing the best performance measurements for every opcode, rather than checking protocol correctness. Maybe we could add an option for that in the future, so it won't affect performance by default. | 
Currently, perftest supports RDMA Write / Write-with-Immediate / Send /
Read and Atomic operations, but lacks Send-with-Immediate support. This
commit adds Send-with-Immediate functionality, allowing users to test it
via ib_send_bw with --send_with_imm flag.
Additionally, to verify the completion logic for operations with
immediate data is correct, completion events are now validated to ensure
the IBV_WC_WITH_IMM flag is set.