ZhengweiZhu opened a new issue, #3463:
URL: https://github.com/apache/brpc/issues/3463
**Describe the bug**
After reviewing and testing the UBRing/UBShmTransport introduced by #3290,
we found several correctness, scalability, portability, performance, and
maintainability issues.
The two shared-memory backends should be considered separately:
- `--ub_shm_type=1`: local POSIX shared memory (IPC)
- `--ub_shm_type=2`: remote shared memory provided by UBS
Some constraints that may be necessary for UBS do not apply to local IPC
shared memory.
### 1. Deleting a timer may block indefinitely
Linux timerfds are created as blocking file descriptors:
```cpp
timerfd_create(CLOCK_MONOTONIC, 0);
```
Both `DeleteTimerInner()` and `DeleteTimerSafe()` execute a blocking read
before closing the fd:
```cpp
uint64_t exp = 0;
read(fd, &exp, sizeof(exp));
close(fd);
```
There is a deterministic problematic path for one-shot timers:
1. `TimerEpoll()` reads and consumes the timerfd expiration.
2. It calls `UnifiedCallback()`.
3. For a non-periodic timer, `UnifiedCallback()` calls `DeleteTimerInner()`.
4. `DeleteTimerInner()` reads the same timerfd again.
5. Since the one-shot timer has already expired and was drained, the second
blocking `read()` may never return.
For periodic timers, deletion may also block until the next expiration. For
example, deleting a heartbeat timer may block for up to the heartbeat interval.
This can stall connection cleanup and delay releasing resources after a peer
disconnects or crashes.
Timer deletion should not require a blocking read. If the custom
implementation is retained, timerfds should at least use `TFD_NONBLOCK |
TFD_CLOEXEC`, and `EAGAIN` should be treated as normal. Removing the fd from
epoll and closing it without draining it may be sufficient.
### 2. The custom timer subsystem should preferably be replaced with brpc's
existing timer implementation
The timer problems are not isolated defects. They are consequences of
introducing a second timer framework based on timerfd/epoll alongside the timer
framework already maintained by brpc.
brpc already provides:
- `bthread::TimerThread`
- `bthread_timer_add()` and `bthread_timer_del()`
- Versioned timer task IDs
- Cancellation semantics that account for a callback already running
- A shared global timer thread
UBRing can schedule one-shot tasks through the global
`bthread::TimerThread`. Periodic heartbeat or connection-check tasks can
reschedule themselves after each callback. Potentially blocking resource
cleanup should be handed off to a bthread or worker; the timer callback should
only validate state and schedule the cleanup work.
Unless UBRing has a concrete timer requirement that the existing
implementation cannot satisfy, maintaining an independent timer subsystem does
not appear justified. Reusing the existing implementation would eliminate most
of the following newly introduced problems.
#### 2.1 Timer context storage scales with `RLIMIT_NOFILE`
`TimerInit()` allocates a context for every possible process fd and
initializes a spinlock for every entry:
```cpp
getrlimit(RLIMIT_NOFILE, &rlim);
g_max_system_fd = (uint32_t)rlim.rlim_cur;
g_timer_fd_ctx_map =
(TimerFdCtx*)malloc(sizeof(TimerFdCtx) * g_max_system_fd);
```
On x86-64, `sizeof(TimerFdCtx)` is approximately 32 bytes:
| `RLIMIT_NOFILE` | Approximate timer table size |
|---:|---:|
| 1,024 | 32 KiB |
| 20,480,000 | 625 MiB |
The allocation and initialization cost depends on the maximum possible fd
count rather than the number of active UBRing timers.
#### 2.2 Using the numeric fd as timer identity introduces an fd-reuse race
Epoll stores only the numeric fd:
```cpp
event.data.fd = timer_fd;
```
The callback context is found through:
```cpp
g_timer_fd_ctx_map[timer_fd]
```
After a timer is removed and closed, Linux may immediately reuse the fd for
another timer. A stale epoll event containing the old fd can then refer to the
new timer context.
Possible consequences include executing a new timer's callback for an old
event, deleting the wrong timer, or accessing callback arguments after their
owning connection has been released.
Changing the container from an fd-indexed array to a map alone is not
sufficient. Stale events need to be rejected with a generation-safe identity.
`bthread::TimerThread` already uses versioned task IDs for this purpose.
#### 2.3 Timer initialization contains a race
`StartTimeEpoll()` creates the timer thread before `TimerInit()` sets:
```cpp
g_timer_module_initialized = 1;
```
The new thread may run immediately and observe the old value:
```cpp
if (g_timer_module_initialized <= 0) {
break;
}
```
In that case, the timer thread exits during initialization. Initialization
and destruction also use non-atomic global state without an explicit
initialization lock.
#### 2.4 The default close-check timer is too frequent
The close-check timer is configured as:
```cpp
DEFINE_int32(ub_event_queue_timer_interval, 100, ...);
uint32_t interval = FLAGS_ub_event_queue_timer_interval * 1000;
```
The resulting default period is 100 microseconds, or 10,000 wakeups per
second for every UBRing connection. Each connection also creates heartbeat and
cleanup timers.
The close transition should preferably be event-driven. If polling is
unavoidable, the default interval should be significantly coarser and its unit
should be documented.
#### 2.5 All timer callbacks execute serially on one pthread
`TimerEpoll()` invokes callbacks inline from the only timer thread. Some
callbacks perform connection cleanup and may wait for `ub_flying_io_timeout`,
currently five seconds.
A slow callback therefore delays heartbeat and cleanup callbacks for all
other UBRing connections. Timer callbacks should schedule potentially blocking
work on an appropriate bthread or worker instead of executing it inline.
### 3. The macOS timerfd emulation is not functionally equivalent
The macOS implementation creates a pipe and returns only its read end:
```cpp
static int timerfd_create_macosx(...) {
int pipefd[2];
pipe(pipefd);
return pipefd[0];
}
```
There are several issues:
- The pipe write end is leaked.
- Nothing writes expiration counters into the pipe.
- `TimerEpoll()` calls `read(timer_fd, ...)` after receiving an
`EVFILT_TIMER` event, so it can block forever on the empty pipe.
- `timerfd_settime_macosx()` is a no-op.
- `EV_SET()` uses `it_value` as the repeating kqueue interval and does not
correctly model `it_interval`.
- Many timers use an initial expiration of one nanosecond, which becomes
zero milliseconds after conversion.
The kqueue path should process the expiration count from `struct
kevent::data` and should not read from a pipe. Reusing brpc's existing timer
implementation would remove the need for this platform-specific emulation.
### 4. The fixed 60-byte payload should be handled differently for IPC and
UBS
The current message format is fixed to one 64-byte cache line:
```cpp
#define UBR_MSG_HEADER_LEN 4
#define UBR_MSG_PAYLOAD_LEN 60
#define UBR_MSG_LEN 64
```
The current send path first assembles the packet in local memory:
```cpp
memcpy(local_msg_space.payload, input, length);
Copy64Byte(remote_data_queue_slot, &local_msg_space);
```
This design should not be applied identically to both backends.
#### 4.1 Local IPC shared memory
For the IPC backend, both processes map the same POSIX shared-memory object.
The sender can write directly into the destination ring slot.
The current staging path introduces an avoidable copy:
```text
IOBuf/iovec
-> local_msg_space
-> shared-memory ring slot
```
It can instead be:
```text
IOBuf/iovec
-> shared-memory ring slot payload
-> publish the slot state
```
The producer should write the payload and metadata first, then publish the
slot with release semantics. The consumer should check the slot state with
acquire semantics before reading the payload.
This removes one local memory copy and allows the IPC backend to use a more
suitable payload size. A 4 KiB or 64 KiB attachment currently has to be split
into approximately 69 or 1,093 chunks respectively.
The IPC payload size could be a startup-only option, with the selected
format included in the handshake. It must not be changed while connections
using that format are alive.
#### 4.2 Remote UBS shared memory
For UBS, the 64-byte unit may be required by the UB transport's indivisible
remote-store semantics. If that hardware/API requirement is confirmed, UBS may
need to continue assembling a complete 64-byte packet locally before publishing
it remotely.
However, this is a backend-specific constraint and should not force the IPC
backend to use the same format.
The non-`LS64` implementation of `Copy64Byte()` currently falls back to
ordinary `memcpy()`:
```cpp
#ifdef LS64
// ST64B
#else
memcpy(dst, src, 64);
#endif
```
An ordinary `memcpy()` does not itself guarantee an indivisible 64-byte
store. The required atomicity and visibility guarantees should therefore be
documented and validated separately for ARM LS64/ST64B, other local
architectures, UBS remote mappings, and POSIX IPC shared memory.
A possible design is to let each backend provide its own payload size, slot
alignment, publication operation, and memory-ordering requirements. The chosen
format or format version should be negotiated during the UBRing handshake.
### 5. There are several code-style and naming issues
The change contains personal debugging remnants, ambiguous flag names, and
terminology copied from the RDMA transport without adapting it to UBRing.
#### 5.1 Personal debugging remnants should be removed
`src/brpc/ubshm_transport.cpp` contains commented debugging logs with a
personal identifier:
```cpp
// LOG(INFO) << "mwj pollin4=" << pollin ...
// LOG(INFO) << "mwj pollin1=" << pollin;
// LOG(INFO) << "mwj pollin2=" << pollin << " mwj_ret=" << mwj_ret;
// LOG(INFO) << "mwj return 0";
```
It also contains:
```cpp
auto mwj_ret = bthread::butex_wait(...);
```
`src/brpc/ubshm/ub_endpoint.cpp` contains:
```cpp
// TODO mwj should polling start after the connection is established?
```
Personal debugging logs should be deleted rather than committed as comments.
The local variable should have a descriptive name such as `wait_rc`. The TODO
should either be resolved or rewritten as a clear actionable comment without an
author identifier.
#### 5.2 Time-related gflags do not document their units
The following flags do not indicate their units in either the flag name or
description:
```cpp
DEFINE_int32(ub_disconnect_timeout, 5, "Ubshm disconnection timeout.");
DEFINE_int32(ub_connect_timeout, 1, "Ubshm connection timeout.");
DEFINE_int32(ub_hb_timer_interval, 5, "Heartbeat timer interval.");
DEFINE_int32(ub_event_queue_timer_interval, 100,
"Interval of the disconnection timer.");
DEFINE_int32(ub_flying_io_timeout, 5,
"Waiting time for stopping data sending and receiving...");
```
The code uses different units:
- `ub_disconnect_timeout`: seconds
- `ub_connect_timeout`: seconds
- `ub_hb_timer_interval`: seconds
- `ub_event_queue_timer_interval`: microseconds
- `ub_flying_io_timeout`: seconds
A user cannot determine these units from `--help`. The unit should be
encoded in the flag name and description, for example:
```text
ub_disconnect_timeout_s
ub_connect_timeout_s
ub_hb_timer_interval_s
ub_event_queue_timer_interval_us
ub_flying_io_timeout_s
```
If renaming is considered incompatible, the descriptions must at least state
the exact units.
The conversion should also use an existing named constant or time utility
instead of:
```cpp
#define TIME_COVERSION 1000
```
`TIME_COVERSION` is misspelled and does not explain what is being converted.
The existing `USEC_TO_NSEC` constant or a standard time conversion helper would
be clearer.
#### 5.3 `_cq_sid` is RDMA terminology and is inappropriate for UBRing
`UBShmEndpoint` uses the following names:
```cpp
SocketId _cq_sid;
struct CqSidOp;
std::unordered_set<CqSidOp, ...> cq_sids;
```
It also logs:
```cpp
PLOG(WARNING) << "Fail to create socket for cq";
```
These names appear to have been copied from `RdmaEndpoint`. In RDMA, `CQ`
means Completion Queue, so `_cq_sid` describes a socket associated with an RDMA
completion queue. UBRing does not have an RDMA completion queue.
In `UBShmEndpoint`, this synthetic `SocketId` represents an entry registered
with the UBRing poller. Names should describe that role, for example
`_poller_sid`, `PollerSidOp`, and `poller_sids`, or equivalent event-source
terminology.
Copying the RDMA polling structure may be reasonable, but RDMA-specific
terminology should not be retained when the underlying concept is different.
### 6. Current tests do not cover lifecycle and data-path cases
`test/brpc_ubring_unittest.cpp` currently covers hello-message
serialization, endpoint construction, basic IPC allocation, and reset behavior.
It does not cover:
- End-to-end UBRing send and receive
- Large attachments spanning many chunks
- Client or server crash/disconnect cleanup
- One-shot timer expiration and deletion
- Concurrent timer deletion and callback execution
- Numeric fd reuse with stale epoll events
- High `RLIMIT_NOFILE`
- Multiple concurrent UBRing connections
- macOS timer behavior
- Different IPC/UBS message formats
- Validation of time-related gflag units and default intervals
### 7. Bazel and CI support was incomplete, but has been fixed
The original UBRing change documented:
```bash
bazel build --define=with_ubring=true
```
but the Bazel build did not define `BRPC_WITH_UBRING`, did not expose the
performance example targets, and the Bazel CI jobs did not build or run
UBRing-enabled unit tests.
This has already been fixed by #3445, which added:
- `--define=BRPC_WITH_UBRING=true`
- `--config=ubring`
- Bazel targets for the UBRing performance client and server
- UBRing-enabled Bazel compilation and unit-test jobs
- Updated English and Chinese documentation
No further action is requested for this resolved build issue. It is listed
here for completeness because it prevented the original implementation from
being exercised by Bazel CI.
**To Reproduce**
#### Cleanup hang
Build brpc and the performance example:
```bash
bazel build --config=ubring \
//example:ubring_performance_server \
//example:ubring_performance_client
```
Start the IPC server:
```bash
./bazel-bin/example/ubring_performance_server \
--use_ubring=true \
--ub_shm_type=1 \
--port=8002
```
Run a client and let it exit or terminate it:
```bash
./bazel-bin/example/ubring_performance_client \
--use_ubring=true \
--ub_shm_type=1 \
--servers=127.0.0.1:8002 \
--thread_num=1 \
--queue_depth=1 \
--attachment_size=4096 \
--test_seconds=30
```
Inspect a process that becomes stuck during cleanup. The stack can stop in
the blocking `read()` called by `DeleteTimerInner()` or `DeleteTimerSafe()`.
#### Timer table allocation
```bash
ulimit -n 20480000
```
Initialize the UBRing timer module and inspect startup time and RSS.
`TimerInit()` allocates and initializes approximately 625 MiB of timer contexts
even when no UBRing connection exists.
#### IPC chunking
Send 4 KiB and 64 KiB attachments through the IPC backend. The current
60-byte payload creates approximately:
```text
4 KiB -> 69 chunks
64 KiB -> 1093 chunks
```
**Expected behavior**
- UBRing should reuse brpc's existing timer framework unless a concrete
unsupported requirement is documented.
- Timer deletion must never block waiting for another expiration.
- Timer callbacks and deletion must be safe when executed concurrently.
- Stale epoll events must not operate on a new timer that reused the same fd.
- Timer bookkeeping should scale with active timers, not `RLIMIT_NOFILE`.
- Timer callbacks should perform lightweight state transitions and dispatch
blocking cleanup elsewhere.
- macOS timers should have behavior equivalent to the Linux implementation.
- IPC and UBS should use backend-appropriate payload and publication
mechanisms.
- IPC should write directly into shared-memory slots and publish them with
explicit release/acquire ordering.
- UBS should retain a 64-byte packet only if required by documented
hardware/API semantics.
- The active message format must be agreed by both peers.
- All time-related gflags should include their units in their names and
descriptions.
- UBRing code should not contain personal debugging markers or RDMA-specific
terminology for unrelated concepts.
- CI tests should exercise the full send/receive and connection lifecycle.
**Versions**
OS: Helix 8.4r, x86-64
Compiler: GCC 8.5.0
brpc: UBRing introduced by `72bf13a3` (#3290); issues still present on
current master
protobuf: Not relevant to these issues
Build: Bazel UBRing support fixed by #3445
**Additional context/screenshots**
We performed local IPC performance tests with 32 MiB data queues. An
experimental version used an 8 KiB payload and wrote directly into the IPC ring
slot.
| Workload | Original 60B format | IPC 8 KiB direct-write |
|---|---:|---:|
| 4 threads, depth 64, 4 KiB attachment | 761 MB/s | 871-905 MB/s |
| 4 threads, depth 64, 64 KiB attachment | 1,869 MB/s | 3,559-3,925 MB/s |
| 1 thread, depth 1, 4 KiB attachment | 102.6 MB/s | 106.4-107.4 MB/s |
These numbers are directional rather than a strict single-variable A/B
comparison because both the chunk payload size and the IPC copy path were
changed. They nevertheless show that the fixed 60-byte format has a significant
cost for larger messages.
The experimental direct-write IPC path was:
```text
IOBuf/iovec
-> destination shared-memory payload
-> write metadata
-> release-store the ready flag
```
The receiver used an acquire-load of the ready flag before consuming the
payload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
