Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1636343 > unrolled thread

[PATCH 7/7] DWARF: add the config option

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-05 14:30 +0200
    Re: [PATCH 7/7] DWARF: add the config option Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-05 22:00 +0200
      Re: [PATCH 7/7] DWARF: add the config option Ingo Molnar <mingo@kernel.org> - 2017-05-06 09:30 +0200
        Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 09:50 +0200
      Re: [PATCH 7/7] DWARF: add the config option Jiri Kosina <jikos@kernel.org> - 2017-05-06 16:30 +0200
      Re: [PATCH 7/7] DWARF: add the config option Ingo Molnar <mingo@kernel.org> - 2017-05-07 23:50 +0200
        Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-07 23:50 +0200
          Re: [PATCH 7/7] DWARF: add the config option Vojtech Pavlik <vojtech@suse.com> - 2017-05-08 10:20 +0200
            Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-08 15:20 +0200
        Re: [PATCH 7/7] DWARF: add the config option hpa@zytor.com - 2017-05-08 00:10 +0200
      Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-07 23:50 +0200
        Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@amacapital.net> - 2017-05-08 07:40 +0200
          Re: [PATCH 7/7] DWARF: add the config option Ingo Molnar <mingo@kernel.org> - 2017-05-08 08:20 +0200
          Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-08 16:50 +0200
            Re: [PATCH 7/7] DWARF: add the config option hpa@zytor.com - 2017-05-08 21:10 +0200
        Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@kernel.org> - 2017-05-09 02:30 +0200
          Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 03:40 +0200
            Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@kernel.org> - 2017-05-09 04:40 +0200
              Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 05:40 +0200
                Re: [PATCH 7/7] DWARF: add the config option hpa@zytor.com - 2017-05-09 12:10 +0200
                  Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 17:00 +0200
                    Re: [PATCH 7/7] DWARF: add the config option "H.J. Lu" <hjl.tools@gmail.com> - 2017-05-09 18:50 +0200
                  Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 10:20 +0200
                    Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-10 15:10 +0200
                      Re: [PATCH 7/7] DWARF: add the config option "H.J. Lu" <hjl.tools@gmail.com> - 2017-05-10 18:30 +0200
        Re: [PATCH 7/7] DWARF: add the config option Jiri Kosina <jikos@kernel.org> - 2017-05-09 20:50 +0200
          Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 21:30 +0200
            Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 10:40 +0200
              Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-10 15:20 +0200
      Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 09:50 +0200
        Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-10 14:50 +0200
          Re: [PATCH 7/7] DWARF: add the config option Jiri Slaby <jslaby@suse.cz> - 2017-05-10 14:50 +0200
        Re: [PATCH 7/7] DWARF: add the config option Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-10 20:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1638175

On Tue, May 09, 2017 at 03:00:45AM -0700, hpa@zytor.com wrote:
> I'm, ahem, highly skeptical to creating our own unwinding data format
> unless there is *documented, supported, and tested* way to force the
> compiler to *automatically* fall back to frame pointers any time there
> may be complexity involved, which at a very minimum includes any kind
> of data-dependent manipulation of the stack pointer.

That would be nice.  But isn't falling back to a frame pointer (or
another callee-saved reg or a stack location) already needed in such
cases?  Otherwise how could DWARF unwinding work?

> Otherwise you will have to fail the kernel build when your static tool
> runs into instruction sequences it can't deal with, but the compiler
> may generate them - boom.

Failing the build is harsh, we could just warn about it and skip the
data for the affected function(s).

BTW, there is another option.  Instead of generating the data from
scratch, we could just convert gcc's DWARF CFI to the format we need.

However that wouldn't solve the problems we have with the holes and
inaccuracies in DWARF from our hand-annotated asm, inline asm, and
special sections (extable, alternatives, etc).  We'd still have to rely
on objtool for that, so we'd still be in the same boat of needing
objtool to be able to follow gcc code paths.

So yes, it sucks that objtool needs to work for unwinding to work.  But
if we want decent DWARF-esque unwinding, I don't see any way around
that due to the low-level nature of the kernel.

> Worse, your tool will not even recognize the problem and you're in a
> worse place than when you started.

