lkml.org 
[lkml]   [2008]   [Jan]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] x86: kprobes change kprobe_handler flow
    On 1/2/08, Harvey Harrison <harvey.harrison@gmail.com> 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


    \
     
     \ /
      Last update: 2008-01-01 21:57    [W:0.026 / U:69.292 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site