On 1/2/08, Harvey Harrison <[EMAIL PROTECTED]> wrote:
> > +#if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
> > +static __always_inline int setup_boost(struct kprobe *p, struct pt_regs
> > *regs)
> > +{
> > + if (p->ainsn.boostable == 1 && !p->post_handler) {
> > + /* Boost up -- we can execute copied instructions directly */
> > + reset_current_kprobe();
> > + regs->ip = (unsigned long)p->ainsn.insn;
> > + preempt_enable_no_resched();
> > + return 0;
> > + }
> > + return 1;
> > +}
> > +#else
> > +static __always_inline int setup_boost(struct kprobe *p, struct pt_regs
> > *regs)
> > +{
> > + return 1;
> > +}
> > +#endif
> > +
> In the kernel __always_inline == inline, also I think it's nicer to only
> have one function declaration, and then ifdef the body of the function.
>
> Something like:
>
> static inline int setup_boost(struct kprobe *p, struct pt_regs *regs)
> {
> #if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
> if (p->ainsn.boostable == 1 && !p->post_handler) {
> /* Boost up -- we can execute copied instructions directly */
> reset_current_kprobe();
> regs->ip = (unsigned long)p->ainsn.insn;
> preempt_enable_no_resched();
> return 0;
> }
> #endif
> return 1;
> }
Ok...will include this after I pick up some more comments.
> > static int __kprobes kprobe_handler(struct pt_regs *regs)
> > {
> > - struct kprobe *p;
> > int ret = 0;
> > kprobe_opcode_t *addr;
> > + struct kprobe *p, *cur;
> > struct kprobe_ctlblk *kcb;
> >
> > addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));
> > + if (*addr != BREAKPOINT_INSTRUCTION) {
> > + /*
> > + * The breakpoint instruction was removed right
> > + * after we hit it. Another cpu has removed
> > + * either a probepoint or a debugger breakpoint
> > + * at this address. In either case, no further
> > + * handling of this interrupt is appropriate.
> > + * Back up over the (now missing) int3 and run
> > + * the original instruction.
> > + */
> > + regs->ip = (unsigned long)addr;
> > + return 1;
> > + }
>
> This return is fine I guess, but after the preempt_disable() I like
> the goto approach as it will be easier to see what paths enable
> preemption again and which don't....bonus points if we can move this
> to the caller or make sure we reenable in all cases before returning
> and pull in the code in the caller that does this for us.
>
> But I guess your approach of using ret to test whether we need to
> reenable preemption or not would work as a signal to the caller that
> they need to reenable preemption.
Hmm...since enabling preemption is tied to 'ret', anyone reading
kprobe_handler will have to follow around all calls which modify it.
There are some checks in the current kprobe_handler definition made
just to do what you're saying, i.e, to push all preemption
enable/disables in krpobe_handler. LIke this one (from the current x86
kprobe_handler):
------------
ret = reenter_kprobe(p, regs, kcb);
if (kcb->kprobe_status == KPROBE_REENTER)
{
ret = 1;
goto out;
}
goto preempt_out;
-------------
This is just confusing because we're not actually making any
exceptions here for the KPROBE_REENTER case (which has been partially
handled in reenter_kprobe), rather just tricking our way out of
preemption enabling for a cpl of cases in reenter_kprobe.
> Cheers,
>
> Harvey
Thanks,
Abhishek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
