Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 2 (3) · Low: 3
- [High] listen() is now executed before the workers have registered
their zcrx ifqs, breaking the ordering that two existing in-tree tests…
- [Medium] A worker thread that is dispatched zero connections reports
success without receiving or verifying a single byte.
- [Medium] The new CQ sizing removes the implicit bound that kept the
zcrx refill-queue producer inside one ring.
- [Low] DISCARDED (resolved later in the series): the concern states that
the new multithread/multi-queue server path is dead code because iou-…
- [Low] `-t` is documented as a thread count but produces its square.
- [Low] `-t 0` is accepted and yields a silent PASS.
Pre-existing issues:
- [Medium] DISCARDED (pre-existing, medium): the concern states that the
5 s watchdog in server_worker() can never fire while the thread is…
- [Medium] DISCARDED (pre-existing, medium): the concern states that per-
worker resources are never released — no io_uring_queue_exit(), no…
- [Medium] DISCARDED (pre-existing, medium): the concern states that
process_recvzc() indexes the 2 MiB `payload` buffer with…
--
selftests: net: add multithread server support to iou-zcrx
This converts the iou-zcrx server into N worker threads, each owning one
RX queue with its own io_uring and zcrx ifq. The main thread keeps the
listening socket and dispatches accepted connections to workers by
matching SO_INCOMING_NAPI_ID against per-queue NAPI IDs queried over ynl.
A few questions about the new threading and setup ordering below.
> diff --git a/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
> b/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
> index f793a6c04e412..29905cb3b2433 100644
> --- a/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
> +++ b/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
[ ... ]
> @@ -97,9 +101,15 @@ struct thread_ctx {
> size_t ring_size;
> struct io_uring_zcrx_rq rq_ring;
> unsigned long area_token;
> - int connfd;
> - bool stop;
> - size_t received;
> + int queue_id;
> + int napi_id;
> + pthread_barrier_t *setup_done;
> + pthread_barrier_t *dispatch_done;
> +
> + int connfds[MAX_CONNS_PER_THREAD];
> + size_t received[MAX_CONNS_PER_THREAD];
> + int oneshot_recvs[MAX_CONNS_PER_THREAD];
> + int nr_conns;
> };
[ ... ]
> @@ -304,10 +296,10 @@ static void process_recvzc(struct thread_ctx *ctx,
> struct io_uring_cqe *cqe,
> data = (char *)ctx->area_ptr + (rcqe->off & mask);
>
> for (i = 0; i < n; i++) {
> - if (*(data + i) != payload[(ctx->received + i)])
> + if (*(data + i) != payload[(ctx->received[conn_idx] + i)])
> error(1, 0, "payload mismatch at %d", i);
> }
> - ctx->received += n;
> + ctx->received[conn_idx] += n;
[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but the
index into payload[] is still unbounded. payload is a single
posix_memalign(page_size, SEND_SIZE) allocation in main(), while
ctx->received[conn_idx] only ever accumulates cqe->res, which is driven
by whatever the peer sends into the wildcard-bound listener.
Would it be worth adding a check that
ctx->received[conn_idx] + n <= cfg_send_size before the comparison loop,
so a peer sending more than cfg_send_size bytes produces a clear error
rather than an out-of-bounds read of payload[]?
>
> rqe = &ctx->rq_ring.rqes[(ctx->rq_ring.rq_tail & rq_mask)];
> rqe->off = (rcqe->off & ~IORING_ZCRX_AREA_MASK) | ctx->area_token;
> @@ -320,28 +312,124 @@ static void server_loop(struct thread_ctx *ctx)
> struct io_uring_cqe *cqe;
> unsigned int count = 0;
> unsigned int head;
> - int i, ret;
>
> io_uring_submit_and_wait(&ctx->ring, 1);
>
> io_uring_for_each_cqe(&ctx->ring, head, cqe) {
> - if (cqe->user_data == 1)
> - process_accept(ctx, cqe);
> - else if (cqe->user_data == 2)
> - process_recvzc(ctx, cqe);
> - else
> - error(1, 0, "unknown cqe");
> + process_recvzc(ctx, cqe, cqe->user_data);
> count++;
> }
> io_uring_cq_advance(&ctx->ring, count);
> }
>
> -static void run_server(void)
> +static void *server_worker(void *arg)
> {
> - struct thread_ctx ctx = {};
> - unsigned int flags = 0;
> - int fd, enable, ret;
> + struct io_uring_params params = { };
> + struct thread_ctx *ctx = arg;
> uint64_t tstop;
> + int i;
> +
> + params.flags |= IORING_SETUP_COOP_TASKRUN;
> + params.flags |= IORING_SETUP_SINGLE_ISSUER;
> + params.flags |= IORING_SETUP_DEFER_TASKRUN;
> + params.flags |= IORING_SETUP_SUBMIT_ALL;
> + params.flags |= IORING_SETUP_CQE32;
> + params.flags |= IORING_SETUP_CQSIZE;
> + params.cq_entries = AREA_SIZE / page_size;
[Severity: Medium]
Can this overrun the zcrx refill ring? The CQ is now sized to
AREA_SIZE / page_size (8192 entries), while setup_zcrx() still uses
rq_entries = 4096.
server_loop() drains every available CQE in one batch, and
process_recvzc() publishes exactly one refill descriptor per CQE without
ever consulting khead:
rqe = &ctx->rq_ring.rqes[(ctx->rq_ring.rq_tail & rq_mask)];
...
io_uring_smp_store_release(ctx->rq_ring.ktail, ++ctx->rq_ring.rq_tail);
Previously io_uring_queue_init(512, ...) gave cq_entries = 1024, which is
strictly smaller than rq_entries, so a single drain could not wrap the
refill ring. With 8192 CQ entries a drain of more than 4096 zcrx CQEs
overwrites descriptors that the kernel has not consumed yet, and the
kernel side clamps rather than complains:
io_uring/zcrx.c:__zcrx_rq_entries() {
u32 entries = rq->cached_tail - rq->cached_head;
return min(entries, rq->nr_entries);
}
so the surplus buffers are simply never returned to the page pool. Should
the producer only publish while (ktail - khead) < rq_entries, or should
rq_entries be at least cq_entries?
> +
> + io_uring_queue_init_params(512, &ctx->ring, ¶ms);
> + setup_zcrx(ctx);
> +
> + if (cfg_dry_run)
> + return NULL;
> +
> + pthread_barrier_wait(ctx->setup_done);
> + pthread_barrier_wait(ctx->dispatch_done);
> +
> + for (i = 0; i < ctx->nr_conns; i++) {
> + if (cfg_oneshot) {
> + ctx->oneshot_recvs[i] = cfg_oneshot_recvs;
> + add_recvzc_oneshot(ctx, i, page_size);
> + } else {
> + add_recvzc(ctx, i);
> + }
> + }
> +
> + tstop = gettimeofday_ms() + 5000;
> + while (ctx->nr_conns > 0 && gettimeofday_ms() < tstop)
> + server_loop(ctx);
> +
> + if (ctx->nr_conns != 0)
> + error(1, 0, "test failed: %d connections incomplete",
> + ctx->nr_conns);
[Severity: Medium]
Can a worker that received no connections at all report success here?
nr_conns is used both as the dispatch count and as the live-connection
countdown, so with nr_conns == 0 the arming loop above does nothing, the
drain loop is skipped, and this final check passes without a single byte
having been received or verified.
Dispatch in run_server() is purely SO_INCOMING_NAPI_ID based, and nothing
guarantees a non-empty distribution. With -t 2 (total_conns = 4) all four
flows can hash onto one RSS queue, in which case the second worker exits
successfully having done nothing, and the multi-queue test reports pass
while only one queue was exercised. The previous code failed loudly in
this situation with "test failed".
Would a post-dispatch assertion in run_server() that every worker owns at
least one connection, plus a check on the received byte totals, keep the
old strictness?
The reverse skew looks unhandled too: total_conns is cfg_num_threads
squared, and if they all land on one worker then for -t 9 and above this
trips the "worker %d connection overflow" error in run_server(), which
would be a spurious failure rather than a real one.
[Severity: Medium]
This isn't a bug introduced by this patch, but the 5 s deadline can still
never fire while the thread is parked inside server_loop():
io_uring_submit_and_wait(&ctx->ring, 1);
The deadline is only re-evaluated between blocking waits, so a stalled
connection hangs the worker (and then pthread_join() in run_server())
instead of producing the "connections incomplete" failure. The same shape
existed before the patch, but it now applies per worker thread. Could the
wait use a timeout so the watchdog is effective?
> +
> + return NULL;
> +}
[ ... ]
> +static void run_server(void)
> +{
> + pthread_barrier_t setup_done, dispatch_done;
> + int total_conns, accepted = 0, connfd;
> + struct thread_ctx *ctxs;
> + int fd, ret, enable, i;
> + unsigned int ifindex;
> + pthread_t *threads;
> +
> + ctxs = calloc(cfg_num_threads, sizeof(*ctxs));
> + threads = calloc(cfg_num_threads, sizeof(*threads));
> + if (!ctxs || !threads)
> + error(1, 0, "calloc()");
[Severity: Low]
Should parse_opts() reject -t 0? cfg_num_threads is taken straight from
strtoul(optarg, NULL, 0) with no positivity check, and with 0:
- calloc(0, ...) returns a non-NULL pointer, so this guard passes
- both barriers are initialised with a single participant
- no workers are created
- total_conns is 0, so the accept loop never runs
- both barrier waits return immediately and run_server() returns 0
The client side also computes total_conns as cfg_num_threads squared, so
it makes no connection and exits successfully as well. Either way the
binary reports success without doing any work.
>
> fd = socket(AF_INET6, SOCK_STREAM, 0);
> if (fd == -1)
> @@ -356,29 +444,68 @@ static void run_server(void)
> if (ret < 0)
> error(1, 0, "bind()");
>
> - flags |= IORING_SETUP_COOP_TASKRUN;
> - flags |= IORING_SETUP_SINGLE_ISSUER;
> - flags |= IORING_SETUP_DEFER_TASKRUN;
> - flags |= IORING_SETUP_SUBMIT_ALL;
> - flags |= IORING_SETUP_CQE32;
> + pthread_barrier_init(&setup_done, NULL, cfg_num_threads + 1);
> + pthread_barrier_init(&dispatch_done, NULL, cfg_num_threads + 1);
> +
> + for (i = 0; i < cfg_num_threads; i++) {
> + ctxs[i].queue_id = cfg_queue_id + i;
> + ctxs[i].setup_done = &setup_done;
> + ctxs[i].dispatch_done = &dispatch_done;
> + }
>
> - io_uring_queue_init(512, &ctx.ring, flags);
> + for (i = 0; i < cfg_num_threads; i++) {
> + ret = pthread_create(&threads[i], NULL,
> + server_worker, &ctxs[i]);
> + if (ret)
> + error(1, ret, "pthread_create()");
> + }
>
> - setup_zcrx(&ctx);
> if (cfg_dry_run)
> - return;
> + goto join;
>
> if (listen(fd, 1024) < 0)
> error(1, 0, "listen()");
>
> - add_accept(&ctx, fd);
> + pthread_barrier_wait(&setup_done);
[Severity: High]
Is the listen() intentionally moved before the workers have registered
their ifqs? The main thread creates the threads, calls listen(), and only
then waits on setup_done, while the workers reach that barrier after
io_uring_queue_init_params() plus setup_zcrx(), i.e. after
io_uring_register_ifq() has restarted the device RX queue.
Before the patch the sequence was serialized in one thread:
io_uring_queue_init(512, &ctx.ring, flags);
setup_zcrx(&ctx);
...
listen(fd, 1024);
so the port only became visible once the memory provider was installed.
Two existing tests depend on that ordering. wait_port_listen() in
net/lib/py/utils.py polls every 5 ms, so it can now return while
registration is still in flight:
iou-zcrx.py:test_zcrx_large_chunks() {
wait_port_listen(cfg.port, proto="tcp")
check_iou_rx_buf_len(cfg, rx_buf_len)
}
check_iou_rx_buf_len() raises "Expected 1 io-uring page pool, found 0",
and nk_qlease.py:test_destroy() does queue_get() plus
ksft_in("io-uring", queue_info) immediately after wait_port_listen() with
no sleep.
Beyond the harness races, data delivered into the queue before the
provider exists is serviced by the copy fallback rather than zero copy,
and the queue restart during registration can drop in-flight frames.
Would moving listen() below pthread_barrier_wait(&setup_done) restore the
previous guarantee?
>
> - tstop = gettimeofday_ms() + 5000;
> - while (!ctx.stop && gettimeofday_ms() < tstop)
> - server_loop(&ctx);
> + if (cfg_num_threads > 1) {
> + ifindex = if_nametoindex(cfg_ifname);
> + if (!ifindex)
> + error(1, 0, "bad interface name: %s", cfg_ifname);
> + for (i = 0; i < cfg_num_threads; i++)
> + ctxs[i].napi_id = query_napi_id(ifindex,
> + ctxs[i].queue_id);
> + }
> +
> + total_conns = cfg_num_threads * cfg_num_threads;
[Severity: Low]
This isn't a bug, but -t is documented as a thread count while both
run_server() and run_client() square it. usage() advertises
-t<num_threads>, and run_client() creates total_conns threads with
total_conns = cfg_num_threads * cfg_num_threads, so -t 8 produces 64
client threads and 64 connections, not 8. That also interacts with the
64-entry MAX_CONNS_PER_THREAD limit. Could the option name or the usage
text spell out the quadratic connection count?
> +
> + while (accepted < total_conns) {
> + int idx = 0;
> +
> + connfd = accept(fd, NULL, NULL);
> + if (connfd < 0)
> + error(1, errno, "accept()");
>
> - if (!ctx.stop)
> - error(1, 0, "test failed\n");
> + if (cfg_num_threads > 1)
> + idx = find_thread_by_conn(ctxs, connfd);
> +
> + if (ctxs[idx].nr_conns >= MAX_CONNS_PER_THREAD)
> + error(1, 0, "worker %d connection overflow", idx);
> + ctxs[idx].connfds[ctxs[idx].nr_conns++] = connfd;
> + accepted++;
> + }
> +
> + pthread_barrier_wait(&dispatch_done);
> +
> +join:
> + for (i = 0; i < cfg_num_threads; i++)
> + pthread_join(threads[i], NULL);
> +
> + pthread_barrier_destroy(&setup_done);
> + pthread_barrier_destroy(&dispatch_done);
> + close(fd);
> + free(threads);
> + free(ctxs);
[Severity: Medium]
This is a pre-existing gap rather than something the patch introduces,
but the teardown is now asymmetric. The barriers, the listening fd and
the two calloc'ed arrays are released here, while the per-worker
resources are not: there is no io_uring_queue_exit(), no munmap() of the
AREA_SIZE zero-copy area or of the refill ring mapping created in
setup_zcrx(), and the accepted connfds stored in ctxs[idx].connfds[] are
never closed. With cfg_num_threads workers this is now multiplied by the
thread count, including on the cfg_dry_run early return in
server_worker(). Would it be worth adding the matching cleanup?
> }
>
> static void *client_worker(void *arg)
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-iou-zcrx-v6-0-02d1d012f0fd%40fastmail.com
