public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: Linus Torvalds <[email protected]>,
	Andy Lutomirski <[email protected]>
Cc: Thomas Gleixner <[email protected]>,
	Stefan Metzmacher <[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: Mon, 3 May 2021 15:26:10 -0600	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHk-=wgo6XEz3VQ9ntqzWLR3-hm1YXrXUz4_heDs4wcLe9NYvA@mail.gmail.com>

On 5/3/21 2:37 PM, Linus Torvalds wrote:
> On Mon, May 3, 2021 at 1:15 PM Andy Lutomirski <[email protected]> wrote:
>>
>> On Mon, May 3, 2021 at 12:15 PM Linus Torvalds
>> <[email protected]> wrote:
>>> So generally, the IO threads are now 100% normal threads - it's
>>> literally just that they never return to user space because they are
>>> always just doing the IO offload on the kernel side.
>>>
>>> That part is lovely, but part of the "100% IO threads" really is that
>>> they share the signal struct too, which in turn means that they very
>>> much show up as normal threads. Again, not a problem: they really
>>> _are_ normal threads for all intents and purposes.
>>
>> I'm a bit confused, though.  All the ptrace register access (AFAICS)
>> goes through ptrace_check_attach(), which should wait until the tracee
>> is stopped.  Does the io_uring thread now stop in response to ptrace
>> stop requests?
> 
> Yup. They really are 100% regular threads. Things like ^Z and friends
> also stop them now, and the freezer freezes them etc.
> 
> And making PTRACE_ATTACH fail just causes gdb to fail.
> 
>> Fair enough.  But I would really, really rather that gdb starts fixing
>> its amazingly broken assumptions about bitness.
> 
> "Preach it, Brother"

That's actually what the original code did, and the "only" problem with
it was that gdb shits itself and just go into an infinite loop trying to
attach. And yes, that's most certainly a gdb bug, and we entertained a
few options for making that work. One was hiding the threads, but nobody
(myself included) liked that, because then we're special casing
something again, and for no other reason than gdb is buggy.

On principle, I think it's arguably the right change to just -EINVAL the
attach. However, a part of me also finds it very annoying that anyone
attempting to debug any program that uses io_uring will not be able to
do so with a buggy gdb. That's regardless of whether or not you want to
look at the io threads or not, or even if you don't care about debugging
the io_uring side of things. And I'm assuming this will take a while to
get fixed, and then even longer to make its way back to distros.

So... You should just make the call. At least then I can just tell
people that Linus made that decision :-)

>>>    So I think Stefan's patch is reasonable, if not pretty. Literally
>>> becasue of that "make these threads look even more normal"
>>
>> I think it's reasonable except for the bit about copying the segment
>> regs.  Can we hardcode __USER_CS, etc, and, when gdb crashes or
>> otherwise malfunctions for compat programs, we can say that gdb needs
>> to stop sucking.
> 
> So that was actually my initial suggestion. Stefan really does seem to
> care about compat programs.
> 
> Any "gdb breaks" would be good to motivate people to fix gdb, but the
> thing is, presumably nobody actually wants to touch gdb with a ten
> foot pole.
> 
> And a "let's break gdb to encourage people to fix it" only works if
> people actually _do_ fit it. Which doesn't seem to be happening.
> 
> Two lines of kernel code seems to be the better option than hoping for
> gdb to be fixed.

As far as I'm concerned, gdb works "well enough" with io threads as it
stands. Yes, it'll complain a bit, but nothing that prevents you from
attaching to a progrem. If we -EINVAL, then gdb will become useless for
debugging a program that uses io_uring. And I'm not holding my breath
while someone fixes that.

-- 
Jens Axboe


  reply	other threads:[~2021-05-03 21:26 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 [this message]
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
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