public inbox for [email protected]
 help / color / mirror / Atom feed
From: Gabriel Krisman Bertazi <[email protected]>
To: Pavel Begunkov <[email protected]>
Cc: [email protected],  [email protected],  [email protected]
Subject: Re: [PATCH RFC 0/9] Launching processes with io_uring
Date: Fri, 13 Dec 2024 15:13:36 -0500	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]> (Pavel Begunkov's message of "Wed, 11 Dec 2024 14:02:14 +0000")


Hi Pavel,

Pavel Begunkov <[email protected]> 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.

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

-- 
Gabriel Krisman Bertazi

  parent reply	other threads:[~2024-12-13 20:13 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-12-09 23:43 [PATCH RFC 0/9] Launching processes with io_uring Gabriel Krisman Bertazi
2024-12-09 23:43 ` [PATCH RFC 1/9] io_uring: Drop __io_req_find_next_prep Gabriel Krisman Bertazi
2024-12-09 23:43 ` [PATCH RFC 2/9] io_uring: Expose failed request helper in internal header Gabriel Krisman Bertazi
2024-12-09 23:43 ` [PATCH RFC 3/9] kernel/fork: Don't inherit PF_USER_WORKER from parent Gabriel Krisman Bertazi
2024-12-09 23:43 ` [PATCH RFC 4/9] fs/exec: Expose do_execveat symbol Gabriel Krisman Bertazi
2024-12-09 23:43 ` [PATCH RFC 5/9] kernel/fork: Add helper to fork from io_uring Gabriel Krisman Bertazi
2024-12-09 23:43 ` [PATCH RFC 6/9] io_uring: Let commands run with current credentials Gabriel Krisman Bertazi
2024-12-11 14:48   ` Pavel Begunkov
2024-12-09 23:43 ` [PATCH RFC 7/9] io_uring: Introduce IORING_OP_CLONE Gabriel Krisman Bertazi
2024-12-11 13:37   ` Pavel Begunkov
2024-12-11 17:26     ` Josh Triplett
2024-12-17 11:03       ` Pavel Begunkov
2024-12-17 19:14         ` Josh Triplett
2024-12-09 23:43 ` [PATCH RFC 8/9] io_uring: Let ->issue know if it was called from spawn thread Gabriel Krisman Bertazi
2024-12-09 23:43 ` [PATCH RFC 9/9] io_uring: Introduce IORING_OP_EXEC command Gabriel Krisman Bertazi
2024-12-10 21:01   ` Josh Triplett
2024-12-10 21:10 ` [PATCH RFC 0/9] Launching processes with io_uring Josh Triplett
2024-12-11 14:02 ` Pavel Begunkov
2024-12-11 17:34   ` Josh Triplett
2024-12-13 20:13   ` Gabriel Krisman Bertazi [this message]
2024-12-17 16:10     ` Pavel Begunkov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox