From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754189AbdEJSMC (ORCPT ); Wed, 10 May 2017 14:12:02 -0400 Received: from mail-io0-f171.google.com ([209.85.223.171]:36710 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754060AbdEJSMA (ORCPT ); Wed, 10 May 2017 14:12:00 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170505122200.31436-1-jslaby@suse.cz> <20170505122200.31436-7-jslaby@suse.cz> From: Linus Torvalds Date: Wed, 10 May 2017 11:11:58 -0700 X-Google-Sender-Auth: d_lSOr86Aq6S-Tlcqc08cXzpjcM Message-ID: Subject: Re: [PATCH 7/7] DWARF: add the config option To: Jiri Slaby Cc: Andrew Morton , live-patching@vger.kernel.org, Linux Kernel Mailing List , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "the arch/x86 maintainers" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 10, 2017 at 12:39 AM, Jiri Slaby 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