On 1/4/08, Masami Hiramatsu <[EMAIL PROTECTED]> wrote:
> > + case KPROBE_HIT_SS:
> > + if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
> > + regs->flags &= ~TF_MASK;
> > + regs->flags |= kcb->kprobe_saved_flags;
> > +
On 1/4/08, Masami Hiramatsu <[EMAIL PROTECTED]> wrote:
> I could understand what the original code did at last.
> If a kprobe is inserted on a breakpoint which other debugger inserts,
> it single step inline instead of out-of-line.(this is done in
> prepare_singlestep)
> In this case, (p &&
Hi Abhishek,
Masami Hiramatsu wrote:
>> Hmm, I can not agree, because it is possible to insert a kprobe
>> into kprobe's instruction buffer. If it should be a bug, we must
>> check it when registering the kprobe.
>
> I discussed it with other maintainers and knew that current kprobes
> does not
Abhishek Sagar wrote:
> Masami Hiramatsu wrote:
...
> Done. You should find the desired changed in this patch.
Well done!
This cleans it up very well.
I have just one more comment.
> @@ -463,14 +487,26 @@ static int __kprobes reenter_kprobe(struct kprobe *p,
> struct pt_regs *regs,
>
Masami Hiramatsu wrote:
>>> As a result, this function just setups re-entrance.
>> As you've also pointed out in your previous reply, this case is
>> peculiar and therefore I believe it should be marked as a BUG(). I've
>> left the original case, if (kcb->kprobe_status==KPROBE_HIT_SS) &&
>>
Hi Abhishek,
Masami Hiramatsu wrote:
...
+ case KPROBE_HIT_SS:
+ if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
+ regs->flags &= ~TF_MASK;
+ regs->flags |= kcb->kprobe_saved_flags;
+ } else {
+
Masami Hiramatsu wrote:
As a result, this function just setups re-entrance.
As you've also pointed out in your previous reply, this case is
peculiar and therefore I believe it should be marked as a BUG(). I've
left the original case, if (kcb-kprobe_status==KPROBE_HIT_SS)
(*p-ainsn.insn ==
Abhishek Sagar wrote:
Masami Hiramatsu wrote:
...
Done. You should find the desired changed in this patch.
Well done!
This cleans it up very well.
I have just one more comment.
@@ -463,14 +487,26 @@ static int __kprobes reenter_kprobe(struct kprobe *p,
struct pt_regs *regs,
Hi Abhishek,
Masami Hiramatsu wrote:
Hmm, I can not agree, because it is possible to insert a kprobe
into kprobe's instruction buffer. If it should be a bug, we must
check it when registering the kprobe.
I discussed it with other maintainers and knew that current kprobes
does not allow
On 1/4/08, Masami Hiramatsu [EMAIL PROTECTED] wrote:
I could understand what the original code did at last.
If a kprobe is inserted on a breakpoint which other debugger inserts,
it single step inline instead of out-of-line.(this is done in
prepare_singlestep)
In this case, (p
On 1/4/08, Masami Hiramatsu [EMAIL PROTECTED] wrote:
+ case KPROBE_HIT_SS:
+ if (*p-ainsn.insn == BREAKPOINT_INSTRUCTION) {
+ regs-flags = ~TF_MASK;
+ regs-flags |= kcb-kprobe_saved_flags;
+ return 0;
+
Hi Abhishek,
Abhishek Sagar wrote:
> On 1/2/08, Masami Hiramatsu <[EMAIL PROTECTED]> wrote:
>> I think setup_singlestep() in your first patch is better, because it avoided
>> code duplication(*).
>
> Will retain it then.
Thank you very much.
>>> static int __kprobes reenter_kprobe(struct
* Abhishek Sagar <[EMAIL PROTECTED]> wrote:
> > > + default:
> > > + /* impossible cases */
> > > + BUG();
> >
> > I think no need to check that here.
>
> It may not get hit at runtime but is quite informative.
sidenote: please use WARN_ON(1) instead. In the case of
On 1/2/08, Masami Hiramatsu <[EMAIL PROTECTED]> wrote:
> I think setup_singlestep() in your first patch is better, because it avoided
> code duplication(*).
Will retain it then.
> > static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
> >
Hi Abhishek,
Thank you for good work.
Abhishek Sagar wrote:
> @@ -441,6 +441,26 @@ void __kprobes arch_prepare_kretprobe(struct
> kretprobe_instance *ri,
> /* Replace the return addr with trampoline addr */
> *sara = (unsigned long) _trampoline;
> }
> +
> +#if
Hi,
Abhishek Sagar wrote:
>>> 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
Hi,
Abhishek Sagar wrote:
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-eip -
Hi Abhishek,
Thank you for good work.
Abhishek Sagar wrote:
@@ -441,6 +441,26 @@ void __kprobes arch_prepare_kretprobe(struct
kretprobe_instance *ri,
/* Replace the return addr with trampoline addr */
*sara = (unsigned long) kretprobe_trampoline;
}
+
+#if
On 1/2/08, Masami Hiramatsu [EMAIL PROTECTED] wrote:
I think setup_singlestep() in your first patch is better, because it avoided
code duplication(*).
Will retain it then.
static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
struct
* Abhishek Sagar [EMAIL PROTECTED] wrote:
+ default:
+ /* impossible cases */
+ BUG();
I think no need to check that here.
It may not get hit at runtime but is quite informative.
sidenote: please use WARN_ON(1) instead. In the case of the impossible
Hi Abhishek,
Abhishek Sagar wrote:
On 1/2/08, Masami Hiramatsu [EMAIL PROTECTED] wrote:
I think setup_singlestep() in your first patch is better, because it avoided
code duplication(*).
Will retain it then.
Thank you very much.
static int __kprobes reenter_kprobe(struct kprobe *p,
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
On 1/1/08, Masami Hiramatsu <[EMAIL PROTECTED]> wrote:
> Could you separate changing logic from cleanup and explain
> why the logic should be changed?
The major portion of logical changes is re-routing of code flow by
removing gotos. Hopefully all the cases have been covered. There are a
couple
Just a few nitpicks. I need to look closer at the reenter_kprobe
changes, but it looks like this should lead to clearer flow than
before. The whole !p/kprobe_running() differences were pretty
twisty before.
On Wed, 2008-01-02 at 01:10 +0530, Abhishek Sagar wrote:
> Thanks for pointing me to the
Ingo Molnar wrote:
> hm, this patch does not apply to x86.git#mm, due to the fixes,
> unifications and cleanups done there. Could you send a patch against -mm
> or against x86.git? (see the tree-fetching instructions below) Thanks,
>
> Ingo
>
> --{ x86.git instructions
Hello Abhishek,
Abhishek Sagar wrote:
> Harvey Harrison wrote:
>> Make the control flow of kprobe_handler more obvious.
>>
>> Collapse the separate if blocks/gotos with if/else blocks
>> this unifies the duplication of the check for a breakpoint
>> instruction race with another cpu.
>
> This is
* Abhishek Sagar <[EMAIL PROTECTED]> wrote:
> Harvey Harrison wrote:
> > Make the control flow of kprobe_handler more obvious.
> >
> > Collapse the separate if blocks/gotos with if/else blocks
> > this unifies the duplication of the check for a breakpoint
> > instruction race with another cpu.
* Abhishek Sagar [EMAIL PROTECTED] wrote:
Harvey Harrison wrote:
Make the control flow of kprobe_handler more obvious.
Collapse the separate if blocks/gotos with if/else blocks
this unifies the duplication of the check for a breakpoint
instruction race with another cpu.
This is a
Hello Abhishek,
Abhishek Sagar wrote:
Harvey Harrison wrote:
Make the control flow of kprobe_handler more obvious.
Collapse the separate if blocks/gotos with if/else blocks
this unifies the duplication of the check for a breakpoint
instruction race with another cpu.
This is a patch
Ingo Molnar wrote:
hm, this patch does not apply to x86.git#mm, due to the fixes,
unifications and cleanups done there. Could you send a patch against -mm
or against x86.git? (see the tree-fetching instructions below) Thanks,
Ingo
--{ x86.git instructions }--
Just a few nitpicks. I need to look closer at the reenter_kprobe
changes, but it looks like this should lead to clearer flow than
before. The whole !p/kprobe_running() differences were pretty
twisty before.
On Wed, 2008-01-02 at 01:10 +0530, Abhishek Sagar wrote:
Thanks for pointing me to the
On 1/1/08, Masami Hiramatsu [EMAIL PROTECTED] wrote:
Could you separate changing logic from cleanup and explain
why the logic should be changed?
The major portion of logical changes is re-routing of code flow by
removing gotos. Hopefully all the cases have been covered. There are a
couple of
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
Harvey Harrison wrote:
> Make the control flow of kprobe_handler more obvious.
>
> Collapse the separate if blocks/gotos with if/else blocks
> this unifies the duplication of the check for a breakpoint
> instruction race with another cpu.
This is a patch derived from kprobe_handler of the ARM
Harvey Harrison wrote:
Make the control flow of kprobe_handler more obvious.
Collapse the separate if blocks/gotos with if/else blocks
this unifies the duplication of the check for a breakpoint
instruction race with another cpu.
This is a patch derived from kprobe_handler of the ARM kprobes
* Masami Hiramatsu <[EMAIL PROTECTED]> wrote:
> And also, I prefer "return 1" to "{ret = 1; goto out;}" for
> simplicity.
it seems that the code already uses 'ret', and sets it to 1 in certain
cases - in that light it's a tiny bit cleaner to just have a single
return code flow.
Ingo
Hello Harvey,
Thank you for your great works!
Harvey Harrison wrote:
> Make the control flow of kprobe_handler more obvious.
>
> Collapse the separate if blocks/gotos with if/else blocks
> this unifies the duplication of the check for a breakpoint
> instruction race with another cpu.
I agree
Hello Harvey,
Thank you for your great works!
Harvey Harrison wrote:
Make the control flow of kprobe_handler more obvious.
Collapse the separate if blocks/gotos with if/else blocks
this unifies the duplication of the check for a breakpoint
instruction race with another cpu.
I agree it is
* Masami Hiramatsu [EMAIL PROTECTED] wrote:
And also, I prefer return 1 to {ret = 1; goto out;} for
simplicity.
it seems that the code already uses 'ret', and sets it to 1 in certain
cases - in that light it's a tiny bit cleaner to just have a single
return code flow.
Ingo
--
To
39 matches
Mail list logo
