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

[Gabriel Krisman Bertazi <krisman@suse.de>]

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

-- 
Gabriel Krisman Bertazi