bgpd: reduce data copying in BGP I/O thread receive path

[enkechen-panw]

The current BGP receive path always copies data from ibuf_scratch
to ibuf_work ringbuf, even when packets are complete. This patch
minimizes data copying in the critical receive path.

BEFORE:

1. read() -> ibuf_scratch
2. ringbuf_put() copies ibuf_scratch -> ibuf_work
3. Validate header from ibuf_work
4. ringbuf_get() copies packet -> stream for main thread

AFTER:

1. Copy partial data (if any) from ibuf_work to ibuf_scratch
2. read() -> ibuf_scratch
3. Validate header directly from ibuf_scratch
4. memcpy() packet -> stream for main thread
5. Copy remaining partial data (if any) to ibuf_work

For complete packets, steps 1 and 5 are no-ops - no extra copy.

Key changes:

• Replace ringbuf with simple linear buffer (uint8_t *ibuf_work)
• Validate headers directly from ibuf_scratch buffer
• Add MTYPE_BGP_IBUF_WORK for memory tracking

Soft limit: inq_limit is checked before reading, not during parsing.
This keeps the kernel receive buffer full when the app queue is at
limit, allowing TCP to advertise a smaller/zero receive window and
propagate back-pressure to the sender. parse_buffer() processes all
complete messages from a single read, and ibuf_work is used for
partial packets only.

The changes are limited to the BGP I/O thread. The interface to
the main thread is unchanged - complete packets are still pushed
to connection->ibuf for processing by bgp_process_packet().

[enkechen-panw]

@greptileai review

[greptile-apps]

Greptile Summary

This PR replaces the ringbuf-based BGP receive path with an in-place parsing approach using a static scratch buffer (ibuf_scratch) and an on-demand partial-packet buffer (ibuf_work). The goal is to eliminate the extra copy from ibuf_scratch → ibuf_work → stream when complete packets arrive in a single read; with the new design, complete packets require only a single memcpy into the new stream.

• Core change: parse_buffer() processes packets directly from ibuf_scratch in a single pass, saving any partial tail into ibuf_work; on the next read bgp_read() prepends the saved partial bytes so new socket data always lands in a contiguous linear buffer.
• Queue limit semantics change: The inq_limit check is now evaluated before the read() syscall, keeping the kernel receive buffer full when the application queue is saturated and letting TCP propagate back-pressure to the remote sender via window-shrink.
• Memory tracking: A new MTYPE_BGP_IBUF_WORK tracks the on-demand partial-packet buffer that replaces the always-allocated ring buffer.

Confidence Score: 5/5

The change is safe to merge. The core invariant — ibuf_work is non-NULL if and only if ibuf_data_len > 0 — is correctly maintained across all code paths: normal completion, EAGAIN, TCP fatal error, and invalid-header teardown. Buffer size arithmetic ensures ibuf_scratch cannot overflow. The inq_limit back-pressure semantics are intentional and correctly re-armed by the existing bgp_process_packet drain logic.

All cleanup paths (bgp_stop, bgp_peer_connection_buffers_free, fatal in bgp_process_reads) correctly reset both ibuf_work and ibuf_data_len. The parse_buffer loop always advances by at least BGP_HEADER_SIZE per iteration and is bounded by the validated pktsize, so no infinite-loop or overflow is possible. The test suite exercises the full scratch/work round-trip, EAGAIN preservation, multi-message batches, and memory-allocation accounting.

bgpd/bgp_io.c warrants a careful read for any future modifications, as correctness depends on the ibuf_data_len/ibuf_work pair being updated atomically at the end of each bgp_process_reads() call.

Important Files Changed

