-
Notifications
You must be signed in to change notification settings - Fork 29
issue: 4213201 Nullify all WQEBB on special WQEs #323
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
Conversation
XLIO does a prefetch with zeroing the next WQEBB after a doorbell. It also presets inline_hdr_sz as a legacy behavior. However, in a corner case when the producer fills the tail WQE and the consumer is still processing the head WQE, the prefetch code corrupts the head WQE. Remove the prefetch to avoid the SQ corruption. Clear each WQE before filling it. Signed-off-by: Dmytro Podgornyi <[email protected]>
|
bot:retest |
|
@pasis, can review when you have free time. |
src/core/dev/hw_queue_tx.cpp
Outdated
| auto *cseg = wqebb_get<xlio_mlx5_wqe_ctrl_seg *>(0U); | ||
| auto *ucseg = wqebb_get<xlio_mlx5_wqe_umr_ctrl_seg *>(0U, sizeof(*cseg)); | ||
|
|
||
| memset(cseg, 0, sizeof(mlx5_eth_wqe)); |
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 is extra memset, since nvme_fill_static_params_control() clears all 64 bytes inside. Hiding memset inside the function seems confusing, so I'd prefer to either remove the extra memset or improve the solution consistently.
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.
Done
src/core/dev/hw_queue_tx.cpp
Outdated
| void hw_queue_tx::nvme_set_progress_context(xlio_tis *tis, uint32_t tcp_seqno) | ||
| { | ||
| auto *wqe = reinterpret_cast<mlx5e_set_nvmeotcp_progress_params_wqe *>(m_sq_wqe_hot); | ||
| memset(wqe, 0, sizeof(mlx5_eth_wqe)); |
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.
similarly to the nvme_set_static_context() comment, nvme_fill_progress_wqe() clears the wqe inside. However, in this case, the wqe is 32 bytes. I'd suggest to:
- remove the memset inside the nested function
- replace sizeof(mlx5_eth_wqe) with WQEBB constant. Ethernet wqe can be confusing in case of non-send operations. For more readability, this can even be
constexprmax<size_t>(WQEBB, sizeof(struct_name))- it is expected to be optimized in compile time.
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.
Done
src/core/dev/hw_queue_tx.cpp
Outdated
| is_tx ? MLX5_OPC_MOD_TLS_TIS_PROGRESS_PARAMS : MLX5_OPC_MOD_TLS_TIR_PROGRESS_PARAMS; | ||
|
|
||
| memset(wqe, 0, sizeof(*wqe)); | ||
| memset(wqe, 0, sizeof(mlx5_eth_wqe)); |
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.
Replace sizeof(mlx5_eth_wqe) with either WQEBB or max<size_t>(WQEBB, sizeof(mlx5_set_tls_progress_params_wqe)). See comment above. Repeat the same for all similar places.
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.
Done
Nullify all WQEBB size even if the special WQE is less then 64B. Signed-off-by: Alexander Grissik <[email protected]>
6520521 to
eb794d1
Compare
|
bot:retest |
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Description
Based on PR 321.
Correct special WQEs cleanup.
What
Fix possible issues and syndroms do to incorrect WQEBB cleanup.
Why ?
Functionality.
How ?
Always nullify the whole first WQEBB
Change type
What kind of change does this PR introduce?
Check list