Fix read flow tag from cqe #333

BasharRadya · 2025-04-08T14:35:48Z

User description

Description

Fix read flow tag from cqe

The flow_tag field in the CQE format is 24 bits, but it was handled
as if it were 32 bits. The additional 8 bits are reserved for
the rx_drop_counter.
Consequently, when there were RX drops, the flow_tag was incorrect
because the most significant 8 bits were not zero.

CQE format for flow_tag is in "Table 186 - 64B Completion Queue Entry Format Layout
"

Add CQ rx_drop_counter

The number of dropped packets because of no
RCV WQE since the last CQE

What

Subject: what this PR is doing in one line.

Why ?

Justification for the PR. If there is existing issue/bug please reference.

How ?

It is optional but for complex PRs please provide information about the design,
architecture, approach, etc.

Change type

What kind of change does this PR introduce?

Check list

Code follows the style de facto guidelines of this project
Comments have been inserted in hard to understand places
Documentation has been updated (if necessary)
Test has been added (if possible)

PR Type

Bug fix, Enhancement

Description

Corrected handling of flow_tag field in CQE to use 24 bits.
Added support for tracking RX drop counter in CQ statistics.
Updated logging and statistics printing to include RX drop counter.
Adjusted data structures and parsing logic to align with updated CQE format.

Changes walkthrough 📝

Relevant files

Enhancement

cq_mgr_rx.cpp `Add RX drop counter logging in statistics` src/core/dev/cq_mgr_rx.cpp Added logging for RX drop counter in statistics print function. Updated condition to include RX drop counter in statistics checks.	+5/-2
stats_reader.cpp `Add RX drop counter to statistics calculations` src/stats/stats_reader.cpp Added RX drop counter to delta statistics calculation. Updated statistics printing to include RX drop counter.	+5/-0
xlio_stats.h `Add RX drop counter to statistics structure` src/core/util/xlio_stats.h Added `n_rx_drop_counter` field to CQ statistics structure.	+1/-0

Bug fix

cq_mgr_rx_regrq.cpp `Fix flow_tag parsing and add RX drop counter` src/core/dev/cq_mgr_rx_regrq.cpp Corrected `flow_tag` parsing to use 24 bits. Added RX drop counter extraction from CQE.	+3/-1
cq_mgr_rx_strq.cpp `Fix flow_tag parsing and add RX drop counter` src/core/dev/cq_mgr_rx_strq.cpp Corrected `flow_tag` parsing to use 24 bits. Added RX drop counter extraction from CQE.	+3/-1

Miscellaneous

ib_mlx5.h `Rename CQE field for clarity` src/core/ib/mlx5/ib_mlx5.h Renamed `sop_drop_qpn` to `sop_rxdrop_qpn_flowtag` for clarity.	+1/-1

Need help?
Type /help how to ... in the comments thread for any questions about PR-Agent usage.
Check out the documentation for more information.

AlexanderGrissik · 2025-04-09T08:24:02Z

bot:retest

galnoam · 2025-04-09T13:22:26Z

/review

pr-review-bot-app · 2025-04-09T13:23:38Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Possible Misinterpretation of Bit Manipulation The bit manipulation logic for extracting `flow_tag_id` and `n_rx_drop_counter` from `sop_rxdrop_qpn_flowtag_h_byte` should be carefully reviewed to ensure correctness and alignment with the CQE format specification. uint32_t sop_rxdrop_qpn_flowtag_h_byte = ntohl((uint32_t)(cqe->sop_rxdrop_qpn_flowtag)); p_rx_wc_buf_desc->rx.flow_tag_id = sop_rxdrop_qpn_flowtag_h_byte & 0x00FFFFFF; m_p_cq_stat->n_rx_drop_counter += sop_rxdrop_qpn_flowtag_h_byte >> 24; Consistency in Bit Extraction Logic The bit extraction logic for `flow_tag_id` and `n_rx_drop_counter` in this file should be validated to ensure it matches the intended CQE format and does not introduce errors. uint32_t sop_rxdrop_qpn_flowtag_h_byte = ntohl((uint32_t)(cqe->sop_rxdrop_qpn_flowtag)); _hot_buffer_stride->rx.flow_tag_id = sop_rxdrop_qpn_flowtag_h_byte & 0x00FFFFFF; m_p_cq_stat->n_rx_drop_counter += sop_rxdrop_qpn_flowtag_h_byte >> 24;

