Re: [PATCH v5 2/2] io_uring: avoid unnecessary io_wq_work copy for fast poll feature

[Jens Axboe <axboe@kernel.dk>]

On 6/7/20 2:57 PM, Pavel Begunkov wrote:
> On 07/06/2020 23:36, Pavel Begunkov wrote:
>> On 07/06/2020 18:02, Jens Axboe wrote:
>>> On 6/3/20 7:46 AM, Pavel Begunkov wrote:
>>>> On 02/06/2020 04:16, Xiaoguang Wang wrote:
>>>>> hi Jens, Pavel,
>>>>>
>>>>> Will you have a look at this V5 version? Or we hold on this patchset, and
>>>>> do the refactoring work related io_wq_work firstly.
>>>>
>>>> It's entirely up to Jens, but frankly, I think it'll bring more bugs than
>>>> merits in the current state of things.
>>>
>>> Well, I'd really like to reduce the overhead where we can, particularly
>>> when the overhead just exists to cater to the slow path.
>>>
>>> Planning on taking the next week off and not do too much, but I'll see
>>> if I can get some testing in with the current patches.
>>>
>>
>> I just think it should not be done at expense of robustness.
>>
>> e.g. instead of having tons of if's around ->func, we can get rid of
>> it and issue everything with io_wq_submit_work(). And there are plenty
>> of pros of doing that:
>> - freeing some space in io_kiocb (in req.work in particular)
>> - removing much of stuff with nice negative diffstat
>> - helping this series
>> - even safer than now -- can't be screwed with memcpy(req).
>>
>> Extra switch-lookup in io-wq shouldn't even be noticeable considering
>> punting overhead. And even though io-wq loses some flexibility, as for
>> me that's fine as long as there is only 1 user.
> 
> How about diff below? if split and cooked properly.
> 3 files changed, 73 insertions(+), 164 deletions(-)
> 
> 
> @@ -94,9 +93,9 @@ struct io_wq_work {
>  	pid_t task_pid;
>  };
>  
> -#define INIT_IO_WORK(work, _func)				\
> +#define INIT_IO_WORK(work)					\
>  	do {							\
> -		*(work) = (struct io_wq_work){ .func = _func };	\
> +		*(work) = (struct io_wq_work){};		\
>  	} while (0)						\
>  

Would be nice to optimize this one, it's a lot of clearing for something
we'll generally not use at all in the fast path. Or at least keep it
only for before when we submit the work for async execution.

From a quick look at this, otherwise looks great! Please do split and
submit this.

-- 
Jens Axboe