Filename	Overview
bgpd/bgp_io.c	Core change: replaces ringbuf with in-place parsing. parse_buffer(), validate_header_from_buf(), and bgp_read() are well-structured. ibuf_work/ibuf_data_len invariant (non-NULL iff > 0) is maintained across all code paths including EAGAIN, fatal, and EBADMSG. The TRANS_ERR early-return correctly avoids the double-copy on EAGAIN.
bgpd/bgpd.h	Changed ibuf_work from ringbuf* to uint8_t* with a companion ibuf_data_len size field. Clean struct change, no issues.
bgpd/bgp_fsm.c	Updated bgp_stop() cleanup: ringbuf_del replaced by XFREE (which also NULLs the pointer) + ibuf_data_len = 0. Correct and complete.
bgpd/bgpd.c	bgp_peer_connection_buffers_free() updated identically to bgp_stop(). Consistent cleanup pattern.
bgpd/bgp_memory.c	Added DEFINE_MTYPE for BGP_IBUF_WORK. Trivial and correct.
bgpd/bgp_memory.h	Added DECLARE_MTYPE for BGP_IBUF_WORK. Trivial and correct.
doc/user/bgp.rst	Updated bgp input-queue-limit docs to reflect the new soft-limit semantics (checked before read, not during parsing).
tests/bgpd/test_ibuf_work.c	Good test coverage for parse_buffer(), scratch/work interaction, EAGAIN preservation, fatal cleanup, and memory tracking. Accesses statics via #include bgp_io.c — works because linker prefers object-file definitions over static-archive copies.
tests/bgpd/subdir.am	Added test_ibuf_work build target inside the BGPD conditional. Correct structure matching other test targets.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant K as Kernel TCP Buffer
    participant IO as I/O Thread (bgp_process_reads)
    participant S as ibuf_scratch (static)
    participant W as ibuf_work (per-conn, on demand)
    participant Q as connection->ibuf (FIFO)
    participant M as Main Thread

    Note over IO: Check inq_limit before reading
    IO->>K: read(fd, ibuf_scratch[+offset], readsize)
    K-->>S: new bytes land at ibuf_scratch[ibuf_data_len..]
    Note over S: ibuf_scratch[0..ibuf_data_len-1] = prior partial
    IO->>IO: parse_buffer(ibuf_scratch, total_len)
    loop For each complete packet
        IO->>Q: stream_fifo_push(pkt) [under io_mtx]
    end
    alt Partial tail remains
        IO->>W: memcpy(ibuf_work, tail, remaining)
        Note over W: ibuf_data_len = remaining
    else No remaining data
        IO->>W: XFREE(ibuf_work)
        Note over W: ibuf_data_len = 0
    end
    IO->>IO: event_add_read (reschedule)
    IO->>M: event_add_event (bgp_process_packet)
    M->>Q: stream_fifo_pop [under io_mtx]
    alt ibuf was at inq_limit before pop
        M->>IO: bgp_reads_on() — re-arm read event
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant K as Kernel TCP Buffer
    participant IO as I/O Thread (bgp_process_reads)
    participant S as ibuf_scratch (static)
    participant W as ibuf_work (per-conn, on demand)
    participant Q as connection->ibuf (FIFO)
    participant M as Main Thread

    Note over IO: Check inq_limit before reading
    IO->>K: read(fd, ibuf_scratch[+offset], readsize)
    K-->>S: new bytes land at ibuf_scratch[ibuf_data_len..]
    Note over S: ibuf_scratch[0..ibuf_data_len-1] = prior partial
    IO->>IO: parse_buffer(ibuf_scratch, total_len)
    loop For each complete packet
        IO->>Q: stream_fifo_push(pkt) [under io_mtx]
    end
    alt Partial tail remains
        IO->>W: memcpy(ibuf_work, tail, remaining)
        Note over W: ibuf_data_len = remaining
    else No remaining data
        IO->>W: XFREE(ibuf_work)
        Note over W: ibuf_data_len = 0
    end
    IO->>IO: event_add_read (reschedule)
    IO->>M: event_add_event (bgp_process_packet)
    M->>Q: stream_fifo_pop [under io_mtx]
    alt ibuf was at inq_limit before pop
        M->>IO: bgp_reads_on() — re-arm read event
    end

Loading

_{Reviews (4): Last reviewed commit: "tests: add unit tests for BGP ibuf_scrat..." | Re-trigger Greptile}

[enkechen-panw]

@greptileai review

[enkechen-panw]

@greptileai review

[donaldsharp]

at first glance this breaks the intentional line between the io pthread just reading data and the master pthread handling the actual parsing of packets. Without some serious proof that this is a huge improvement of some sort I'm mroe than a bit skeptical to even take such a change. Especially since this should probably be broken up into more commits instead of just a single big change.

[enkechen-panw]

at first glance this breaks the intentional line between the io pthread just reading data and the master pthread handling the actual parsing of packets. Without some serious proof that this is a huge improvement of some sort I'm mroe than a bit skeptical to even take such a change. Especially since this should probably be broken up into more commits instead of just a single big change.

Hi, @donaldsharp The optimization (and simplification) is only for the BGP I/O thread, and it does not change the interaction with the main thread. The commit title and commit message have been revised and clarified.

For splitting the commit, I have made the "input queue limit" change as a separate commit (and self-contained).

Thanks.

[donaldsharp]

If you don't answer what the performance improvements are this PR is gonna go nowhere. Please spend some time quantifying how this is better and how performance is being improved.

[enkechen-panw]

If you don't answer what the performance improvements are this PR is gonna go nowhere. Please spend some time quantifying how this is better and how performance is being improved.

I don't have the numbers right now. Will work on it.

[enkechen-panw]

Hi, @donaldsharp I instrumented the code, and got the comparison data:

Results (received 100k IPv6 routes, averaged over multiple runs):

  Routes received: 100000
  I/O calls:       ~30
  Bytes read:      1704622 (~1.7 MB)

  | Metric     | Baseline (ringbuf) | Optimized (ibuf_scratch) | Improvement   |
  |------------|--------------------|--------------------------| --------------|
  | Total time | ~0.71 ms           | ~0.50 ms                 | 30% faster    |
  | CPU time   | ~0.72 ms           | ~0.51 ms                 | 29% less CPU  |
  | Read time  | ~0.22 ms           | ~0.19 ms                 | ~same         |
  | Overhead   | ~0.49 ms           | ~0.31 ms                 | 37% reduction |

Note: These measurements are for the I/O thread ONLY - the time spent
reading from the socket and queuing packets for the main thread. The
actual BGP UPDATE parsing and route installation happens in the main
thread and is NOT included in these stats.

Validation: CPU time matches wall-clock time on both branches,
confirming the I/O thread is CPU-bound during the burst (not waiting
for data). The measurements are valid.

Thanks.