On 2/10/22 12:03 PM, Alexander V. Buev wrote:
>> On 2/10/22 6:08 AM, Alexander V. Buev wrote:
>>> Added new READV_PI/WRITEV_PI operations to io_uring.
>>> Added new pi_addr & pi_len fields to SQE struct.
>>> Added new pi_iter field and IOCB_USE_PI flag to kiocb struct.
>>> Make corresponding corrections to io uring trace event.
>>>
>>> +struct io_rw_pi_state {
>>> + struct iov_iter iter;
>>> + struct iov_iter_state iter_state;
>>> + struct iovec fast_iov[UIO_FASTIOV_PI];
>>> +};
>>> +
>>> +struct io_rw_pi {
>>> + struct io_rw rw;
>>> + struct iovec *pi_iov;
>>> + u32 nr_pi_segs;
>>> + struct io_rw_pi_state *s;
>>> +};
>>
>> One immediate issue I see here is that io_rw_pi is big, and we try very
>> hard to keep the per-command payload to 64-bytes. This would be 88 bytes
>> by my count :-/
>>
>> Do you need everything from io_rw? If not, I'd just make io_rw_pi
>> contain the bits you need and see if you can squeeze it into the
>> existing cacheline.
>
> In short - Yes. Current patch code call existing io_read/io_write functions.
> This functions use io_rw struct information and process this data.
> I wanted to use existing functions but may be this is wrong way in this
> case.
>
> The second problem with request size is that the patch adds pi_iter
> pointer to kiocb struct. This also increase whole request union
> length.
>
> So I can see some (may be possible) solution for this:
>
> 1) do not store whole kiocb struct in request
> and write fully separated io_read/write_pi functions
>
> 2) make special CONFIG_XXX variable and simplify hide this code
> as default
Option 2 really sucks, because then obviously everyone wants their
feature enabled, and then we are back to square one. So never rely on a
config option, if it can be avoided.
I'd like to see what option 1 looks like, that sounds like a far better
solution.
--
Jens Axboe
