lkml.org 
[lkml]   [2011]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC 4/5] x86, perf: implements lwp-perf-integration (rc1)

* Joerg Roedel <joro@8bytes.org> wrote:

> Hi Ingo,
>
> On Tue, Dec 20, 2011 at 11:09:17AM +0100, Ingo Molnar wrote:
> > >
> > > No, it's worse. They are ring 3 writeable, and ring 3
> > > configurable.
> >
> > Avi, i know that very well.
>
> So you agree that your ideas presented in this thread of
> integrating LWP into perf have serious security implications?

No, i do not agree at all - you are drastically misrepresending
my position.

> > > btw, that means that the intended use case -
> > > self-monitoring with no kernel support - cannot be done.
> > > [...]
> >
> > Arguably many years ago the hardware was designed for
> > brain-dead instrumentation abstractions.
>
> The point of LWP design is, that it doesn't require
> abstractions except for the threshold interrupt.
>
> I am fine with integrating LWP into perf as long as it makes
> sense and does not break the intended usage scenario for LWP.

That's the wrong way around - in reality we'll integrate LWP
upstream only once it makes sense and works well with the
primary instrumentation abstraction we have in the kernel.

Otherwise my "sorry, it's not convincing enough yet" NAK against
the new feature stands.

In fact as per Linus's rules about new kernel features,
maintainers don't even have to justify NAK's by offering an
implementation roadmap that would make the feature acceptable.

Me or PeterZ could just say "this feature is too limited and not
convincing enough yet, sorry".

*You* who are pushing the feature have to convince the objecting
maintainer that the feature is worth integrating.

But i'm being nice and helpful here by giving you a rough
technical outline of how you could overcome my "sorry, this is
not convincing in its current form yet" rejection of the current
LWP patches.

> [ Because LWP is a user-space feature and designed as such,
> forcing it into an abstraction makes software that uses LWP
> unportable. ]
>
> But Ingo, the ideas you presented in this thread are clearly
> no-gos.

Nonsense.

> Having a shared per-cpu buffer for LWP data that is read by
> perf obviously has very bad security implications, as Avi
> already pointed out. [...]

Stop this stupidity already!

There's no "security implications" whatsoever. LWP is a ring-3
hw feature and it can do nothing that the user-space app could
not already do ...

> [...] It also destroys the intended use-case for LWP because
> it disturbs any process that is doing self-profiling with LWP.

Why would it destroy that? Self-profiling can install events
just fine, the kernel will arbitrate the resource.

The 'intended usecase' is meaningless to me - it was done in
some closed process apparently not talking to anyone who knows a
bit about Linux instrumentation. If you want this code upstream
then you need to convince me that the feature makes sense in the
general and current scheme of things.

I've outlined the (rather workable) technical roadmap for that.

> > Note that as i said user-space *can* acccess the area if it
> > thinks it can do it better than the kernel (and we could
> > export that information in a well defined way - we could do
> > the same for PEBS as well) - i have no particular strong
> > feelings about allowing that other than i think it's an
> > obviously inferior model - *as long* as proper, generic,
> > usable support is added.
>
> LWP can't be compared in any serious way with PEBS. The only
> common thing is the hardware-managed ring-buffer. [...]

Which ring-buffer is actually happens to be one of the main
things that has to be managed ...

> [...] But PEBS is an addition to MSR based performance
> monitoring resources (for which a kernel abstraction makes a
> lot of sense) and can only be controlled from ring 0 while LWP
> is a complete user-space controlled PMU which has no link at
> all to the MSR-based, ring 0 controlled PMU.

It's a ring-3 controlled PMU feature, not a user-space PMU
feature. It *can* be controlled by user-space - but it obviously
can also (and i argue, it should be) - managed by the kernel,
under Linux.

The kernel is running ring-3 code as well, and it's managing
ring-3 accessible resources as well, there's nothing new about
that.

> > From my perspective there's really just one realistic option
> > to accept this feature: if it's properly fit into existing,
> > modern instrumentation abstractions. I made that abundantly
> > clear in my feedback so far.
>
> The threshold interrupt fits well into the perf-abstraction
> layer. Even self-monitoring of processes does, and Hans posted
> patches from Benjamin for that. What do you think about this
> approach?

As as i said it's a promising first step - although the
discussion here convinced me that it needs to be even more
feature complete, i don't really see that you guys understand
how such things should be implemented.

You seem to be dead set on supporting a weird special case
'intended workload' while forgetting the *much* more common
profilin workloads we have under Linux.

I don't mind supporting weird stuff as well, but you have to
keep the common case in mind ...

I'd like to see the ring-buffer and the events managed by the
kernel too, at least so that perf record works fine with this
PMU feature.

> > User-space can smash it and make it not profile or profile
> > the wrong thing or into the wrong buffer - but LWP itself
> > runs with ring3 privileges so it won't do anything the user
> > couldnt do already.
>
> The point is, if user-space re-programs LWP it will continue
> to write its samples to the new ring-buffer virtual-address
> set up by user-space. It will still use that virtual address
> in another address-space after a task-switch. This allows
> processes to corrupt memory of other processes. [...]

That's nonsense. As i said it my previous mail the LWPC should
be per task and switched on task switch - just like the DS/PEBS
context is.

> [...] There are ways to hack around that but these have a
> serious impact on task-switch costs so this is also no way to
> go.

We are seeing no problems with this approach under PEBS.

> > Lack of protection against self-misconfiguration-damage is a
> > benign hardware mis-feature - something for LWP v2 to
> > specify i guess.
>
> So what you are saying is (not just here, also in other emails
> in this thread) that every hardware not designed for perf is
> crap?

No - PMU hardware designed to not allow the profiling of the
kernel is obviously a crappy aspect of it. Also, PMU hardware
that does not allow 100% encapsulation by the kernel is
obviously not very wisely done either.

Those limitations are not a big problem for usable Linux support
- and future iterations of the LWP hardware can trivially
address those shortcomings.

> > get_unmapped_area() + install_special_mapping() is probably
> > better, but yeah.
>
> get_unmapped_area() only works on current. [...]

Which is a perfectly fine first step to support the
'perf record' inheritance-tree case - which is a very
common profiling method.

> [...] So it can't be used for that purpose too. [...]

Hey, i wrote bits of get_unmapped_area(), way back. I had code
on my machine that inserted vmas into other tasks's address
spaces and can confirm that it works. Do you take my word for it
that it's possible?

Firstly, the perf record case - which is an important, primary
workflow - can work with the code as-is just fine.

Secondly, for system-wide profiling vmas can be inserted into
another task's mm context just fine as well: technically we do
that all the time, when a threaded program is running.

Inserting a vma into another task's mm where that mm is not ours
is indeed not typical, but not unprecedented either, UML patches
did that a couple of years ago. (In fact the upcoming uprobes
patches are doing something far more intrusive.)

The VM modification is trivial AFAICS: an 'mm' parameter has to
be added to a new do_mmap() variant, that's all - the code is
already SMP-safe, due to the threaded case.

Otherwise using another task's mm is safe if you acquire it via
get_task_mm()/mmput().

[ Sidenote: as a bonus this would put infrastructure in place to
have user-space accessible trace buffers, insertable via
the LWPINS instruction and recoverable via the regular kernel
perf event processing facilities. LWP has more potential than
just self-profiling, if we use the right abstractions... ]

Thanks,

Ingo


\
 
 \ /
  Last update: 2011-12-20 19:45    [W:0.092 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site