On 12/13/24 20:13, Gabriel Krisman Bertazi wrote:
Hi Pavel,
Pavel Begunkov <asml.silence@xxxxxxxxx> writes:
On 12/9/24 23:43, Gabriel Krisman Bertazi wrote:
Sorry to say but the series is rather concerning.
1) It creates a special path that tries to mimick the core
path, but not without a bunch of troubles and in quite a
special way.
I fully agree this is one of the main problem with the series. I'm
interested in how we can merge this implementation into the existing
io_uring paths. My idea, which I hinted in the cover letter, is to have
a flavor of io-wq that executes one linked sequence and then terminates.
When a work is queued there, the newly spawned worker thread will live
only until the end of that link. This wq is only used to execute the
link following a IORING_OP_CLONE and the user can pass CLONE_ flags to
determine how it is created. This allows the user to create a detached
file descriptor table in the worker thread, for instance.
It'd allows us to reuse the dispatching infrastructure of io-wq, hide
io_uring internals from the OP_CLONE implementation, and
enable, if I understand correctly, the workarounds to execute
task_works. We'd need to ensure nothing from the link gets
executed outside of this context.
One problem with io-wq is that it's not guaranteed that it's able to
serve all types of requests. Though it's limited to multishots atm,
which you might not need, but the situation might change. And there
is no guarantee that the request is completed by the time it returns
from ->issue(), it might even change hands from inside the callback
via task_work or by any other mean.
It also sounds like you want the cloned task to be a normal
io_uring submmiter in terms of infra even though it can't
initiate a syscall, which also sounds a bit like an SQPOLL task.
And do we really need to execute everything from the new task
context, or ops can take a task as an argument and run whenever
while final exec could be special cased inside the callback?
2) There would be a special set of ops that can only be run
from that special path.
There are problems with cancellations and timeouts, that I'd expect to
be more solvable when reusing the io-wq code. But this task is
executing from a cloned context, so we have a copy of the parent
context, and share the same memory map. It should be safe to do IO to
open file descriptors, wake futexes and pretty much anything that
doesn't touch io_uring itself. There are oddities, like the fact the fd
table is split from the parent task while the io_uring direct
descriptors are shared. That needs to be handled with more sane
semantics.
At this point it raises a question why it even needs io_uring
infra? I don't think it's really helping you. E.g. why not do it
as a list of operation in a custom format instead of links? That
can be run by a single io_uring request or can even be a normal
syscall.
struct clone_op ops = { { CLONE },
{ SET_CRED, cred_id }, ...,
{ EXEC, path }};
Makes me wonder about a different ways of handling. E.g. why should
it be run in the created task context (apart from final exec)? Can
requests be run as normal by the original task, each will take the
half created and not yet launched task as a parameter (in some form),
modify it, and the final exec would launch it?
A single operation would be a very complex operation doing many things
at once , and much less flexible. This approach is flexible: you
can combine any (in theory) io_uring operation to obtain the desired
behavior.
Ok. And links are not flexible enough for it either. Think of
error handling, passing results from one request to another and
more complex relations. Unless chains are supposed to be very
short and simple, it'd need to be able to return back to user
space (the one issuing requests) for error handling.
--
Pavel Begunkov
