Message-ID: <20120217104958.GA21998@elte.hu>
Date:	Fri, 17 Feb 2012 11:49:58 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, hpa@...or.com, mingo@...hat.com,
	jkenisto@...ibm.com, a.p.zijlstra@...llo.nl, ananth@...ibm.com,
	anton@...hat.com, masami.hiramatsu.pt@...achi.com,
	acme@...radead.org, srikar@...ux.vnet.ibm.com, tglx@...utronix.de,
	oleg@...hat.com
Subject: Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve
 the code


Srikar,

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

> Commit-ID:  7b2d81d48a2d8e37efb6ce7b4d5ef58822b30d89
> Gitweb:     http://git.kernel.org/tip/7b2d81d48a2d8e37efb6ce7b4d5ef58822b30d89
> Author:     Ingo Molnar <mingo@...e.hu>
> AuthorDate: Fri, 17 Feb 2012 09:27:41 +0100
> Committer:  Ingo Molnar <mingo@...e.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@...ux.vnet.ibm.com>
> Cc: Jim Keniston <jkenisto@...ibm.com>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Oleg Nesterov <oleg@...hat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> Cc: Arnaldo Carvalho de Melo <acme@...radead.org>
> Cc: Anton Arapov <anton@...hat.com>
> Cc: Ananth N Mavinakayanahalli <ananth@...ibm.com>
> Link: http://lkml.kernel.org/n/tip-ewbwhb8o6navvllsauu7k07p@git.kernel.org
> Signed-off-by: Ingo Molnar <mingo@...e.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists