Re: [PATCH] tracing: (backport) Replace kmap with copy_from_user() in trace_marker

[Henrik Austad]

On Fri, Dec 09, 2016 at 08:22:05AM +0100, Greg KH wrote:
> On Fri, Dec 09, 2016 at 07:34:04AM +0100, Henrik Austad wrote:
> > Instead of using get_user_pages_fast() and kmap_atomic() when writing
> > to the trace_marker file, just allocate enough space on the ring buffer
> > directly, and write into it via copy_from_user().
> > 
> > Writing into the trace_marker file use to allocate a temporary buffer
> > to perform the copy_from_user(), as we didn't want to write into the
> > ring buffer if the copy failed. But as a trace_marker write is suppose
> > to be extremely fast, and allocating memory causes other tracepoints to
> > trigger, Peter Zijlstra suggested using get_user_pages_fast() and
> > kmap_atomic() to keep the user space pages in memory and reading it
> > directly.
> > 
> > Instead, just allocate the space in the ring buffer and use
> > copy_from_user() directly. If it faults, return -EFAULT and write
> > "<faulted>" into the ring buffer.
> > 
> > On architectures without a arch-specific get_user_pages_fast(), this
> > will end up in the generic get_user_pages_fast() and this grabs
> > mm->mmap_sem. Once you do this, then suddenly writing to the
> > trace_marker can cause priority-inversions.
> > 
> > This is a backport of Steven Rostedts patch [1] and applied to 3.10.x so the
> > signed-off-chain by is somewhat uncertain at this stage.
> > 
> > The patch compiles, boots and does not immediately explode on impact. By
> > definition [2] it must therefore be perfect
> > 
> > 2) https://www.spinics.net/lists/kernel/msg2400769.html
> > 2) http://lkml.iu.edu/hypermail/linux/kernel/9804.1/0149.html
> > 
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Henrik Austad <henrik@xxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > 
> > Suggested-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Used-to-be-signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Backported-by: Henrik Austad <haustad@xxxxxxxxx>
> > Tested-by: Henrik Austad <haustad@xxxxxxxxx>
> > Signed-off-by: Henrik Austad <haustad@xxxxxxxxx>
> > ---
> >  kernel/trace/trace.c | 78 +++++++++++++++-------------------------------------
> >  1 file changed, 22 insertions(+), 56 deletions(-)
> 
> What is the git commit id of this patch in Linus's tree?  And what
> stable trees do you feel it should be applied to?

Ah, perhaps I jumped the gun here. I don't think Linus has picked this one 
up yet, Steven sent out the patch yesterday.

Since then, I've backported it to 3.10 and ran the first set of tests 
over night and it looks good. So ideally this would find its way into 
3.10(.104).

Do you want med to resubmit when Stevens patch is merged upstream?

-Henrik