Re: [PATCH RFC 5.13] io_uring: add IORING_REGISTER_PRIORITY

[Pavel Begunkov <asml.silence@gmail.com>]

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.

>> 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.

-- 
Pavel Begunkov