Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improvethe code

[Ingo Molnar]

* Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx> wrote:

> >  - 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.
> > 
> >  - write_opcode() and any similar functions should be renamed to 
> >    the arch_uprobes_write_opcode() pattern
> 
> Currently the kernel/uprobes.c code handles insn as arch 
> agnostic in some cases and uses arch specific stuff for 
> analysis, verification and to set up fixups. The analysis, 
> verification, and fixups is only done at the probe insertion 
> only.
> 
> The copy_insn code, write_opcode is mostly arch agnostic 
> except for the maximum length of any supported instruction for 
> that architecture. If we move the insn to arch_uprobe, then we 
> would have to duplicate this code in arch specific files to do 
> the copying of the instruction. (not only at 
> registration/unregistration times and also at probe hit time 
> to copy into the slot).

Is there any reason why the core kernel uprobes.c code could not 
use uprobe->arch.insn directly?

It's in the architecture specific structure, mainly to 
encapsulate architecture-accessible fields and isolate low-level 
functionality from high-level one. This does not preclude the 
high-level code from using that field though.

Obviously every uprobes supporting architecture would have to 
define an 'insn' field in their 'struct arch_uprobe'.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/