tomerdbz · 2025-04-09T15:19:04Z

/suggest

tomerdbz · 2025-04-09T15:19:10Z

/describe

pr-review-bot-app · 2025-04-09T15:19:59Z

PR Description updated to latest commit (c562f6a)

tomerdbz · 2025-04-09T15:21:18Z

/improve

pr-review-bot-app · 2025-04-09T15:21:38Z

PR Code Suggestions ✨

Category	Suggestion	Impact
Possible issue	Prevent division by zero errors Add a check to ensure `delay` is not zero to prevent division by zero errors. src/stats/stats_reader.cpp [401-402] -p_prev_cq_stats->n_rx_drop_counter = - (p_curr_cq_stats->n_rx_drop_counter - p_prev_cq_stats->n_rx_drop_counter) / delay; +if (delay != 0) { + p_prev_cq_stats->n_rx_drop_counter = + (p_curr_cq_stats->n_rx_drop_counter - p_prev_cq_stats->n_rx_drop_counter) / delay; +} else { + // Handle zero delay case appropriately +} Suggestion importance[1-10]: 9 __ Why: This suggestion prevents a critical runtime error (division by zero) by adding a validation check for the `delay` variable. It is highly impactful as it ensures the code does not crash under certain conditions and handles edge cases appropriately.	High
	Add overflow check for counter updates Ensure that the addition to `m_p_cq_stat->n_rx_drop_counter` does not cause an integer overflow by validating the value before performing the addition. src/core/dev/cq_mgr_rx_regrq.cpp [120-122] uint32_t sop_rxdrop_qpn_flowtag_h_byte = ntohl((uint32_t)(cqe->sop_rxdrop_qpn_flowtag)); p_rx_wc_buf_desc->rx.flow_tag_id = sop_rxdrop_qpn_flowtag_h_byte & 0x00FFFFFF; -m_p_cq_stat->n_rx_drop_counter += sop_rxdrop_qpn_flowtag_h_byte >> 24; +uint32_t drop_counter_increment = sop_rxdrop_qpn_flowtag_h_byte >> 24; +if (UINT64_MAX - m_p_cq_stat->n_rx_drop_counter >= drop_counter_increment) { + m_p_cq_stat->n_rx_drop_counter += drop_counter_increment; +} else { + // Handle overflow case appropriately +} Suggestion importance[1-10]: 8 __ Why: The suggestion addresses a potential integer overflow issue when updating `m_p_cq_stat->n_rx_drop_counter`, which is critical for ensuring the correctness and stability of the code. The improved code provides a robust solution by adding a validation check.	Medium
	Prevent overflow in counter updates Validate the value being added to `m_p_cq_stat->n_rx_drop_counter` to prevent potential integer overflow issues. src/core/dev/cq_mgr_rx_strq.cpp [227-229] uint32_t sop_rxdrop_qpn_flowtag_h_byte = ntohl((uint32_t)(cqe->sop_rxdrop_qpn_flowtag)); _hot_buffer_stride->rx.flow_tag_id = sop_rxdrop_qpn_flowtag_h_byte & 0x00FFFFFF; -m_p_cq_stat->n_rx_drop_counter += sop_rxdrop_qpn_flowtag_h_byte >> 24; +uint32_t drop_counter_increment = sop_rxdrop_qpn_flowtag_h_byte >> 24; +if (UINT64_MAX - m_p_cq_stat->n_rx_drop_counter >= drop_counter_increment) { + m_p_cq_stat->n_rx_drop_counter += drop_counter_increment; +} else { + // Handle overflow case appropriately +} Suggestion importance[1-10]: 8 __ Why: Similar to the previous suggestion, this one mitigates the risk of integer overflow in `m_p_cq_stat->n_rx_drop_counter`. It is highly relevant and improves the reliability of the code by introducing a validation mechanism.	Medium

tomerdbz · 2025-04-09T15:22:26Z

/analyze

src/stats/stats_reader.cpp

src/core/dev/cq_mgr_rx_regrq.cpp

The flow_tag field in the CQE format is 24 bits, but it was handled as if it were 32 bits. The additional 8 bits are reserved for the rx_drop_counter. Consequently, when there were RX drops, the flow_tag was incorrect because the most significant 8 bits were not zero. Signed-off-by: Bashar Abdelgafer <[email protected]>

src/core/dev/net_device_table_mgr.cpp

src/core/dev/buffer_pool.cpp

BasharRadya · 2025-04-16T10:04:22Z

bot:retest

src/core/dev/cq_mgr_rx_regrq.cpp

src/core/dev/net_device_table_mgr.h

src/core/dev/net_device_val.cpp

src/stats/stats_reader.cpp

BasharRadya · 2025-04-20T09:15:07Z

bot:retest

AlexanderGrissik · 2025-04-21T09:10:10Z

bot:retest

BasharRadya · 2025-04-22T13:52:18Z

bot:retest

BasharRadya · 2025-04-28T09:55:29Z

bot:retest

BasharRadya · 2025-04-28T10:14:51Z

bot:retest

src/core/dev/buffer_pool.cpp

src/core/dev/cq_mgr_rx.cpp

pasis · 2025-04-28T12:15:21Z

src/core/dev/cq_mgr_rx.cpp

    }
 }
-
+uint64_t cq_mgr_rx::get_n_rx_drop_counter()


move this definition to the header

Fixed.
but why? because its 1 line?

src/core/main.cpp

The number of dropped packets because of no RCV WQE since the last CQE Signed-off-by: Bashar Abdelgafer <[email protected]>

Added an accumlated RX out of buffer drop counter to the print report The accumlated result includes: A)RX drops counts from open rings at termination phase. B)RX drops from rings that were closed during runtime before the termination phase (e.g. when all connections belonging to the ring were closed). Signed-off-by: Bashar Abdelgafer <[email protected]>

