issue: 4183221 propagate socket errors #276

tomerdbz · 2024-12-10T12:22:01Z

In case we got an ECQE - set the socket in a closing state. Effectively this sends a TCP-RST.

Description

Currently, in case we got an ECQE - we don't notify our callers we got to an invalid state.
The callers will keep trying to transmit TX segments, unaware of any issues.
This causes callers to not be able to identify and overcome error states, causing issues like 4183221.

What

Propagates the pcb to sq_wqe_prop, so when getting an ECQE - we could notify the TCP/IP stack.

Why ?

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

How ?

Tested sockperf gets correct error + errno
Tested xlio_socket_api.c test gets correct error

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)

tomerdbz · 2024-12-10T12:26:41Z

Tested with sockperf

src/core/dev/cq_mgr_tx.cpp

AlexanderGrissik · 2024-12-12T08:10:37Z

BTW, maybe instead of propagating errors to socket and handling socket deletion cases, we can mark some bit on the mem_buf_desc/pbuf, and on the next rexmit attempt lwip/socket will check the bit and fail it.

tomerdbz · 2024-12-12T12:55:35Z

ZC API tester yielded:
XLIO WARNING: cq_mgr_tx[0x5602d17e5920]:256:log_cqe_error() cqe: syndrome=0x4 vendor=0x52 hw=0x84 (type=0x0) wqe_opcode_qpn=0xa0014c2 wqe_counter=0x4
XLIO WARNING: cq_mgr_tx[0x5602d17e5920]:226:poll_and_process_element_tx() triggering low-level interface error on sockinfo=0x5602d17ecac0
Event callback: event=4 value=104 (sock=deadc0de).

tomerdbz · 2024-12-17T08:56:59Z

TODO - squash

tomerdbz · 2024-12-17T11:56:41Z

Following the split concern @AlexanderGrissik - validated tcp_seg_move_flags duplicates flags before it is set by ip_output.
Flow:

ECQE found - mem_buf_desc gets signaled
split happens on lwip segment REXMIT
ip_output called for 1st segment
mem_buf_desc noticed - RST gets initiated
ip_output called for 2nd segment
no anomaly in mem_buf_desc
gets sent
another ECQE generated

tomerdbz · 2024-12-17T12:18:38Z

bot:retest

AlexanderGrissik · 2024-12-25T15:46:12Z

Do we want this to vNext?

src/core/dev/cq_mgr_tx.cpp

AlexanderGrissik · 2024-12-17T07:17:25Z

src/core/proto/mem_buf_desc.h

+        CLONED = 0x01,
+        ZCOPY = 0x02,
+        INVALID = 0x04,
+    };


Please do another scan and check we do not use other values than these for flags.

AlexanderGrissik · 2024-12-17T08:13:25Z

src/core/sock/sockinfo_tcp.cpp

