Re: [PATCH v3 06/25] io_uring/kbuf: add buffer ring pinning/unpinning

[Joanne Koong <joannelkoong@gmail.com>]

On Tue, Dec 30, 2025 at 9:54 AM Gabriel Krisman Bertazi <krisman@suse.de> wrote:
>
> Joanne Koong <joannelkoong@gmail.com> writes:
>
> > On Mon, Dec 29, 2025 at 1:07 PM Gabriel Krisman Bertazi <krisman@suse.de> wrote:
> >
> >>
> >> Joanne Koong <joannelkoong@gmail.com> writes:
> >>
> >> > +int io_kbuf_ring_pin(struct io_kiocb *req, unsigned buf_group,
> >> > +                  unsigned issue_flags, struct io_buffer_list **bl)
> >> > +{
> >> > +     struct io_buffer_list *buffer_list;
> >> > +     struct io_ring_ctx *ctx = req->ctx;
> >> > +     int ret = -EINVAL;
> >> > +
> >> > +     io_ring_submit_lock(ctx, issue_flags);
> >> > +
> >> > +     buffer_list = io_buffer_get_list(ctx, buf_group);
> >> > +     if (likely(buffer_list) && likely(buffer_list->flags & IOBL_BUF_RING)) {
> >>
> >> FWIW, the likely construct is unnecessary here. At least, it should
> >> encompass the entire expression:
> >>
> >>     if (likely(buffer_list && buffer_list->flags & IOBL_BUF_RING))
> >>
> >> But you can just drop it.
> >
> > I see, thanks. Could you explain when likelys/unlikelys should be used
> > vs not? It's unclear to me when they need to be included vs can be
> > dropped. I see some other io-uring code use likely() for similar-ish
> > logic, but is the idea that it's unnecessary because the compiler
> > already infers it?
>
> likely/unlikely help the compiler decide whether it should reverse the
> jump to optimize branch prediction and code spacial locality for icache.
> The compiler is usually great in figuring it out by itself and, in
> general, these should only be used after profilings shows the specific
> jump is problematic, or when you know the jump will or will not be taken
> almost every time.  The compiler decision depends on heuristics (which I
> guess considers the leg size and favors the if leg), but it usually gets
> it right.
>
> One obvious case where *unlikely* is useful is to handle error paths.
> The logic behind it is that the error path is obviously not the
> hot-path, so a branch misprediction or a cache miss in that path is
> just fine.
>
> The usage of likely is more rare, and some usages are just cargo-cult.
> Here you could use it, as the hot path is definitely the if leg.  But
> if you look at the generated code, it most likely doesn't make any
> difference, because gcc is smart enough to handle it.
>
> A problem arises when likely/unlikely are used improperly, or the code
> changes and the frequency when each leg is taken changes.  Now the
> likely/unlikely is introducing mispredictions the compiler could have
> avoided and harming performance.
>
> I wasn't gonna comment in the review, since the likely() seems harmless
> in your patch.  But what got my attention was that each separate
> expression was under a single likely() expression.  I don't think that
> makes much sense, since the hint is useful for the placement of the
> if/else legs, it should encompass the whole condition.  That's how it is
> used almost anywhere else in the kernel (there are a few occurrences
> drivers/scsi/ that also look a bit fishy, IMO).

That makes sense. Thanks for the elaboration.
>
> --
> Gabriel Krisman Bertazi