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