We could have a runtime NMI-based stack checker which ensures it can
always unwind to the bottom of the stack.  Over time this would
hopefully provide full validation of the unwinder data and
functionality.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1638280

On Tue, May 9, 2017 at 7:58 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Tue, May 09, 2017 at 03:00:45AM -0700, hpa@zytor.com wrote:
>> I'm, ahem, highly skeptical to creating our own unwinding data format
>> unless there is *documented, supported, and tested* way to force the
>> compiler to *automatically* fall back to frame pointers any time there
>> may be complexity involved, which at a very minimum includes any kind
>> of data-dependent manipulation of the stack pointer.
>
> That would be nice.  But isn't falling back to a frame pointer (or
> another callee-saved reg or a stack location) already needed in such
> cases?  Otherwise how could DWARF unwinding work?
>
>> Otherwise you will have to fail the kernel build when your static tool
>> runs into instruction sequences it can't deal with, but the compiler
>> may generate them - boom.
>
> Failing the build is harsh, we could just warn about it and skip the
> data for the affected function(s).
>
> BTW, there is another option.  Instead of generating the data from
> scratch, we could just convert gcc's DWARF CFI to the format we need.
>
> However that wouldn't solve the problems we have with the holes and
> inaccuracies in DWARF from our hand-annotated asm, inline asm, and
> special sections (extable, alternatives, etc).  We'd still have to rely
> on objtool for that, so we'd still be in the same boat of needing
> objtool to be able to follow gcc code paths.

CFI directives are documented in GNU assembler manual.  They
store unwind info in .eh_frame section.  They work well with assembly
codes in glibc.  But I don't know how well they work with kernel unwind.

> So yes, it sucks that objtool needs to work for unwinding to work.  But
> if we want decent DWARF-esque unwinding, I don't see any way around
> that due to the low-level nature of the kernel.
>
>> Worse, your tool will not even recognize the problem and you're in a
>> worse place than when you started.
>
> We could have a runtime NMI-based stack checker which ensures it can
> always unwind to the bottom of the stack.  Over time this would
> hopefully provide full validation of the unwinder data and
> functionality.
>
> --
> Josh



-- 
H.J.

[toc] | [prev] | [next] | [standalone]

#1638645

On 05/09/2017, 12:00 PM, hpa@zytor.com wrote:
> As far as I understand, the .eh_frame section is supposed to contain the subset of the DWARF bytecode needed to do a stack unwind when an exception is thrown, whereas the .debug* sections contain the full DWARF data a debugger might want.  Thus .eh_frame is mapped into the runtime process while .debug* [usually?] is not.  .debug* can easily be 10x larger than the executable text segments.

As it currently stands, the (same) data is generated either to
.eh_frame, or to .debug_frame. Depending if DWARF_UNWINDER is turned on,
or off, respectively. vdso has the same data in both, always.

> Assembly language routines become problematic no matter what you do unless you restrict the way the assembly can be written.  Experience has shown us that hand-maintaining annotations in assembly code is doomed to failure, and in many cases it isn't even clear to even experienced programmers how to do it.

Sure, manual annotations of assembly will be avoided as much as
possible. We have to rely objtool to generate them in most cases.

>  [H.J.: is the .cfi_* operations set even documented anywhere in a way that non-compiler-writers can comprehend it?]

Until now, I always looked into as manual:
$ pinfo --node='CFI directives' as

> I'm, ahem, highly skeptical to creating our own unwinding data format unless there is *documented, supported, and tested* way to force the compiler to *automatically* fall back to frame pointers any time there may be complexity involved,

I second this. Inventing a new format like this mostly ends up with
using the standard one after several iterations. One cannot think of all
the consequences and needs while proposing a new one.

The memory footprint is ~2M for average vmlinux. And people who need to
access:
* either need it frequently -- those do not need performance (LOCKDEP,
KASAN, or other debug builds)
* or are in the middle of WARNING, BUG, crash, panic or such and this is
not that often...

And we would need *both*. The limited proprietary one in some sort of
.kernel_eh_frame, and DWARF cfis in .debug_frame for tools like crash,
gdb and so on.

And yes, the DWARF unwinder falls back to FP if they are available (see
function dw_fp_fallback).

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1638802

