All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bobby Eshleman <bobbyeshleman@gmail.com>
To: netdev-bot+sashiko@kernel.org
Cc: juanlu@fastmail.com, dw@davidwei.uk, netdev@vger.kernel.org,
	io-uring@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-kernel@vger.kernel.org, kuba@kernel.org,
	asml.silence@gmail.com, pabeni@redhat.com, shuah@kernel.org
Subject: Re: [net-next,v6,5/6] selftests: net: add multithread server support to iou-zcrx
Date: Wed, 9 Sep 2026 09:16:58 -0700	[thread overview]
Message-ID: <aqGGevrEG5+iZosZ@devvm29614.prn0.facebook.com> (raw)
In-Reply-To: <178839229706.3394541.1555608492152892136@kernel.org>

On Wed, Sep 02, 2026 at 11:38:17PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider:
> 

[...]

> > @@ -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?

Just a heads up that I tested this on BRCM and did find this listen()
ordering issue to cause genuine (but flaky) regression for
nk_qlease.test_destroy. Moving the listen() to after wait(setup_done)
indeed fixed it.

Best,
Bobby

  parent reply	other threads:[~2026-09-09 16:17 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-31 17:34 [PATCH net-next v6 0/6] selftests: net: make iou-zcrx tests multithreaded Juanlu Herrero
2026-08-31 17:34 ` [PATCH net-next v6 1/6] selftests: net: fix get_refill_ring_size() to use its local variable Juanlu Herrero
2026-08-31 18:09   ` David Wei
2026-08-31 17:34 ` [PATCH net-next v6 2/6] selftests: net: remove unused variable in process_recvzc() Juanlu Herrero
2026-08-31 18:09   ` David Wei
2026-08-31 17:34 ` [PATCH net-next v6 3/6] selftests: net: refactor server state into struct thread_ctx Juanlu Herrero
2026-08-31 17:34 ` [PATCH net-next v6 4/6] selftests: net: add multithread client support to iou-zcrx Juanlu Herrero
2026-09-02 23:38   ` [net-next,v6,4/6] " netdev-bot+sashiko
2026-08-31 17:34 ` [PATCH net-next v6 5/6] selftests: net: add multithread server " Juanlu Herrero
2026-08-31 18:08   ` David Wei
2026-09-02 23:38   ` [net-next,v6,5/6] " netdev-bot+sashiko
2026-09-03 11:06     ` Paolo Abeni
2026-09-03 20:47       ` Juanlu Herrero
2026-09-09 16:16     ` Bobby Eshleman [this message]
2026-09-09 18:47       ` Juanlu Herrero
2026-08-31 17:34 ` [PATCH net-next v6 6/6] selftests: net: add rss_multiqueue test variant " Juanlu Herrero
2026-09-02 23:38   ` [net-next,v6,6/6] " netdev-bot+sashiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aqGGevrEG5+iZosZ@devvm29614.prn0.facebook.com \
    --to=bobbyeshleman@gmail.com \
    --cc=asml.silence@gmail.com \
    --cc=dw@davidwei.uk \
    --cc=io-uring@vger.kernel.org \
    --cc=juanlu@fastmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=netdev-bot+sashiko@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=shuah@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.