Message-ID: <CADUfDZp+FoFCMoi_PJGe7vnNUJ47uxfcQ7irLX9h61xOuWcjvA@mail.gmail.com>
Date: Tue, 30 Sep 2025 16:37:41 -0700
From: Caleb Sander Mateos <csander@...estorage.com>
To: Jens Axboe <axboe@...nel.dk>
Cc: Pavel Begunkov <asml.silence@...il.com>, io-uring@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/5] io_uring: avoid uring_lock for IORING_SETUP_SINGLE_ISSUER

On Wed, Sep 10, 2025 at 8:36 AM Jens Axboe <axboe@...nel.dk> wrote:
>
> On 9/10/25 5:57 AM, Pavel Begunkov wrote:
> > On 9/9/25 14:35, Jens Axboe wrote:
> >>
> >> On Thu, 04 Sep 2025 11:08:57 -0600, Caleb Sander Mateos wrote:
> >>> As far as I can tell, setting IORING_SETUP_SINGLE_ISSUER when creating
> >>> an io_uring doesn't actually enable any additional optimizations (aside
> >>> from being a requirement for IORING_SETUP_DEFER_TASKRUN). This series
> >>> leverages IORING_SETUP_SINGLE_ISSUER's guarantee that only one task
> >>> submits SQEs to skip taking the uring_lock mutex in the submission and
> >>> task work paths.
> >>>
> >>> [...]
> >>
> >> Applied, thanks!
> >>
> >> [1/5] io_uring: don't include filetable.h in io_uring.h
> >>        commit: 5d4c52bfa8cdc1dc1ff701246e662be3f43a3fe1
> >> [2/5] io_uring/rsrc: respect submitter_task in io_register_clone_buffers()
> >>        commit: 2f076a453f75de691a081c89bce31b530153d53b
> >> [3/5] io_uring: clear IORING_SETUP_SINGLE_ISSUER for IORING_SETUP_SQPOLL
> >>        commit: 6f5a203998fcf43df1d43f60657d264d1918cdcd
> >> [4/5] io_uring: factor out uring_lock helpers
> >>        commit: 7940a4f3394a6af801af3f2bcd1d491a71a7631d
> >> [5/5] io_uring: avoid uring_lock for IORING_SETUP_SINGLE_ISSUER
> >>        commit: 4cc292a0faf1f0755935aebc9b288ce578d0ced2
> >
> > FWIW, from a glance that should be quite broken, there is a bunch of
> > bits protected from parallel use by the lock. I described this
> > optimisation few years back around when first introduced SINGLE_ISSUER
> > and the DEFER_TASKRUN locking model, but to this day think it's not
> > worth it as it'll be a major pain for any future changes. It would've
> > been more feasible if links wasn't a thing. Though, none of it is
> > my problem anymore, and I'm not insisting.
>
> Hmm yes, was actually pondering this last night as well and was going
> to take a closer look today as I have a flight coming up. I'll leave
> them in there for now just to see if syzbot finds anything, and take
> that closer look and see if it's salvageable for now or if we just need
> a new revised take on this.

Hi Jens,
I'd love to understand the race condition concerns you have about not
holding the uring_lock during submission and task work. My limited
mental model of io_uring is that the io_ring_ctx is only accessed in
the context of the task that submitted work to it (during the
io_uring_enter() syscall or task work) or from some interrupt context
that reports a completed operation. Since it's not possible to block
on a mutex in interrupt context, the uring_lock couldn't be used to
synchronize anything running in interrupt context. And then all other
work would be running in the context of the single issuer task, which
couldn't race with itself. Perhaps io_uring worker threads complicate
this picture?
No urgency on this, but I would love to find a way forward here.
Acquiring and releasing the uring_lock is one of the single hottest
CPU instructions in some of our workloads even though each mutex is
accessed only on one CPU. If  uring_lock is necessary to prevent some
other critical sections from racing, perhaps there's an alternate way
to synchronize them (e.g. by deferring them to task work). Or if the
racing sections are specific to certain uses of io_uring, maybe we
could add a setup flag allowing an io_uring user to indicate that it
won't use those features.

Thanks,
Caleb

Powered by blists - more mailing lists