On Wed, May 10, 2017 at 10:15:09AM +0200, Jiri Slaby wrote:
> > I'm, ahem, highly skeptical to creating our own unwinding data
> > format unless there is *documented, supported, and tested* way to
> > force the compiler to *automatically* fall back to frame pointers
> > any time there may be complexity involved,
> 
> I second this. Inventing a new format like this mostly ends up with
> using the standard one after several iterations. One cannot think of all
> the consequences and needs while proposing a new one.
> 
> The memory footprint is ~2M for average vmlinux. And people who need to
> access:
> * either need it frequently -- those do not need performance (LOCKDEP,
> KASAN, or other debug builds)
> * or are in the middle of WARNING, BUG, crash, panic or such and this is
> not that often...
> 
> And we would need *both*. The limited proprietary one in some sort of
> .kernel_eh_frame, and DWARF cfis in .debug_frame for tools like crash,
> gdb and so on.

I don't think so.  DWARF CFI is optimized for size.  My proposal is to
take the same data (or some subset of it) and reformat it to optimize
for simplicity.

If, for some reason, we ended up needing *all* the original DWARF data,
we could still have it in the simpler format.  In that case it might end
up being 8M instead of 2M :-) But I don't see that being possible.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1638952

On Wed, May 10, 2017 at 6:09 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Wed, May 10, 2017 at 10:15:09AM +0200, Jiri Slaby wrote:
>> > I'm, ahem, highly skeptical to creating our own unwinding data
>> > format unless there is *documented, supported, and tested* way to
>> > force the compiler to *automatically* fall back to frame pointers
>> > any time there may be complexity involved,
>>
>> I second this. Inventing a new format like this mostly ends up with
>> using the standard one after several iterations. One cannot think of all
>> the consequences and needs while proposing a new one.
>>
>> The memory footprint is ~2M for average vmlinux. And people who need to
>> access:
>> * either need it frequently -- those do not need performance (LOCKDEP,
>> KASAN, or other debug builds)
>> * or are in the middle of WARNING, BUG, crash, panic or such and this is
>> not that often...
>>
>> And we would need *both*. The limited proprietary one in some sort of
>> .kernel_eh_frame, and DWARF cfis in .debug_frame for tools like crash,
>> gdb and so on.
>
> I don't think so.  DWARF CFI is optimized for size.  My proposal is to
> take the same data (or some subset of it) and reformat it to optimize
> for simplicity.
>
> If, for some reason, we ended up needing *all* the original DWARF data,
> we could still have it in the simpler format.  In that case it might end
> up being 8M instead of 2M :-) But I don't see that being possible.

There is a compact EH for MIPS:

https://github.com/itanium-cxx-abi/cxx-abi/blob/master/MIPSCompactEH.pdf

It can be extended to other targets.

-- 
H.J.

[toc] | [prev] | [next] | [standalone]

#1638346

On Sun, 7 May 2017, Josh Poimboeuf wrote:

> DWARF is great for debuggers.  It helps you find all the registers on 
> the stack, so you can see function arguments and local variables.  All 
> expressed in a nice compact format.
> 
> But that's overkill for unwinders.  We don't need all those registers,
> and the state machine is too complicated.  

OTOH if we make the failures in processing of those "auxiliary" 
information non-fatal (in a sense that it neither causes kernel bug nor 
does it actually corrupt the unwinding process, but the only effect is 
losing "optional" information), having this data available doesn't hurt. 

It's there anyway for builds containing debuginfo, and the information is 
all there so that it can be used by things like gdb or crash, so it seems 
natural to re-use as much as possible of it.

> Unwinders basically only need to know one thing: given an instruction 
> address and a stack pointer, where is the caller's stack frame?

