lkml.org 
[lkml]   [2012]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

Srikar,

* tip-bot for Ingo Molnar <mingo@elte.hu> wrote:

> Commit-ID: 7b2d81d48a2d8e37efb6ce7b4d5ef58822b30d89
> Gitweb: http://git.kernel.org/tip/7b2d81d48a2d8e37efb6ce7b4d5ef58822b30d89
> Author: Ingo Molnar <mingo@elte.hu>
> AuthorDate: Fri, 17 Feb 2012 09:27:41 +0100
> Committer: Ingo Molnar <mingo@elte.hu>
> CommitDate: Fri, 17 Feb 2012 10:18:07 +0100
>
> uprobes/core: Clean up, refactor and improve the code
>
> Make the uprobes code readable to me:
>
> - improve the Kconfig text so that a mere mortal gets some idea
> what CONFIG_UPROBES=y is really about
>
> - do trivial renames to standardize around the uprobes_*() namespace
>
> - clean up and simplify various code flow details
>
> - separate basic blocks of functionality
>
> - line break artifact and white space related removal
>
> - use standard local varible definition blocks
>
> - use vertical spacing to make things more readable
>
> - remove unnecessary volatile
>
> - restructure comment blocks to make them more uniform and
> more readable in general
>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Jim Keniston <jkenisto@us.ibm.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
> Cc: Anton Arapov <anton@redhat.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Link: http://lkml.kernel.org/n/tip-ewbwhb8o6navvllsauu7k07p@git.kernel.org
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
> arch/Kconfig | 14 ++-
> arch/x86/include/asm/uprobes.h | 17 ++--
> arch/x86/kernel/uprobes.c | 129 ++++++++++++-----------
> include/linux/uprobes.h | 28 +++---
> kernel/uprobes.c | 219 +++++++++++++++++++++++-----------------
> mm/mmap.c | 12 +-
> 6 files changed, 233 insertions(+), 186 deletions(-)

FYI, I created a new -tip topic branch for uprobes commits
[tip:perf/uprobes], with two patches applied so far:

- your v10 #1/9 patch
- the above clean-ups and restructurings above it

( Please have a good look at the code flow clean-ups I have
performed, I have only build-tested the result. )

As you can see it from the diffstat, there were many small
details that hindered code cleanliness, and there's at least one
bigger item that still needs to be addressed, which I have not
done in the above patch:

The arch methods and structures need to be better separated from
core uprobes code. Right now there's uprobes.h which exports
'struct uprobe' to the rest of the kernel, such as
arch/x86/kernel/uprobes.c.

What we want instead is a clean separation of core code from
architecture code:

- 'struct arch_uprobe' which includes all fields that
'struct uprobe_arch_info' contains today. In the first
approximation this is a rename.

- please rename uprobe->arch_info to uprobe->arch - it's
already clear that it's information.

- uprobe->insn[] needs to move from struct uprobe to
uprobe->arch.insn

- The uprobes_arch_*() method(s) should be passed a
'struct arch_uprobe *', not a 'struct uprobe *'.

- Once this is done, 'struct uprobe' can move to the head of
kernel/uprobes.c, without any ugly #ifdefs and wrappery -
that code only compiles if uprobes are enabled and if the
architecture supports it.

- asm/uprobes.h defines 'struct arch_uprobe' and the arch
method(s) - nothing else.

- unless uprobe_opcode_t is non-byte on any architecture this
typedef should probably be go away and be char * or void *.

- write_opcode() and any similar functions should be renamed to
the arch_uprobes_write_opcode() pattern

These changes should be done on top of latest -tip, in a single
patch or a small series, it's mostly pretty straightforward.

Please keep an eye on stylistic details for future patches - the
above patch gives a laundry list of what details were wrong in
the existing code.

Once this is done can we add #2, #3, etc. patches in a clean
manner.

I'd suggest you don't send a full series but send small, edible
steps on top of the minimal (and right now not yet functional)
uprobes core code I've already applied, in series of 1-3
patches.

I'm sure there will be other small details we'll have to change
in these patches as we go forward, so not porting the full
series straight away would save you time and effort that the
inevitable conflicts create.

As an additional benefit, if you do it in such a workflow then I
can apply your new patches to tip:perf/uprobes pretty much the
moment you send them out, reviewing and testing them quickly -
without the time-consuming review of a long and interdependent
patch series and the resulting churn on your side.

I believe we could iterate this through in a couple of days and
arrive to a working 'perf probe' prototype.

Thanks,

Ingo


\
 
 \ /
  Last update: 2012-02-17 11:53    [W:0.072 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site