Messages in this thread | | | Date | Fri, 17 Feb 2012 11:49:58 +0100 | From | Ingo Molnar <> | Subject | Re: [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
| |