Re: [PATCH v1 11/30] io_uring/kbuf: return buffer id in buffer selection

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

On Thu, Dec 4, 2025 at 11:22 AM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Wed, Dec 3, 2025 at 1:54 PM Caleb Sander Mateos
> <csander@purestorage.com> wrote:
> >
> > On Tue, Dec 2, 2025 at 4:36 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > Return the id of the selected buffer in io_buffer_select(). This is
> > > needed for kernel-managed buffer rings to later recycle the selected
> > > buffer.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> > >  include/linux/io_uring/cmd.h   | 2 +-
> > >  include/linux/io_uring_types.h | 2 ++
> > >  io_uring/kbuf.c                | 7 +++++--
> > >  3 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> > > index e1a75cfe57d9..dcc95e73f12f 100644
> > > --- a/include/linux/io_uring_types.h
> > > +++ b/include/linux/io_uring_types.h
> > > @@ -109,6 +109,8 @@ struct io_br_sel {
> > >                 void *kaddr;
> > >         };
> > >         ssize_t val;
> > > +       /* id of the selected buffer */
> > > +       unsigned buf_id;
> >
> > Looks like this could be unioned with val? I think val's size can be
> > reduced to an int since only int values are assigned to it.
> >
>
> I'm not sure I see the advantage of this. imo I think it makes the
> interface a bit more confusing, as val also represents the error code
> and is logically its own separate entity from buf_id. I don't see the
> struct io_br_sel being stored anywhere where it seems important to
> save a few bytes, but maybe I'm missing something here.

Yeah, fair enough. Looks like it's only stored on the stack and
returned from functions. Splitting ssize_t val out of the union has
already increased its size beyond 16 bytes, so it was already too
large to fit in a register.

>
> > >  };
> > >
> > >
> > > diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
> > > index 8a94de6e530f..3ecb6494adea 100644
> > > --- a/io_uring/kbuf.c
> > > +++ b/io_uring/kbuf.c
> > > @@ -239,6 +239,7 @@ static struct io_br_sel io_ring_buffer_select(struct io_kiocb *req, size_t *len,
> > >         req->flags |= REQ_F_BUFFER_RING | REQ_F_BUFFERS_COMMIT;
> > >         req->buf_index = buf->bid;
> > >         sel.buf_list = bl;
> > > +       sel.buf_id = buf->bid;
> >
> > This is userspace mapped, so probably should be using READ_ONCE() and
> > reusing the value between req->buf_index and buf->bid? Looks like an
> > existing bug that the reads of buf->bid and buf->addr aren't using
> > READ_ONCE().
>
> Agreed. I think that existing bug should be its own patch. Are you
> planning to submit that fix? since you found that bug, I think you
> should get the authorship for it, but if it's annoying for whatever
> reason and you prefer not to, I can roll that up into this patchset.
> EIther way, I'll rebase v2 on top of that and change this line to
> "sel.buf_id = req->buf_index".

Sure, happy to send a patch.

>
> >
> > >         if (bl->flags & IOBL_KERNEL_MANAGED)
> > >                 sel.kaddr = (void *)buf->addr;
> > >         else
> > > @@ -262,10 +263,12 @@ struct io_br_sel io_buffer_select(struct io_kiocb *req, size_t *len,
> > >
> > >         bl = io_buffer_get_list(ctx, buf_group);
> > >         if (likely(bl)) {
> > > -               if (bl->flags & IOBL_BUF_RING)
> > > +               if (bl->flags & IOBL_BUF_RING) {
> > >                         sel = io_ring_buffer_select(req, len, bl, issue_flags);
> > > -               else
> > > +               } else {
> > >                         sel.addr = io_provided_buffer_select(req, len, bl);
> > > +                       sel.buf_id = req->buf_index;
> >
> > Could this cover both IOBL_BUF_RING and !IOBL_BUF_RING cases to avoid
> > the additional logic in io_ring_buffer_select()?
>
> Ah the next patch (patch 12/30: io_uring/kbuf: export
> io_ring_buffer_select() [1]) exports io_ring_buffer_select() to be
> directly called, so that was my motivation for having that sel.buf_id
> assignment logic in io_ring_buffer_select(). I should have swapped the
> ordering of those 2 patches to make that more clear, I'll do that for
> v2.

Makes sense. No need to switch the order of the patches.

Best,
Caleb