Again, DWARF should be able to give us all of this (including the 
FP-fallback etc). It feels a bit silly to purposedly ignore it and 
reinvent parts of it again, instead of fixing (read: "asking toolchain 
guys to fix") the cases where we actually are not getting the proper data 
in DWARF. That's a win-win at the end of the day.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1638369

On Tue, May 09, 2017 at 08:47:50PM +0200, Jiri Kosina wrote:
> On Sun, 7 May 2017, Josh Poimboeuf wrote:
> 
> > DWARF is great for debuggers.  It helps you find all the registers on 
> > the stack, so you can see function arguments and local variables.  All 
> > expressed in a nice compact format.
> > 
> > But that's overkill for unwinders.  We don't need all those registers,
> > and the state machine is too complicated.  
> 
> OTOH if we make the failures in processing of those "auxiliary" 
> information non-fatal (in a sense that it neither causes kernel bug nor 
> does it actually corrupt the unwinding process, but the only effect is 
> losing "optional" information), having this data available doesn't hurt. 

But it does hurt, in the sense that the complicated format of DWARF CFI
means the unwinder has to jump through a lot more hoops to read it.

> It's there anyway for builds containing debuginfo, and the information is 
> all there so that it can be used by things like gdb or crash, so it seems 
> natural to re-use as much as possible of it.

There's a valid argument to be made that we should start with the DWARF
data instead of creating the new data from scratch.  That might be fine.
Right now I don't have a strong feeling about it either way.

But if we do that, we should still convert the DWARF data to a simple
streamlined format for the in-kernel unwinder, so it can easily be read
by the kernel without having to fire up a DWARF state machine in the
middle of an oops.

And if we wanted it to be reasonably reliable, we'd also need to fix up
the DWARF data somehow before converting it, presumably with objtool.

> > Unwinders basically only need to know one thing: given an instruction 
> > address and a stack pointer, where is the caller's stack frame?
> 
> Again, DWARF should be able to give us all of this (including the 
> FP-fallback etc). It feels a bit silly to purposedly ignore it and 
> reinvent parts of it again, instead of fixing (read: "asking toolchain 
> guys to fix") the cases where we actually are not getting the proper data 
> in DWARF. That's a win-win at the end of the day.

Most of the kernel DWARF issues I've seen aren't caused by toolchain
bugs.  They're caused by the kernel's quirks: asm, inline asm, special
sections.

And anyway, fixing the correctness of the DWARF data is only half the
problem IMO.  The other half of the problem is unwinder complexity.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1638653

On 05/09/2017, 09:22 PM, Josh Poimboeuf wrote:
> On Tue, May 09, 2017 at 08:47:50PM +0200, Jiri Kosina wrote:
>> On Sun, 7 May 2017, Josh Poimboeuf wrote:
>>
>>> DWARF is great for debuggers.  It helps you find all the registers on 
>>> the stack, so you can see function arguments and local variables.  All 
>>> expressed in a nice compact format.
>>>
>>> But that's overkill for unwinders.  We don't need all those registers,
>>> and the state machine is too complicated.  
>>
>> OTOH if we make the failures in processing of those "auxiliary" 
>> information non-fatal (in a sense that it neither causes kernel bug nor 
>> does it actually corrupt the unwinding process, but the only effect is 
>> losing "optional" information), having this data available doesn't hurt. 
> 
> But it does hurt, in the sense that the complicated format of DWARF CFI
> means the unwinder has to jump through a lot more hoops to read it.

Why that matters, actually? Unwinder is nothing to be performance
oriented. And if somebody is doing a lot of unwinding during runtime,
they can switch to in-this-case-faster FP unwinder.

> And if we wanted it to be reasonably reliable, we'd also need to fix up
> the DWARF data somehow before converting it, presumably with objtool.

We have to do this anyway. Be it the DWARF info or whatever we end up with.

>>> Unwinders basically only need to know one thing: given an instruction 
>>> address and a stack pointer, where is the caller's stack frame?
>>
>> Again, DWARF should be able to give us all of this (including the 
>> FP-fallback etc). It feels a bit silly to purposedly ignore it and 
>> reinvent parts of it again, instead of fixing (read: "asking toolchain 
>> guys to fix")

And we can just do, if a totally broken compiler emerges:
#if defined(CONFIG_DWARF_UNWINDER) && GCC_VERSION == 59000
#error Sorry, choose a different compiler or disable DWARF unwinder
#endif

We haven't to do this during the past decade and I am sceptic if we
would have to do it in the next one.

>>  the cases where we actually are not getting the proper data 
>> in DWARF. That's a win-win at the end of the day.
> 
> Most of the kernel DWARF issues I've seen aren't caused by toolchain
> bugs.  They're caused by the kernel's quirks: asm, inline asm, special
> sections.

Right.

> And anyway, fixing the correctness of the DWARF data is only half the
> problem IMO.  The other half of the problem is unwinder complexity.

Complex, but generic and working. IMO, it would be rather though to come
up with some tool working on different compilers or even different
versions of gcc. I mean some tool to convert the DWARF data to something
proprietary. The conversion would be as complex as is the unwinder plus
conversion to the proprietary format and its dump into ELF. We would
still rely on a (now out-of-kernel-runtime-code) complex monolith to do
it right.

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1638808

On Wed, May 10, 2017 at 10:32:06AM +0200, Jiri Slaby wrote:
> On 05/09/2017, 09:22 PM, Josh Poimboeuf wrote:
> > On Tue, May 09, 2017 at 08:47:50PM +0200, Jiri Kosina wrote:
> >> On Sun, 7 May 2017, Josh Poimboeuf wrote:
> >>
> >>> DWARF is great for debuggers.  It helps you find all the registers on 
> >>> the stack, so you can see function arguments and local variables.  All 
> >>> expressed in a nice compact format.
> >>>
> >>> But that's overkill for unwinders.  We don't need all those registers,
> >>> and the state machine is too complicated.  
> >>
> >> OTOH if we make the failures in processing of those "auxiliary" 
> >> information non-fatal (in a sense that it neither causes kernel bug nor 
> >> does it actually corrupt the unwinding process, but the only effect is 
> >> losing "optional" information), having this data available doesn't hurt. 
> > 
> > But it does hurt, in the sense that the complicated format of DWARF CFI
> > means the unwinder has to jump through a lot more hoops to read it.
> 
> Why that matters, actually? Unwinder is nothing to be performance
> oriented. And if somebody is doing a lot of unwinding during runtime,
> they can switch to in-this-case-faster FP unwinder.

More complexity == more bugs.

> > And anyway, fixing the correctness of the DWARF data is only half the
> > problem IMO.  The other half of the problem is unwinder complexity.
> 
> Complex, but generic and working. IMO, it would be rather though to come
> up with some tool working on different compilers or even different
> versions of gcc. I mean some tool to convert the DWARF data to something
> proprietary. The conversion would be as complex as is the unwinder plus
> conversion to the proprietary format and its dump into ELF. We would
> still rely on a (now out-of-kernel-runtime-code) complex monolith to do
> it right.

Complexity outside of the kernel is infinitely better than complexity in
mission critical oops code.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1638621

On 05/05/2017, 09:57 PM, Linus Torvalds wrote:
> On Fri, May 5, 2017 at 5:22 AM, Jiri Slaby <jslaby@suse.cz> wrote:
>> The DWARF unwinder is in place and ready. So introduce the config option
>> to allow users to enable it. It is by default off due to missing
>> assembly annotations.
> 
> Who actually ends up using this?

Every SUSE user has been using this for almost a decade and we are not
about to switch to FP for performance reasons as noted by Jiri Kosina.
So SUSE users are going to be exposed to DWARF unwinder for another
decade or so at least.

Therefore, this is another attempt to make the unwinder (in some form)
upstream. Since this was first proposed many years ago, we have been
forced to forward-port it over and over and everyone knows what pain it
is. So it is nice, that this opened the discussion at least.

> Because from the last time we had fancy unwindoers, and all the
> problems it caused for oops handling with absolutely _zero_ upsides
> ever, I do not ever again want to see fancy unwinders with complex
> state machine handling used by the oopsing code.

Well, reliable stack-traces with minimal performance impact thanks to
out-of-band data is hell good reason in my opinion.

> The fact that it gets disabled for KASAN also makes me suspicious. It
> basically means that now all the accesses it does are not even
> validated.

OK, I inclined to disable KASAN when I started cleaning this up for
_performance_ reasons. The system was so slow, that the RCU stall or
soft-lockup detectors came up complaining. From that time, I measured
the bottlenecks and optimized the unwinder so that 1000 iterations of
unwinding takes:

Before:
real    0m1.808s
user    0m0.001s
sys     0m1.807s

After:
real    0m0.018s
user    0m0.001s
sys     0m0.017s

So let me check, whether KASAN still has to be disabled globally. I do
not think so.

OTOH, TBH, I am not sure KASAN can be enabled for dwarf.c, the same as
holds now for the rest of the current unwinding:
KASAN_SANITIZE_dumpstack.o                              := n
KASAN_SANITIZE_dumpstack_$(BITS).o                      := n
KASAN_SANITIZE_stacktrace.o := n

Still, I can let KASAN := y for dwarf.c for testing purposes locally and
smoke-test the unwinder.

> The fact that the most of the code seems to be disabled for the first
> six patches, and then just enabled in the last patch, also seems to
> mean that the series also gets no bisection coverage or testing that
> the individual patches make any sense. (ie there's a lot of code
> inside "CONFIG_DWARF_UNWIND" in the early patches but that config
> option cannot even be enabled until the last patch).

Correct. This was one big patch previously. I separated that patch into
several smaller commits touching different places of the kernel for
easier review.

It does not make sense to test any of the patches separately except the
first. Hence the config option which enables the rest of the series is
the last one. I deemed this as one of possible approaches to split
patches (I have seen this many times in the past.) I can of course
squash this back into a single patch (or two).

> We used to have nasty issues with not just missing dwarf info, but
> also actively *wrong* dwarf info. Compiler versions that generated
> subtly wrong info, because nobody actually really depended on it, and
> the people who had tested it seldom did the kinds of things we do in 
> the kernel (eg inline asms etc).

I must admit I am not aware of any issues in this manner during the
years. Again, this unwinder is the default in SUSE kernels since ever,
so we have been using gcc from at least 3.2 to 7. But see below.

> So I'm personally still very suspicious of these things.
> 
> Last time I had huge issues with people also always blaming *anything*
> else than that unwinder. It was always "oh, somebody wrote asm without
> getting it right". Or "oh, the compiler generated bad tables, it's not
> *my* fault that now the kernel oopsing code no longer works".

Now we have objtool. My objtool clone:
1) verifies the DWARF data (prepared by Josh)

2) generates DWARF data for assembly -- incomplete yet: see the thread
about x86 assembly cleanup which is a pre-requisite for this to work.
This is BTW the reason why the DWARF unwinder is default-off in this
series yet.

And we can add:
3) fix up the data, if they are wrong

