From: Stefan Metzmacher <[email protected]>
To: Linus Torvalds <[email protected]>,
Thomas Gleixner <[email protected]>
Cc: Andy Lutomirski <[email protected]>, Jens Axboe <[email protected]>,
Linux Kernel Mailing List <[email protected]>,
io-uring <[email protected]>,
the arch/x86 maintainers <[email protected]>
Subject: Re: [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es' registers for io_threads
Date: Tue, 4 May 2021 01:27:56 +0200 [thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHk-=wi4Dyg_Z70J_hJbtFLPQDG+Zx3dP2jB5QrOdZC6W6j4Gw@mail.gmail.com>
Am 04.05.21 um 01:16 schrieb Linus Torvalds:
> On Mon, May 3, 2021 at 3:56 PM Thomas Gleixner <[email protected]> wrote:
>>
>> It's all fine that we have lots of blurb about GDB, but there is no
>> reasoning why this does not affect regular kernel threads which take the
>> same code path.
>
> Actual kernel threads don't get attached to by ptrace.
>
>> This is a half setup user space thread which is assumed to behave like a
>> regular kernel thread, but is this assumption actually true?
>
> No, no.
>
> It's a *fully set up USER thread*.
>
> Those IO threads used to be kernel threads. That didn't work out for
> the reasons already mentioned earlier.
>
> These days they really are fully regular user threads, they just don't
> return to user space because they continue to do the IO work that they
> were created for.
>
> Maybe instead of Stefan's patch, we could do something like this:
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 43cbfc84153a..890f3992e781 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -156,7 +156,7 @@ int copy_thread(unsigned long clone_flags,
> unsigned long sp, unsigned long arg,
> #endif
>
> /* Kernel thread ? */
> - if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
> + if (unlikely(p->flags & PF_KTHREAD)) {
> memset(childregs, 0, sizeof(struct pt_regs));
> kthread_frame_init(frame, sp, arg);
> return 0;
> @@ -168,6 +168,17 @@ int copy_thread(unsigned long clone_flags,
> unsigned long sp, unsigned long arg,
> if (sp)
> childregs->sp = sp;
>
> + /*
> + * An IO thread is a user space thread, but it doesn't
> + * return to ret_after_fork(), it does the same kernel
> + * frame setup to return to a kernel function that
> + * a kernel thread does.
> + */
> + if (unlikely(p->flags & PF_IO_WORKER)) {
> + kthread_frame_init(frame, sp, arg);
> + return 0;
> + }
> +
> #ifdef CONFIG_X86_32
> task_user_gs(p) = get_user_gs(current_pt_regs());
> #endif
>
> does that clarify things and make people happier?
>
> Maybe the compiler might even notice that the
>
> kthread_frame_init(frame, sp, arg);
> return 0;
>
> part is common code and then it will result in less generated code too.
>
> NOTE! The above is - as usual - COMPLETELY UNTESTED. It looks obvious
> enough, and it builds cleanly. But that's all I'm going to guarantee.
I think I also tested something similar, see:
https://git.samba.org/?p=metze/linux/wip.git;a=commitdiff;h=82fcee2774add04fbc0e4755c405e6c0b7467e3a
If I remember correctly gdb showed bogus addresses for the backtraces of the io_threads,
as some regs where not cleared.
The patch I posted shows this instead:
Thread 2 (LWP 8744):
#0 0x0000000000000000 in ?? ()
Backtrace stopped: Cannot access memory at address 0x0
I think that's a saner behavior.
However splitting the if statements might be a good idea to make things
more clear.
Thanks discussing this again!
metze
next prev parent reply other threads:[~2021-05-03 23:28 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <[email protected]>
2021-05-03 16:05 ` [PATCH] io_thread/x86: don't reset 'cs', 'ss', 'ds' and 'es' registers for io_threads Andy Lutomirski
2021-05-03 19:14 ` Linus Torvalds
2021-05-03 20:15 ` Andy Lutomirski
2021-05-03 20:21 ` Andy Lutomirski
2021-05-03 20:37 ` Linus Torvalds
2021-05-03 21:26 ` Jens Axboe
2021-05-03 21:49 ` Andy Lutomirski
2021-05-03 22:08 ` Linus Torvalds
2021-05-03 22:56 ` Thomas Gleixner
2021-05-03 23:15 ` Andy Lutomirski
2021-05-03 23:16 ` Linus Torvalds
2021-05-03 23:19 ` Linus Torvalds
2021-05-03 23:27 ` Stefan Metzmacher [this message]
2021-05-03 23:48 ` Linus Torvalds
2021-05-04 2:50 ` Jens Axboe
2021-05-04 11:39 ` Stefan Metzmacher
2021-05-04 15:53 ` Linus Torvalds
2021-05-12 4:24 ` Olivier Langlois
2021-05-12 17:44 ` Linus Torvalds
2021-05-12 20:55 ` Jens Axboe
2021-05-20 4:13 ` Olivier Langlois
2021-05-21 7:31 ` Olivier Langlois
2021-05-25 19:39 ` Olivier Langlois
2021-05-25 19:45 ` Olivier Langlois
2021-05-25 19:52 ` Jens Axboe
2021-05-25 20:23 ` Linus Torvalds
2021-05-04 8:22 ` David Laight
2021-05-04 0:01 ` Andy Lutomirski
2021-05-04 8:39 ` Peter Zijlstra
2021-05-04 15:35 ` Borislav Petkov
2021-05-04 15:55 ` Simon Marchi
2021-05-05 11:29 ` Stefan Metzmacher
2021-05-05 21:59 ` Simon Marchi
2021-05-05 22:11 ` Andy Lutomirski
2021-05-05 23:12 ` Borislav Petkov
2021-05-05 23:22 ` Andy Lutomirski
2021-05-06 1:04 ` Simon Marchi
2021-05-06 15:11 ` Andy Lutomirski
2021-05-06 9:47 ` David Laight
2021-05-06 9:53 ` David Laight
2021-05-05 22:21 ` Stefan Metzmacher
2021-05-05 23:15 ` Simon Marchi
2021-04-11 15:27 Stefan Metzmacher
2021-04-14 21:28 ` Stefan Metzmacher
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] \
[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