README

src/core/dev/net_device_table_mgr.h

src/core/util/xlio_stats.h

src/core/dev/allocator.cpp

In 'AUTO' mode, the report is printed only if an anomaly is detected (e.g., RX drops or buffer pool exhaustion). Set XLIO_PRINT_REPORT default to 'AUTO', and update README accordingly. Signed-off-by: Bashar Abdelgafer <[email protected]>

Renamed the print label from 'Packets dropped' to 'SW RX Packets dropped' to make it clear that the drop counter refers to software-level RX drops. Signed-off-by: Bashar Abdelgafer <[email protected]>

Rename n_rx_drop_counter and n_rx_pkt_drop. Signed-off-by: Bashar Abdelgafer <[email protected]>

BasharRadya · 2025-04-30T06:46:30Z

bot:retest

BasharRadya · 2025-04-30T07:41:07Z

bot:retest

pasis · 2025-04-30T10:25:42Z

bot:retest

BasharRadya · 2025-05-04T06:52:39Z

bot:retest

BasharRadya requested review from AlexanderGrissik and pasis April 8, 2025 14:35

pr-review-bot-app bot added the Review effort 3/5 label Apr 9, 2025

pasis reviewed Apr 14, 2025

View reviewed changes

src/stats/stats_reader.cpp Outdated Show resolved Hide resolved

src/core/dev/cq_mgr_rx_regrq.cpp Outdated Show resolved Hide resolved

src/core/dev/cq_mgr_rx_regrq.cpp Outdated Show resolved Hide resolved

galnoam added the draft Not to review yet label Apr 15, 2025

BasharRadya force-pushed the fix_read_flow_tag_from_cqe branch from a6f3d19 to f17c65b Compare April 15, 2025 09:07

pasis reviewed Apr 15, 2025

View reviewed changes

src/core/dev/net_device_table_mgr.cpp Outdated Show resolved Hide resolved

src/core/dev/buffer_pool.cpp Outdated Show resolved Hide resolved

BasharRadya force-pushed the fix_read_flow_tag_from_cqe branch from f17c65b to 08a78af Compare April 15, 2025 12:50

BasharRadya requested a review from pasis April 15, 2025 13:12

BasharRadya force-pushed the fix_read_flow_tag_from_cqe branch from 08a78af to 1431c3b Compare April 15, 2025 13:12