Re: [PATCH RFC 5.13] io_uring: add IORING_REGISTER_PRIORITY

[Jens Axboe <axboe@kernel.dk>]

On 5/7/21 9:19 AM, Pavel Begunkov wrote:
> On 5/6/21 8:20 PM, Hao Xu wrote:
>> 在 2021/5/7 上午1:10, Jens Axboe 写道:
>>> On 5/6/21 8:33 AM, Hao Xu wrote:
>>>> Users may want a higher priority for sq_thread or io-worker. Provide a
>>>> way to change the nice value(for SCHED_NORMAL) or scheduling policy.
>>>
>>> Silly question - why is this needed for sqpoll? With the threads now
>>> being essentially user threads, why can't we just modify nice and
>>> scheduler class from userspace instead? That should work now. I think
>>> this is especially true for sqpoll where it's persistent, and argument
>>> could be made for the io-wq worker threads that we'd need io_uring
>>> support for that, as they come and go and there's no reliable way to
>>> find and tweak the thread scheduler settings for that particular use
>>> case.
>>>
>>> It may be more convenient to support this through io_uring, and that is
>>> a valid argument. I do think that the better way would then be to simply
>>> pass back the sqpoll pid after ring setup, because then it'd almost be
>>> as simple to do it from the app itself using the regular system call
>>> interfaces for that.
>>>> It's my bad. I didn't realize this until I almost completed the patch,
>> then I looked into io_uring_param, found just __u32 resv[3] can be
>> leveraged. Not sure if it's neccessary to occupy one to do this, so I
>> still sent this patch for comments.
> 
> io_uring_param is not a problem, can be extended.
> 
>>> In summary, I do think this _may_ make sense for the worker threads,
>>> being able to pass in this information and have io-wq worker thread
>>> setup perform the necessary tweaks when a thread is created, but it does
>> I'm working on this(for the io-wq worker), have done part of it.
> 
> I'm not sure the io-wq part makes much sense,
> 
> 1) they are per thread, so an instance not related to some particular
> ring, and so should not be controlled by it. E.g. what if a ring
> has two different rings and sets different schedulers?
> 
> 2) io-wq is slow path in any case, don't think it's worth trinking
> with it.

Right, it's normally slow path, so perhaps you'd want to nice it down
or use a different scheduler class for it. I'm not personally seeing
a strong need, but willing to entertain use cases if valid.

>>> seem a bit silly to add this for sqpoll where it could just as easily be
>>> achieved from the application itself without needing to add this
>> It's beyond my knowledge, correct me if I'm wrong: if we do
>> it from application, we have to search the pid of sq_thread by it's name
>> which is iou-sqp-`sqd->task_pid`, and may be cut off because of
>> TASK_COMM_LEN(would this macro value be possibly changed in the
>> future?). And set_task_comm() is called when sq_thread runs, so there is
>> very small chance(but there is) that set_task_comm() hasn't been called
>> when application try to get the command name of sq_thread. Based on this
>> (if it is not wrong...) I think return pid of sq_thread in io_uring
>> level may be a better choice.
> 
> Right, we may return some id of sqpoll task back in io_uring_param,
> though we need to be careful with namespaces.

Yep

-- 
Jens Axboe