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

[Caleb Sander Mateos <csander@purestorage.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.

Yeah, the compiler can make a guess at the likely branch direction,
but it's not magic. Anecdotally, early return paths generally seem to
be inferred to be unlikely and if branches seemed to be inferred as
likely (and else branches therefore as unlikely). That's a good
starting point, but a programmer may well have more context to know
which direction branches are likely to go. Additionally, explicit
likely()/unlikely() annotations seem to encourage compilers to
optimize more aggressively for the likely path (e.g. splitting the
unlikely path into a separate function). I guess compilers want to
hedge their bets if they're relying on heuristics to predict the
branch direction.
The performance benefit of each likely()/unlikely() is generally
small, so I tend to agree adding them is usually a premature
optimization. But it can certainly be helpful to improve the code
generation of a hot path. And it's used widely throughout io_uring due
to the focus on maximizing performance.

>
> 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.

And that's the case here; the if condition is only false in the error
case where buffer_group doesn't specify a valid kernel buf ring.

>
> 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.

Not sure I agree about this distinction between likely() and
unlikely() in general. For example, it can make sense to annotate an
early return path with likely() to override the compiler's heuristic
that early returns are unlikely.

>
> 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.

Yes, this is probably the biggest pitfall of likely()/unlikely(). The
additional macro call also makes the code a bit harder for a human to
parse. Commit 4ec703ec0c38 ("io_uring: fix incorrect unlikely() usage
in io_waitid_prep()") is a good example of both these problems.

>
> 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).

It can absolutely make sense to use likely()/unlikely() to annotate
individual components of an && or || expression, not just a full
if/while/for/ternary condition. Since && and || are short-circuiting
operators, the compiler will often emit a branch after each boolean
operand. (For example, absent optimizations, if (a && b) ... is
translated into if (a) { if (b) ... }.)
But you're totally correct that likely(A && B) implies likely(A) &&
likely(B), so there's no need to separately annotate each of the
operands to the &&s here.

Best,
Caleb