Skip to content

Conversation

@AlexanderGrissik
Copy link
Collaborator

Description

Removing TSO (TCP Segmentation offload) compilation flavor.

Why ?

Removing obsolete features

Change type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

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)

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR removes TCP Segmentation Offload (TSO) support from libvma, eliminating an obsolete feature compilation flavor. The change removes approximately 1,782 lines across 41 files.

Major changes:

  • Removed --enable-tso configuration option and DEFINED_TSO compile-time flag
  • Eliminated TSO-specific WQE handling in MLX5 queue pair manager (295 lines)
  • Removed TSO segment joining, splitting, and retransmission logic from lwip TCP stack (415 lines)
  • Deleted TSO-specific send paths in destination entry handling (174 lines)
  • Removed TSO capability detection and initialization from device and ring layers
  • Cleaned up TSO-related environment variables (VMA_TSO, VMA_TX_BUF_SIZE) and documentation

Code quality:
The removal is systematic and complete. All TSO-specific code paths are properly removed, including:

  • Build system configuration
  • Runtime configuration variables
  • Device capability detection
  • Data path implementations
  • Documentation

The PR maintains clean fallback to standard non-TSO code paths that were already present. No TSO references remain in the codebase after this change.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk - it's a clean feature removal with no functional regressions expected
  • Score of 5 reflects that this is a straightforward feature removal where all TSO code paths are properly eliminated and the codebase falls back to existing non-TSO implementations that were already tested. The changes are systematic, complete, and self-contained with no remaining TSO references.
  • No files require special attention - all changes are consistent deletions of TSO-specific code

Important Files Changed

File Analysis

Filename Score Overview
configure.ac 5/5 Removed --enable-tso configuration option and associated autoconf macro
src/vma/ib/base/verbs_extra.h 5/5 Removed TSO-related macros and type definitions for both verbs v2 and v3
src/vma/dev/qp_mgr_eth_mlx5.cpp 5/5 Removed TSO WQE handling, LSO fill functions, and TSO-specific doorbell ringing (295 lines removed)
src/vma/lwip/tcp.h 5/5 Removed TSO structure from tcp_pcb and TSO-related macros
src/vma/lwip/tcp_out.c 5/5 Removed TSO segment joining, splitting, and retransmission logic (415 lines removed)
src/vma/proto/dst_entry_tcp.cpp 5/5 Removed TSO-specific fast_send and slow_send implementations (174 lines removed)
src/vma/sock/sockinfo_tcp.cpp 5/5 Removed TSO PCB initialization and TSO-specific ip_output implementations (99 lines removed)

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant TCP as TCP Stack (lwip)
    participant Dst as dst_entry_tcp
    participant Ring as ring_simple
    participant QP as qp_mgr_eth_mlx5
    participant HW as Hardware NIC

    Note over App,HW: Before TSO Removal
    App->>TCP: tcp_write(data)
    TCP->>TCP: Check TSO capability
    alt TSO Enabled
        TCP->>TCP: tcp_tso_segment() - join segments
        TCP->>Dst: fast_send() with VMA_TX_PACKET_TSO
        Dst->>Ring: Check TSO caps (max_payload_sz, max_header_sz)
        Ring->>QP: send() with TSO WQE
        QP->>QP: fill_wqe_lso() - prepare TSO WQE
        QP->>HW: Post TSO WQE with large payload
    else TSO Disabled
        TCP->>Dst: fast_send() standard
        Dst->>Ring: send()
        Ring->>QP: send()
        QP->>QP: fill_wqe() - standard WQE
        QP->>HW: Post standard WQE
    end

    Note over App,HW: After TSO Removal
    App->>TCP: tcp_write(data)
    TCP->>Dst: fast_send() standard
    Dst->>Ring: send()
    Ring->>QP: send()
    QP->>QP: fill_wqe() - standard WQE
    QP->>HW: Post standard WQE
Loading

41 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@AlexanderGrissik
Copy link
Collaborator Author

bot:retest

@greptile-apps
Copy link

greptile-apps bot commented Nov 11, 2025

Greptile Overview

Greptile Summary

This PR comprehensively removes TSO (TCP Segmentation Offload) support from libvma, eliminating the compilation flavor that allowed hardware-accelerated large TCP segment transmission.

Key Changes

  • Removed --enable-tso build configuration option from autoconf
  • Eliminated all #ifdef DEFINED_TSO conditional compilation blocks across 42 files
  • Removed TSO-specific system variables (VMA_TSO, VMA_TX_BUF_SIZE) and runtime configuration
  • Deleted TSO-aware send paths in TCP/UDP protocol layers that supported >MSS sized segments
  • Removed hardware-specific TSO implementation (MLX5_OPCODE_TSO, fill_wqe_lso)
  • Eliminated TSO segment joining/splitting logic in lwIP TCP stack
  • Cleaned up TSO capability detection and validation code
  • Updated documentation to remove TSO references

