Re: [PATCH 2/2] io_uring: introduce non-circular SQ

[Caleb Sander Mateos <csander@purestorage.com>]

On Tue, Oct 14, 2025 at 12:25 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 10/14/25 19:37, Caleb Sander Mateos wrote:
> > On Tue, Oct 14, 2025 at 3:57 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
> ...>> + * SQEs always start at index 0 in the submission ring instead of using a
> >> + * wrap around indexing.
> >> + */
> >> +#define IORING_SETUP_SQ_REWIND         (1U << 19)
> >
> > Keith's mixed-SQE-size patch series is already planning to use this
> > flag: https://lore.kernel.org/io-uring/20251013180011.134131-3-kbusch@meta.com/
>
> I'll rebase it as ususual if that gets merged before.
> >> -       /*
> >> -        * Ensure any loads from the SQEs are done at this point,
> >> -        * since once we write the new head, the application could
> >> -        * write new data to them.
> >> -        */
> >> -       smp_store_release(&rings->sq.head, ctx->cached_sq_head);
> >> +       if (ctx->flags & IORING_SETUP_SQ_REWIND) {
> >> +               ctx->cached_sq_head = 0;
> >
> > The only awkward thing about this interface seems to be if
> > io_submit_sqes() aborts early without submitting all the requested
> > SQEs. Does userspace then need to memmove() the remaining SQEs to the
> > start of the ring? It's certainly an unlikely case but something
> > userspace has to handle because io_alloc_req() can fail for reasons
> > outside its control. Seems like it might simplify the userspace side
> > if cached_sq_head wasn't rewound if not all SQEs were consumed.
> This kind of special rules is what usually makes interfaces a pain to
> work with. What if you want to abort all un-submitted requests
> instead? You can empty the queue, but then the next syscall will
> still start from the middle. Or what if the application wants to
> queue more requests before resubmitting previous ones? There are
> reasons b/c the kernel will need to handle it in a less elegant way
> than it potentially can otherwise. memmove sounds appropriate.

Maybe most convenient would be a way for userspace to pass both a head
and a nr/tail value to the syscall instead of assuming the head is
always 0. But it's probably difficult to modify the existing syscall
interface without an indirection to the head value, which seems to be
a main point of this series. So always resetting to 0 and requiring
userspace to memmove() the remaining SQEs in the rare case that
io_uring_enter() doesn't consume all of them seems like a reasonable
approach.

>
> >> @@ -3678,6 +3687,12 @@ static int io_uring_sanitise_params(struct io_uring_params *p)
> >>   {
> >>          unsigned flags = p->flags;
> >>
> >> +       if (flags & IORING_SETUP_SQ_REWIND) {
> >> +               if ((flags & IORING_SETUP_SQPOLL) ||
> >> +                   !(flags & IORING_SETUP_NO_SQARRAY))
> >
> > Is there a reason IORING_SETUP_NO_SQARRAY is required? It seems like
> > the implementation would work just fine with the SQ indirection ring;
> > the rewind would just apply to the indirection ring instead of the SQE
> > array. The cache hit rate benefit would probably be smaller since many
> > more SQ indirection entries fit in a single cache line, but I don't
> > see a reason to explicitly forbid it.
>
> B/c I don't care about sqarray setups, they are on the way out for soft
> deprecation with liburing defaulting to NO_SQARRAY, and once you try
> to optimise the kernel IORING_SETUP_SQ_REWIND handling it might turn
> out that !NO_SQARRAY is in the way... or not, but you can always allow
> it later while limiting it would break uapi. In short, it's weighting
> chances of (micro) optimisations in the future vs supporting a case
> which is unlikely going to be used.

Fair point, tradeoffs either way.

Best,
Caleb

>
> --
> Pavel Begunkov
>