Re: Deduplicate io_*_prep calls?

[Jens Axboe <axboe@kernel.dk>]

On 2/23/20 8:33 PM, Andres Freund wrote:
> Hi,
> 
> On 2020-02-23 20:17:45 -0700, Jens Axboe wrote:
>>> that seems a bit unnecessary. How about breaking that out into a
>>> separate function?  I can write up a patch, just didn't want to do so if
>>> there's a reason for the current split.
>>>
>>>
>>> Alternatively it'd could all be just be dispatches via io_op_defs, but
>>> that'd be a bigger change with potential performance implications. And
>>> it'd benefit from prior deduplication anyway.
>>
>> The reason for the split is that if we defer a request, it has to be
>> prepared up front. If the request has been deferred, then the
>> io_issue_sqe() invocation has sqe == NULL. Hence we only run the prep
>> handler once, and read the sqe just once.
> 
>> This could of course be compacted with some indirect function calls, but
>> I didn't want to pay the overhead of doing so... The downside is that
>> the code is a bit bigger.
> 
> Shouldn't need indirect function calls? At most the switch() would be
> duplicated, if the compiler can't optimize it away (ok, that's an
> indirect jump...).  I was just thinking of moving the io_*_prep() switch
> into something like io_prep_sqe().
> 
> io_req_defer_prep() would basically move its switch into io_prep_sqe
> (but not touch the rest of its code). io_issue_sqe() would have
> 
> if (sqe) {
>     ret = io_prep_sqe(req, sqe, force_nonblock);
>     if (ret != 0)
>         return ret;
> }
> 
> at the start.
> 
> Even if the added switch can't be optimized away from io_issue_sqe(),
> the code for all the branches inside the opcode cases isn't free
> either...

The fast case is not being deferred, that's by far the common (and hot)
case, which means io_issue() is called with sqe != NULL. My worry is
that by moving it into a prep helper, the compiler isn't smart enough to
not make that basically two switches. Feel free to prove me wrong, I'd
love to reduce it ;-)

-- 
Jens Axboe