Architecture Impact

The removal simplifies the codebase by consolidating dual code paths (TSO vs non-TSO) into a single standard path. TCP segmentation now occurs entirely in software at the MSS boundary, rather than being offloaded to hardware. This affects throughput characteristics for large data transfers but simplifies maintenance and reduces code complexity by ~1,771 lines.

Confidence Score: 5/5

  • This PR is safe to merge with no functional risks - it's a clean removal of an obsolete feature
  • The TSO removal is implemented systematically and completely across all layers. All #ifdef DEFINED_TSO blocks are properly removed, leaving only the standard code paths. The changes are consistent throughout the stack (build system, configuration, lwIP, protocol layers, device drivers, and documentation). No orphaned TSO references or incomplete removals were found. The retained code represents the well-tested non-TSO path that was already functional.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
configure.ac 5/5 removed TSO configuration option and related checks - clean removal of build-time feature flag
src/vma/util/sys_vars.cpp 5/5 removed TSO parameter initialization and environment variable parsing logic
src/vma/dev/qp_mgr_eth_mlx5.cpp 5/5 removed TSO-specific WQE filling logic, LSO implementation, and MLX5_OPCODE_TSO support - retained single unified code path
src/vma/dev/ring_simple.cpp 5/5 removed TSO validation, capability detection, and accessor methods - simplified send path
src/vma/lwip/tcp_out.c 5/5 removed TSO segment joining logic, split logic, and xmit_size_goal calculation - retained standard TCP segmentation
src/vma/proto/dst_entry_tcp.cpp 5/5 removed TSO-aware fast_send and slow_send implementations - retained non-TSO code paths
README 5/5 removed VMA_TSO and VMA_TX_BUF_SIZE documentation

Sequence Diagram

sequenceDiagram
    participant User as User Application
    participant Config as Build System
    participant SysVars as System Variables
    participant Socket as Socket Layer
    participant TCP as TCP/lwIP Layer
    participant DST as Destination Entry
    participant Ring as Ring (QP)
    participant QP as QP Manager (MLX5)
    participant HW as Hardware/Verbs

    Note over Config,HW: Before: TSO Enabled Path
    User->>Config: Configure with --enable-tso
    Config->>SysVars: Set DEFINED_TSO flag
    SysVars->>Socket: enable_tso=true, tx_buf_size set
    Socket->>TCP: Initialize TSO caps (max_payload_sz, max_header_sz)
    TCP->>TCP: tcp_tso_segment() - join segments
    TCP->>DST: Send large buffer (>MSS)
    DST->>Ring: fast_send with TSO flag
    Ring->>QP: fill_wqe_lso() - TSO work queue
    QP->>HW: MLX5_OPCODE_TSO with large payload

    Note over Config,HW: After: TSO Removed Path
    User->>Config: Configure (no TSO option)
    Config->>SysVars: No DEFINED_TSO flag
    SysVars->>Socket: Standard configuration only
    Socket->>TCP: Standard TCP segmentation
    TCP->>TCP: Standard MSS-sized segments
    TCP->>DST: Send standard buffer (<=MSS)
    DST->>Ring: fast_send standard path
    Ring->>QP: fill_wqe() - standard WQE
    QP->>HW: MLX5_OPCODE_SEND with normal payload
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

42 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

42 files reviewed, no comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

}

LWIP_DEBUGF(TCP_TSO_DEBUG | LWIP_DBG_TRACE,
LWIP_DEBUGF(LWIP_DBG_TRACE,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can combine to 1 line, it does not cross the chars count per line limit

m_sge = new (nothrow) struct ibv_sge [m_p_ring->get_max_send_sge()];
#else

m_sge = new (nothrow) struct ibv_sge [2];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 will be used for ZC yea? 1 for headers and 1 for data

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not related to ZC just general WQE posting with ibv_sge

@BasharRadya
Copy link
Collaborator

Removing TSO code, introduced DEAD code, this struct is only used in TSO deleted logic.

"typedef struct {
vma_wr_tx_packet_attr flags;
uint16_t mss;
} vma_send_attr;
"

@BasharRadya
Copy link
Collaborator

@dpressle Please review changes in CI files and approvde

@BasharRadya BasharRadya requested a review from dpressle November 19, 2025 15:36
Copy link
Collaborator

@BasharRadya BasharRadya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove dead code

Removing TSO (TCP Segmentation offload) compilation flavor.

Signed-off-by: Alexander Grissik <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

42 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@galnoam galnoam merged commit 5e448e2 into Mellanox:master Nov 20, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants