All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	hpa@zytor.com, eranian@google.com, linux-kernel@vger.kernel.org,
	fweisbec@gmail.com, akpm@linux-foundation.org,
	tglx@linutronix.de, linux-tip-commits@vger.kernel.org,
	Robert Richter <robert.richter@amd.com>
Subject: Re: [tip:perf/core] perf/x86: Fix USER/KERNEL tagging of samples
Date: Tue, 10 Jul 2012 10:21:04 +0200	[thread overview]
Message-ID: <20120710082104.GA11187@gmail.com> (raw)
In-Reply-To: <1341906848.3462.92.camel@twins>


* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Mon, 2012-07-09 at 20:41 +0200, Ingo Molnar wrote:
> > > +static unsigned long get_segment_base(unsigned int segment)
> > > +{
> > > +     struct desc_struct *desc;
> > > +     int idx = segment >> 3;
> > > +
> > > +     if ((segment & SEGMENT_TI_MASK) == SEGMENT_LDT) {
> > > +             if (idx > LDT_ENTRIES)
> > > +                     return 0;
> > > +
> > > +             desc = current->active_mm->context.ldt;
> > > +     } else {
> > > +             if (idx > GDT_ENTRIES)
> > > +                     return 0;
> > > +
> > > +             desc = __this_cpu_ptr(&gdt_page.gdt[0]);
> > > +     }
> > > +
> > > +     return get_desc_base(desc + idx);
> > 
> > Shouldn't idx be checked against active_mm->context.ldt.size, 
> > not LDT_ENTRIES (which is really just an upper limit)?
> 
> Ah indeed, fixed that.

Another boundary condition would be when we intentionally 
twiddle the GDT: such as during suspend or during BIOS upcalls. 
Can we then get a PMU interrupt? If yes then this will probably 
result in garbage:

> > > +             desc = __this_cpu_ptr(&gdt_page.gdt[0]);

it won't outright crash, we don't ever deallocate our GDT - but 
it will return a garbage RIP.

Then there's also all the Xen craziness with segments ...

Both ought to be rare an uninteresting - but then again, 
segmented execution is already rare and uninteresting to begin 
with.

So, instead of trying to discover all these weird x86 cases - 
with little to no testing done after that - I thought that it 
might be more future proof to just handle the cases we are 
explicitly interested in: flat code, and pounce in some well 
defined way in all the other situations by returning the RIP to 
an empty __X86_LEGACY_SEGMENTED_CODE() symbol.

That way we will at least give *some* useful information to the 
poor segmented code user, if the profile says:

    21.32%  [kernel]      [k] __X86_LEGACY_SEGMENTED_CODE
    11.01%  [kernel]      [k] kallsyms_expand_symbol     
     8.29%  [kernel]      [k] vsnprintf                  
     7.37%  libc-2.15.so  [.] __strcmp_sse42             
     6.93%  perf          [.] symbol_filter              
     4.20%  perf          [.] kallsyms__parse            
     3.92%  [kernel]      [k] format_decode              
     3.62%  [kernel]      [k] string.isra.4              
     3.59%  [kernel]      [k] memcpy                     
     3.11%  [kernel]      [k] strnlen                    

then the user at least knows that there's 21% of overhead in 
some sort of segmented x86 code. Or if they *really* want to 
resolve that, they can take your patch and add symbol decoding 
to user-space and test it all.

KISS and such.

Linus?

Thanks,

	Ingo

  parent reply	other threads:[~2012-07-10  8:21 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-07-06  6:20 [tip:perf/core] perf/x86: Fix USER/KERNEL tagging of samples tip-bot for Peter Zijlstra
2012-07-06 16:29 ` Linus Torvalds
2012-07-06 18:12   ` Peter Zijlstra
2012-07-06 18:16     ` Linus Torvalds
2012-07-06 18:34       ` Linus Torvalds
2012-07-06 20:48         ` Peter Zijlstra
2012-07-06 20:59           ` Linus Torvalds
2012-07-09 11:23         ` Peter Zijlstra
2012-07-09 17:55           ` Linus Torvalds
2012-07-10  9:02             ` Peter Zijlstra
2012-07-10  9:48               ` Ingo Molnar
2012-07-10  9:50               ` Peter Zijlstra
2012-07-10  9:52                 ` Peter Zijlstra
2012-07-10  9:55                 ` Ingo Molnar
2012-07-31 17:57               ` [tip:perf/urgent] perf/x86: Fix USER/ KERNEL tagging of samples properly tip-bot for Peter Zijlstra
2012-07-09 18:41           ` [tip:perf/core] perf/x86: Fix USER/KERNEL tagging of samples Ingo Molnar
2012-07-10  7:54             ` Peter Zijlstra
2012-07-10  8:02               ` Ingo Molnar
2012-07-10  8:21               ` Ingo Molnar [this message]
2012-07-10  8:52                 ` Peter Zijlstra
2012-07-10  9:48                   ` Ingo Molnar
2012-07-31 18:11                     ` H. Peter Anvin

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 \
    --in-reply-to=20120710082104.GA11187@gmail.com \
    --to=mingo@kernel.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=akpm@linux-foundation.org \
    --cc=eranian@google.com \
    --cc=fweisbec@gmail.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tip-commits@vger.kernel.org \
    --cc=robert.richter@amd.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.