@@ -1372,6 +1372,17 @@ err_t sockinfo_tcp::ip_output(struct pbuf *p, struct tcp_seg *seg, void *v_p_con
    int count = 0;
    void *cur_end;

+    if (unlikely(is_set(attr.flags, XLIO_TX_PACKET_REXMIT))) {


Lets move this method to a separate inline method. Lets make sure that possible splits will not break it.

AlexanderGrissik · 2024-12-17T12:58:59Z

src/core/sock/sockinfo_tcp.cpp

@@ -1372,6 +1387,10 @@ err_t sockinfo_tcp::ip_output(struct pbuf *p, struct tcp_seg *seg, void *v_p_con
    int count = 0;
    void *cur_end;

+    if (unlikely(is_socket_in_error_state(p, (struct tcp_pcb *)v_p_conn, flags))) {


maybe is_buffer_in_error_state

AlexanderGrissik · 2024-12-17T13:00:32Z

src/core/sock/sockinfo_tcp.cpp

@@ -1347,6 +1347,21 @@ ssize_t sockinfo_tcp::tcp_tx_slow_path(xlio_tx_call_attr_t &tx_arg)
    return tcp_tx_handle_done_and_unlock(total_tx, errno_tmp, is_dummy, is_send_zerocopy);
 }

+static bool is_socket_in_error_state(const struct pbuf *p, struct tcp_pcb *pcb, uint16_t flags)
+{
+    if (unlikely(flags & XLIO_TX_PACKET_REXMIT)) {


i would move this check to sockinfo_tcp::ip_output because now we have 2 ifs

AlexanderGrissik · 2024-12-25T15:35:40Z

src/core/proto/mem_buf_desc.h

+        TYPICAL = 0,
+        CLONED = 0x01,
+        ZCOPY = 0x02,
+        INVALID = 0x04,


I would rename it to something more specific than just invalid. Maybe: HAD_CQE_ERROR

AlexanderGrissik · 2024-12-25T15:36:43Z

src/core/sock/sockinfo_tcp.cpp

@@ -1372,6 +1384,13 @@ err_t sockinfo_tcp::ip_output(struct pbuf *p, struct tcp_seg *seg, void *v_p_con
    int count = 0;
    void *cur_end;

+    if (unlikely(flags & XLIO_TX_PACKET_REXMIT)) {
+        const mem_buf_desc_t *mem_buf_desc = (const mem_buf_desc_t *)p;


Please consider cpp style cast

AlexanderGrissik · 2024-12-25T15:36:57Z

src/core/sock/sockinfo_tcp.cpp

@@ -1372,6 +1384,13 @@ err_t sockinfo_tcp::ip_output(struct pbuf *p, struct tcp_seg *seg, void *v_p_con
    int count = 0;
    void *cur_end;

+    if (unlikely(flags & XLIO_TX_PACKET_REXMIT)) {
+        const mem_buf_desc_t *mem_buf_desc = (const mem_buf_desc_t *)p;
+        if (unlikely(is_socket_in_error_state(mem_buf_desc, (struct tcp_pcb *)v_p_conn))) {


What do you think about making this method a member and doing:
p_si_tcp->is_socket_in_error_state()
Also the name is_socket_in_error_state is a bit confusing because it suggests some read only check but in fact it sets flags. Maybe check_previous_buffer_failure?

AlexanderGrissik · 2024-12-25T15:45:11Z

src/core/sock/sockinfo_tcp.cpp

@@ -5096,8 +5118,9 @@ int sockinfo_tcp::rx_wait_helper(int &poll_count, bool blocking)
    // It can be too expansive for the application to get nothing just because of lock contention.
    // In this case it will be better to have a lock() version of poll_and_process_element_rx.
    // And then we should continue polling untill we have ready packets or we drained the CQ.
+    bool all_drained = true;


Why these are here?

In case we got an ECQE - set the socket in a error state. Effectively this sends a TCP-RST. Signed-off-by: Tomer Cabouly <[email protected]>

tomerdbz force-pushed the propagate_4183221_to_release_3_40 branch from fa87745 to 519b192 Compare December 10, 2024 12:25

tomerdbz requested review from AlexanderGrissik and galnoam December 10, 2024 12:26

AlexanderGrissik reviewed Dec 11, 2024

View reviewed changes

src/core/dev/cq_mgr_tx.cpp Show resolved Hide resolved

tomerdbz force-pushed the propagate_4183221_to_release_3_40 branch from 519b192 to 4b913e2 Compare December 12, 2024 12:44

tomerdbz force-pushed the propagate_4183221_to_release_3_40 branch 2 times, most recently from 9dfc5b1 to e78f6dd Compare December 16, 2024 13:47

tomerdbz force-pushed the propagate_4183221_to_release_3_40 branch from 9620582 to 8b7a5b4 Compare December 17, 2024 10:34

tomerdbz force-pushed the propagate_4183221_to_release_3_40 branch 2 times, most recently from ea28c28 to 7c9b9a2 Compare December 18, 2024 09:04

AlexanderGrissik requested changes Dec 25, 2024

View reviewed changes

tomerdbz force-pushed the propagate_4183221_to_release_3_40 branch 4 times, most recently from 6b5b2bc to c174be4 Compare December 25, 2024 16:15

AlexanderGrissik approved these changes Dec 25, 2024

View reviewed changes

tomerdbz force-pushed the propagate_4183221_to_release_3_40 branch from c174be4 to 489c0bc Compare December 25, 2024 16:33

issue: 4183221 propagate socket errors

780d15e

In case we got an ECQE - set the socket in a error state. Effectively this sends a TCP-RST. Signed-off-by: Tomer Cabouly <[email protected]>

tomerdbz force-pushed the propagate_4183221_to_release_3_40 branch from 489c0bc to 780d15e Compare January 5, 2025 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue: 4183221 propagate socket errors #276

issue: 4183221 propagate socket errors #276

tomerdbz commented Dec 10, 2024 •

edited

Loading

tomerdbz commented Dec 10, 2024

AlexanderGrissik commented Dec 12, 2024

tomerdbz commented Dec 12, 2024

tomerdbz commented Dec 17, 2024

tomerdbz commented Dec 17, 2024

tomerdbz commented Dec 17, 2024

AlexanderGrissik commented Dec 25, 2024

AlexanderGrissik Dec 17, 2024

tomerdbz Dec 25, 2024

AlexanderGrissik Dec 17, 2024

AlexanderGrissik Dec 17, 2024

AlexanderGrissik Dec 17, 2024

AlexanderGrissik Dec 25, 2024

AlexanderGrissik Dec 25, 2024

AlexanderGrissik Dec 25, 2024

AlexanderGrissik Dec 25, 2024

AlexanderGrissik Dec 25, 2024

issue: 4183221 propagate socket errors #276

Are you sure you want to change the base?

issue: 4183221 propagate socket errors #276

Conversation

tomerdbz commented Dec 10, 2024 • edited Loading

Description

What

Why ?

How ?

Change type

Check list

tomerdbz commented Dec 10, 2024

AlexanderGrissik commented Dec 12, 2024

tomerdbz commented Dec 12, 2024

tomerdbz commented Dec 17, 2024

tomerdbz commented Dec 17, 2024

tomerdbz commented Dec 17, 2024

AlexanderGrissik commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerdbz commented Dec 10, 2024 •

edited

Loading