That said, objtool could handle the data so they are correct and
as-expected for the unwinder. Without objtool, the data (and unwinder)
is hopeless (only vdso from all the assembly is annotated.)

> When I asked for more stricter debug table validation to avoid issues,
> it was always "oh, we fixed it, no worries", and then two months later
> somebody hit another issue.

Reasonable, indeed. I am all for strict checking. objtool is to do that.

> Put another way; the last time we did crazy stuff like this, it got
> reverted. For a damn good reason, despite some people being in denial
> about those reasons.

Speaking for myself, having it out-of-tree causes me only troubles with
fwd-porting. So I am all ears to find a path to make this upstream and
maintain this there according to opinions of general kernel-community.
(Which reminds me I didn't add an entry to the MAINTAINERS file.)

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1638788

On Wed, May 10, 2017 at 09:39:39AM +0200, Jiri Slaby wrote:
> OTOH, TBH, I am not sure KASAN can be enabled for dwarf.c, the same as
> holds now for the rest of the current unwinding:
> KASAN_SANITIZE_dumpstack.o                              := n
> KASAN_SANITIZE_dumpstack_$(BITS).o                      := n
> KASAN_SANITIZE_stacktrace.o := n

Most of the unwinder code is now in unwind_frame.c, which *does* have
KASAN enabled.

I think the above is leftover from the days before I rewrote the
unwinder.  I'll look at renabling KASAN for those files.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1638789

On 05/10/2017, 02:42 PM, Josh Poimboeuf wrote:
> On Wed, May 10, 2017 at 09:39:39AM +0200, Jiri Slaby wrote:
>> OTOH, TBH, I am not sure KASAN can be enabled for dwarf.c, the same as
>> holds now for the rest of the current unwinding:
>> KASAN_SANITIZE_dumpstack.o                              := n
>> KASAN_SANITIZE_dumpstack_$(BITS).o                      := n
>> KASAN_SANITIZE_stacktrace.o := n
> 
> Most of the unwinder code is now in unwind_frame.c, which *does* have
> KASAN enabled.
> 
> I think the above is leftover from the days before I rewrote the
> unwinder.  I'll look at renabling KASAN for those files.

Ok, fair enough.

I will do some measurements in the FP field while I will be at it.

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1639015

On Wed, May 10, 2017 at 12:39 AM, Jiri Slaby <jslaby@suse.cz> wrote:
>
> Every SUSE user has been using this for almost a decade and we are not
> about to switch to FP for performance reasons as noted by Jiri Kosina.

The whole "not about to switch on frame pointers" argument is bogus.

Lots of people don't have frame pointers. The tracebacks don't look
all that horrible despite that.

If the problem is that some debug option that you want to use do that
"select FRAME_POINTER" thing, then maybe we should just fix that.

For example, I think it's annoying that the LATENCYTOP helper config
option basically forces frame pointers. That just seems stupid. Even
enabling CONFIG_STACKTRACE doesn't do that.

We do fine without frame pointers. Do traces get a bit uglier? Sure.
But that's not a huge deal.

> Well, reliable stack-traces with minimal performance impact thanks to
> out-of-band data is hell good reason in my opinion.

It's not the performance impact.

It's the other crap.

It's the fact that you have a whole state machine that isn't even
used. The only reason for that state machine is for register contents,
but then the register contents aren't actually used by the stack trace
code afaik.

Yeah, in theory that register engine might be used for dynamic stack
sizes too, but I don't think gcc actually generates code like that -
it uses frame pointers for variable-sized stacks, doesn't it?

But historically, it's even more the "oops, the unwind tables are
buggy because the test coverage is horrible, and we walked off into
the weeds while walking them, taking a recursive page fault, which
turned a WARN_ON() into a dead machine that didn't even give us the
information we wanted in the first place".

Now, it may be that with tools like objtool, we might be able to
*validate* the tables, which might actually be a good safety net.

So I'm not saying that the unwinder is a total no-go.

But I want to get rid of these red herring arguments in its favor.

If the argument *for* the unwinder i scrazy bullshit (eg "I want to
use LATENCYTOP without CONFIG_FRAME_POINTER"), then we should fix
those things independently.

>> The fact that it gets disabled for KASAN also makes me suspicious. It
>> basically means that now all the accesses it does are not even
>> validated.
>
> OK, I inclined to disable KASAN when I started cleaning this up for
> _performance_ reasons. The system was so slow, that the RCU stall or
> soft-lockup detectors came up complaining. From that time, I measured
> the bottlenecks and optimized the unwinder so that 1000 iterations of
> unwinding takes:
>
> Before:
> real    0m1.808s
> user    0m0.001s
> sys     0m1.807s
>
> After:
> real    0m0.018s
> user    0m0.001s
> sys     0m0.017s
>
> So let me check, whether KASAN still has to be disabled globally. I do
> not think so.
>
> OTOH, TBH, I am not sure KASAN can be enabled for dwarf.c, the same as
> holds now for the rest of the current unwinding:
> KASAN_SANITIZE_dumpstack.o                              := n
> KASAN_SANITIZE_dumpstack_$(BITS).o                      := n
> KASAN_SANITIZE_stacktrace.o := n

That's fine. But if the unwinder means no KASAN at all, then I don't
think the unwinder is good.

> Now we have objtool. My objtool clone:
> 1) verifies the DWARF data (prepared by Josh)
>
> 2) generates DWARF data for assembly -- incomplete yet: see the thread
> about x86 assembly cleanup which is a pre-requisite for this to work.
> This is BTW the reason why the DWARF unwinder is default-off in this
> series yet.
>
> And we can add:
> 3) fix up the data, if they are wrong

Yes. objtool might make the unwinder acceptable.

One of the things that caused the old unwinder to be absolutely
incredible *crap* was how it turned assembly language (both inline and
separate .S files) from a useful thing to absolutely horrible line
noise that was illegible and unmaintainable, and likely to be buggy to
boot.

So we may be in a different situation that we used to. But still..

              Linus

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web