[PATCH -v5 00/11] ftrace for MIPS

On Sun, 2009-10-25 at 23:17 +0800, Wu Zhangjin wrote:

> +
> +unsigned long ftrace_get_parent_addr(unsigned long self_addr,
> + unsigned long parent,
> + unsigned long parent_addr,
> + unsigned long fp)
> +{
> + unsigned long sp, ip, ra;
> + unsigned int code;
> +
> + /* move to the instruction "move ra, at" */
> + ip = self_addr - 8;
> +
> + /* search the text until finding the "move s8, sp" instruction or
> + * "s{d,w} ra, offset(sp)" instruction */
> + do {
> + ip -= 4;
> +
> + /* get the code at "ip" */
> + code = *(unsigned int *)ip;

Probably want to put the above in an asm with exception handling.

> +
> + /* If we hit the "move s8(fp), sp" instruction before finding
> + * where the ra is stored, then this is a leaf function and it
> + * does not store the ra on the stack. */
> + if ((code & MOV_FP_SP) == MOV_FP_SP)
> + return parent_addr;
> + } while (((code & S_RA) != S_RA));

Hmm, that condition also looks worrisome. Should we just always search
for s{d,w} R,X(sp)?

Since there should only be stores of registers into the sp above the
jump to mcount. The break out loop is a check for move. I think it would
be safer to have the break out loop is a check for non storing of a
register into SP.

> +
> + sp = fp + (code & STACK_OFFSET_MASK);
> + ra = *(unsigned long *)sp;

Also might want to make the above into a asm with exception handling.

> +
> + if (ra == parent)
> + return sp;
> +
> + ftrace_graph_stop();
> + WARN_ON(1);
> + return parent_addr;

Hmm, may need to do more than this. See below.

> +}
> +
> +/*
> + * Hook the return address and push it in the stack of return addrs
> + * in current thread info.
> + */
> +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
> + unsigned long fp)
> +{
> + unsigned long old;
> + struct ftrace_graph_ent trace;
> + unsigned long return_hooker = (unsigned long)
> + &return_to_handler;
> +
> + if (unlikely(atomic_read(&current->tracing_graph_pause)))
> + return;
> +
> + /* "parent" is the stack address saved the return address of the caller
> + * of _mcount, for a leaf function not save the return address in the
> + * stack address, so, we "emulate" one in _mcount's stack space, and
> + * hijack it directly, but for a non-leaf function, it will save the
> + * return address to the its stack space, so, we can not hijack the
> + * "parent" directly, but need to find the real stack address,
> + * ftrace_get_parent_addr() does it!
> + */
> +
> + old = *parent;
> +
> + parent = (unsigned long *)ftrace_get_parent_addr(self_addr, old,
> + (unsigned long)parent,
> + fp);
> +
> + *parent = return_hooker;

Although you may have turned off fgraph tracer in
ftrace_get_parent_addr, nothing stops the below from messing with the
stack. The return stack may get off sync and break later. If you fail
the above, you should not be calling the push function below.


-- Steve

> +
> + if (ftrace_push_return_trace(old, self_addr, &trace.depth, fp) ==
> + -EBUSY) {
> + *parent = old;
> + return;
> + }
> +
> + trace.func = self_addr;
> +
> + /* Only trace if the calling function expects to */
> + if (!ftrace_graph_entry(&trace)) {
> + current->curr_ret_stack--;
> + *parent = old;
> + }
> +}
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */


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

Hi,

On Mon, 2009-10-26 at 11:13 -0400, Steven Rostedt wrote:
[...]
> > +
> > + /* get the code at "ip" */
> > + code = *(unsigned int *)ip;
>
> Probably want to put the above in an asm with exception handling.
>

Seems that exception handling in an asm is really "awful"(un-readable)
and the above ip is what we have got from the ftrace_graph_caller, it
should be okay. but if exception handling is necessary, I will send a
new patch for the places(including the following one) which need it.

> > +
> > + /* If we hit the "move s8(fp), sp" instruction before finding
> > + * where the ra is stored, then this is a leaf function and it
> > + * does not store the ra on the stack. */
> > + if ((code & MOV_FP_SP) == MOV_FP_SP)
> > + return parent_addr;
> > + } while (((code & S_RA) != S_RA));
>
> Hmm, that condition also looks worrisome. Should we just always search
> for s{d,w} R,X(sp)?
>
> Since there should only be stores of registers into the sp above the
> jump to mcount. The break out loop is a check for move. I think it would
> be safer to have the break out loop is a check for non storing of a
> register into SP.


Okay, let's look at this with -mlong-calls,

leaf function:

ffffffff80243cd8 <oops_may_print>:
ffffffff80243cd8: 67bdfff0 daddiu sp,sp,-16
ffffffff80243cdc: ffbe0008 sd s8,8(sp)
ffffffff80243ce0: 03a0f02d move s8,sp
ffffffff80243ce4: 3c038021 lui v1,0x8021
ffffffff80243ce8: 646316b0 daddiu v1,v1,5808
ffffffff80243cec: 03e0082d move at,ra
ffffffff80243cf0: 0060f809 jalr v1
ffffffff80243cf4: 00020021 nop

non-leaf function:

ffffffff802414c0 <copy_process>:
ffffffff802414c0: 67bdff40 daddiu sp,sp,-192
ffffffff802414c4: ffbe00b0 sd s8,176(sp)
ffffffff802414c8: 03a0f02d move s8,sp
ffffffff802414cc: ffbf00b8 sd ra,184(sp)
ffffffff802414d0: ffb700a8 sd s7,168(sp)
ffffffff802414d4: ffb600a0 sd s6,160(sp)
ffffffff802414d8: ffb50098 sd s5,152(sp)
ffffffff802414dc: ffb40090 sd s4,144(sp)
ffffffff802414e0: ffb30088 sd s3,136(sp)
ffffffff802414e4: ffb20080 sd s2,128(sp)
ffffffff802414e8: ffb10078 sd s1,120(sp)
ffffffff802414ec: ffb00070 sd s0,112(sp)
ffffffff802414f0: 3c038021 lui v1,0x8021
ffffffff802414f4: 646316b0 daddiu v1,v1,5808
ffffffff802414f8: 03e0082d move at,ra
ffffffff802414fc: 0060f809 jalr v1
ffffffff80241500: 00020021 nop
ip -->

At first, we move to "lui, v1, HI_16BIT_OF_MCOUNT", ip = ip - 12(not 8
when without -mlong-calls, i need to update the source code later).

and then, we check whether there is a "Store" instruction, if it's not a
"Store" instruction, the function should be a leaf? otherwise, we
continue the searching until finding the "s{d,w} ra, offset(sp)"
instruction, get the offset, calculate the stack address, and finish?

So, we just need to replace this:

if ((code & MOV_FP_SP) == MOV_FP_SP)
return parent_addr;

by

#define S_INSN (0xafb0 << 16)

if ((code & S_INSN) != S_INSN)
return parent_addr;

>
> > +
> > + sp = fp + (code & STACK_OFFSET_MASK);
> > + ra = *(unsigned long *)sp;
>
> Also might want to make the above into a asm with exception handling.
>
> > +
> > + if (ra == parent)
> > + return sp;
> > +
> > + ftrace_graph_stop();
> > + WARN_ON(1);
> > + return parent_addr;
>
> Hmm, may need to do more than this. See below.
>
> > +}
> > +
> > +/*
> > + * Hook the return address and push it in the stack of return addrs
> > + * in current thread info.
> > + */
> > +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
> > + unsigned long fp)
> > +{
> > + unsigned long old;
> > + struct ftrace_graph_ent trace;
> > + unsigned long return_hooker = (unsigned long)
> > + &return_to_handler;
> > +
> > + if (unlikely(atomic_read(&current->tracing_graph_pause)))
> > + return;
> > +
> > + /* "parent" is the stack address saved the return address of the caller
> > + * of _mcount, for a leaf function not save the return address in the
> > + * stack address, so, we "emulate" one in _mcount's stack space, and
> > + * hijack it directly, but for a non-leaf function, it will save the
> > + * return address to the its stack space, so, we can not hijack the
> > + * "parent" directly, but need to find the real stack address,
> > + * ftrace_get_parent_addr() does it!
> > + */
> > +
> > + old = *parent;
> > +
> > + parent = (unsigned long *)ftrace_get_parent_addr(self_addr, old,
> > + (unsigned long)parent,
> > + fp);
> > +
> > + *parent = return_hooker;
>
> Although you may have turned off fgraph tracer in
> ftrace_get_parent_addr, nothing stops the below from messing with the
> stack. The return stack may get off sync and break later. If you fail
> the above, you should not be calling the push function below.
>

We need to really stop before ftrace_push_return_trace to avoid messing
with the stack but if we have stopped the tracer, is it important to
mess with the stack or not?

Regards,
Wu Zhangjin

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

On Tue, 2009-10-27 at 00:11 +0800, Wu Zhangjin wrote:
> Hi,
>
> On Mon, 2009-10-26 at 11:13 -0400, Steven Rostedt wrote:
> [...]
> > > +
> > > + /* get the code at "ip" */
> > > + code = *(unsigned int *)ip;
> >
> > Probably want to put the above in an asm with exception handling.
> >
>
> Seems that exception handling in an asm is really "awful"(un-readable)
> and the above ip is what we have got from the ftrace_graph_caller, it
> should be okay. but if exception handling is necessary, I will send a
> new patch for the places(including the following one) which need it.

Yeah, and probably not as important in the mips world, as it is used
more with embedded devices than desktops. We must always take the
"paranoid" approach for tracing. At least in PPC and x86, we assume
everything is broken And we want to be as robust as possible. If
something goes wrong, we want to detect it ASAP and report it. And keep
the system from crashing.

At least with MIPS we don't need to worry about crashing Linus's
desktop. With is the #1 priority we have on x86 ... "Don't crash Linus's
desktop!".

If Linus sees a warning, he'll bitch at us. If we crash his box, and he
was to lose any information, he'll strip out our code!

>
> > > +
> > > + /* If we hit the "move s8(fp), sp" instruction before finding
> > > + * where the ra is stored, then this is a leaf function and it
> > > + * does not store the ra on the stack. */
> > > + if ((code & MOV_FP_SP) == MOV_FP_SP)
> > > + return parent_addr;
> > > + } while (((code & S_RA) != S_RA));
> >
> > Hmm, that condition also looks worrisome. Should we just always search
> > for s{d,w} R,X(sp)?
> >
> > Since there should only be stores of registers into the sp above the
> > jump to mcount. The break out loop is a check for move. I think it would
> > be safer to have the break out loop is a check for non storing of a
> > register into SP.
>
>
> Okay, let's look at this with -mlong-calls,
>
> leaf function:
>
> ffffffff80243cd8 <oops_may_print>:
> ffffffff80243cd8: 67bdfff0 daddiu sp,sp,-16
> ffffffff80243cdc: ffbe0008 sd s8,8(sp)
> ffffffff80243ce0: 03a0f02d move s8,sp
> ffffffff80243ce4: 3c038021 lui v1,0x8021
> ffffffff80243ce8: 646316b0 daddiu v1,v1,5808
> ffffffff80243cec: 03e0082d move at,ra
> ffffffff80243cf0: 0060f809 jalr v1
> ffffffff80243cf4: 00020021 nop
>
> non-leaf function:
>
> ffffffff802414c0 <copy_process>:
> ffffffff802414c0: 67bdff40 daddiu sp,sp,-192
> ffffffff802414c4: ffbe00b0 sd s8,176(sp)
> ffffffff802414c8: 03a0f02d move s8,sp
> ffffffff802414cc: ffbf00b8 sd ra,184(sp)
> ffffffff802414d0: ffb700a8 sd s7,168(sp)
> ffffffff802414d4: ffb600a0 sd s6,160(sp)
> ffffffff802414d8: ffb50098 sd s5,152(sp)
> ffffffff802414dc: ffb40090 sd s4,144(sp)
> ffffffff802414e0: ffb30088 sd s3,136(sp)
> ffffffff802414e4: ffb20080 sd s2,128(sp)
> ffffffff802414e8: ffb10078 sd s1,120(sp)
> ffffffff802414ec: ffb00070 sd s0,112(sp)
> ffffffff802414f0: 3c038021 lui v1,0x8021
> ffffffff802414f4: 646316b0 daddiu v1,v1,5808
> ffffffff802414f8: 03e0082d move at,ra
> ffffffff802414fc: 0060f809 jalr v1
> ffffffff80241500: 00020021 nop
> ip -->
>
> At first, we move to "lui, v1, HI_16BIT_OF_MCOUNT", ip = ip - 12(not 8
> when without -mlong-calls, i need to update the source code later).

Yes with -mlong-calls you must jump pass the setting up of the jalr.

>
> and then, we check whether there is a "Store" instruction, if it's not a
> "Store" instruction, the function should be a leaf? otherwise, we
> continue the searching until finding the "s{d,w} ra, offset(sp)"
> instruction, get the offset, calculate the stack address, and finish?

Note, you are commenting different than your code. Your code matches
more what I want than your comments You keep saying "if the
instruction just after the jump to mcount (and preparation for) is not a
store than it is a leaf", where I'm saying (and the code matches),
search above the jump to mcount and if we don't find the store of ra,
then it is a leaf.

>
> So, we just need to replace this:
>
> if ((code & MOV_FP_SP) == MOV_FP_SP)
> return parent_addr;
>
> by
>
> #define S_INSN (0xafb0 << 16)
>
> if ((code & S_INSN) != S_INSN)
> return parent_addr;

I would be even more paranoid, and make sure each of those stores, store
into sp.

>
> >
> > > +
> > > + sp = fp + (code & STACK_OFFSET_MASK);
> > > + ra = *(unsigned long *)sp;
> >
> > Also might want to make the above into a asm with exception handling.
> >
> > > +
> > > + if (ra == parent)
> > > + return sp;
> > > +
> > > + ftrace_graph_stop();
> > > + WARN_ON(1);
> > > + return parent_addr;
> >
> > Hmm, may need to do more than this. See below.
> >
> > > +}
> > > +
> > > +/*
> > > + * Hook the return address and push it in the stack of return addrs
> > > + * in current thread info.
> > > + */
> > > +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
> > > + unsigned long fp)
> > > +{
> > > + unsigned long old;
> > > + struct ftrace_graph_ent trace;
> > > + unsigned long return_hooker = (unsigned long)
> > > + &return_to_handler;
> > > +
> > > + if (unlikely(atomic_read(&current->tracing_graph_pause)))
> > > + return;
> > > +
> > > + /* "parent" is the stack address saved the return address of the caller
> > > + * of _mcount, for a leaf function not save the return address in the
> > > + * stack address, so, we "emulate" one in _mcount's stack space, and
> > > + * hijack it directly, but for a non-leaf function, it will save the
> > > + * return address to the its stack space, so, we can not hijack the
> > > + * "parent" directly, but need to find the real stack address,
> > > + * ftrace_get_parent_addr() does it!
> > > + */
> > > +
> > > + old = *parent;
> > > +
> > > + parent = (unsigned long *)ftrace_get_parent_addr(self_addr, old,
> > > + (unsigned long)parent,
> > > + fp);
> > > +
> > > + *parent = return_hooker;
> >
> > Although you may have turned off fgraph tracer in
> > ftrace_get_parent_addr, nothing stops the below from messing with the
> > stack. The return stack may get off sync and break later. If you fail
> > the above, you should not be calling the push function below.
> >
>
> We need to really stop before ftrace_push_return_trace to avoid messing
> with the stack but if we have stopped the tracer, is it important to
> mess with the stack or not?

The ftrace_push_return_trace does not test if the trace stopped, that is
expected to be done by the caller. If you mess with the stack set up,
you will crash the box. Remember, before the failure, you could have
already replaced return jumps. Those will still be falling back to the
return_to_handler. If you mess with the stack, but don't update the
return, the other returns will be out of sync and call the wrong return
address.

-- Steve


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

Hi,

[...]
>
> Yeah, and probably not as important in the mips world, as it is used
> more with embedded devices than desktops. We must always take the
> "paranoid" approach for tracing. At least in PPC and x86, we assume
> everything is broken And we want to be as robust as possible. If
> something goes wrong, we want to detect it ASAP and report it. And keep
> the system from crashing.
>
> At least with MIPS we don't need to worry about crashing Linus's
> desktop. With is the #1 priority we have on x86 ... "Don't crash Linus's
> desktop!".
>
> If Linus sees a warning, he'll bitch at us. If we crash his box, and he
> was to lose any information, he'll strip out our code!
>

Okay, a new patch for all of the exception handling will go into -v7.

>
> >
> > So, we just need to replace this:
> >
> > if ((code & MOV_FP_SP) == MOV_FP_SP)
> > return parent_addr;
> >
> > by
> >
> > #define S_INSN (0xafb0 << 16)
> >
> > if ((code & S_INSN) != S_INSN)
> > return parent_addr;
>
> I would be even more paranoid, and make sure each of those stores, store
> into sp.

get it

(I need to be more paranoid too, otherwise, Steven will not accept my
patches!)

>
> >
> > >
> > > > +
> > > > + sp = fp + (code & STACK_OFFSET_MASK);
> > > > + ra = *(unsigned long *)sp;
> > >
> > > Also might want to make the above into a asm with exception handling.
> > >
> > > > +
> > > > + if (ra == parent)
> > > > + return sp;
> > > > +
> > > > + ftrace_graph_stop();
> > > > + WARN_ON(1);
> > > > + return parent_addr;
> > >
> > > Hmm, may need to do more than this. See below.
> > >
[...]
> > > > +
> > > > + old = *parent;
> > > > +
> > > > + parent = (unsigned long *)ftrace_get_parent_addr(self_addr, old,
> > > > + (unsigned long)parent,
> > > > + fp);
> > > > +
> > > > + *parent = return_hooker;
> > >
> > > Although you may have turned off fgraph tracer in
> > > ftrace_get_parent_addr, nothing stops the below from messing with the
> > > stack. The return stack may get off sync and break later. If you fail
> > > the above, you should not be calling the push function below.
> > >
> >
> > We need to really stop before ftrace_push_return_trace to avoid messing
> > with the stack but if we have stopped the tracer, is it important to
> > mess with the stack or not?
>
> The ftrace_push_return_trace does not test if the trace stopped, that is
> expected to be done by the caller. If you mess with the stack set up,
> you will crash the box. Remember, before the failure, you could have
> already replaced return jumps. Those will still be falling back to the
> return_to_handler. If you mess with the stack, but don't update the
> return, the other returns will be out of sync and call the wrong return
> address.
>

As you can see, after stopping the function graph tracer(here the function is non-leaf)
with ftrace_graph_stop() in ftrace_get_parent_addr(), I return the old parent_addr,
this is only the stack address in the stack space of ftrace_graph_caller, which means
that, I never touch the real stack address of the non-leaf function, and it will not trap
into the return_to_handler hooker 'Cause the non-leaf function will load it's own normal
return address from it's own stack, and then just return back normally.
-- This is another trick

Regards,
Wu Zhangjin

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

On Tue, 2009-10-27 at 00:57 +0800, Wu Zhangjin wrote:

> > I would be even more paranoid, and make sure each of those stores, store
> > into sp.
>
> get it
>
> (I need to be more paranoid too, otherwise, Steven will not accept my
> patches!)

Sure I would accept them. I don't know of any MIPS boxes that Linus
runs. So I'm not afraid of crashing his boxes with these patches

> > >
> > > We need to really stop before ftrace_push_return_trace to avoid messing
> > > with the stack but if we have stopped the tracer, is it important to
> > > mess with the stack or not?
> >
> > The ftrace_push_return_trace does not test if the trace stopped, that is
> > expected to be done by the caller. If you mess with the stack set up,
> > you will crash the box. Remember, before the failure, you could have
> > already replaced return jumps. Those will still be falling back to the
> > return_to_handler. If you mess with the stack, but don't update the
> > return, the other returns will be out of sync and call the wrong return
> > address.
> >
>
> As you can see, after stopping the function graph tracer(here the function is non-leaf)
> with ftrace_graph_stop() in ftrace_get_parent_addr(), I return the old parent_addr,
> this is only the stack address in the stack space of ftrace_graph_caller, which means
> that, I never touch the real stack address of the non-leaf function, and it will not trap
> into the return_to_handler hooker 'Cause the non-leaf function will load it's own normal
> return address from it's own stack, and then just return back normally.

But then you should not be calling the push function. That will still
push onto the graph stack.

The function graph tracer keeps a separate return stack
(current->ret_stack). This is what holds the return addresses.


(normal operation)

func1
jalr _mcount

push ra onto ret_stack
replace ra with return_to_handler

jr ra --> return_to_handler


return_to_handler

pop ret_stack, have original ra
jr original_ra


Now what happens if we fail a call but still push to ret_stack

func1
jalr _mcount

(success)
push ra onto ret_stack
replace ra with return_to_handler

jalr func2

func2
jalr _mcount

(failed)
push ra onto ret_stack <<-- this is wrong!
keep original ra

jr ra << does not call tracer function!!!

jr ra << goes to return_to_handler


return_to_handler

pop ra from ret_stack <<--- has func2 ra not func1 ra!!

jr func1_ra

**** CRASH ****

Make sense?

-- Steve



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

2009/10/26 Wu Zhangjin <wuzhangjin RemoveThis @gmail.com>:
> ooh, Sorry, only this patch added(I stopped after fixing the compiling
> errors, no more check! so lazy a guy!).
>
> Just checked the source code of MIPS, the do_IRQ() is defined as a
> macro, so, I must move the macro to a C file, and also, there is a
> irq_enter...irq_exit block in a "big" function, I need to split it out.
>
> [...]
> /*
>  * do_IRQ handles all normal device IRQ's (the special
>  * SMP cross-CPU interrupts have their own specific
>  * handlers).
>  *
>  * Ideally there should be away to get this into kernel/irq/handle.c to
>  * avoid the overhead of a call for just a tiny function ...
>  */
> #define do_IRQ(irq)
> \
> do {
> \
>        irq_enter();
> \
>        __DO_IRQ_SMTC_HOOK(irq);
> \
>        generic_handle_irq(irq);
> \
>        irq_exit();
> \
> } while (0)
> [...]
>
> But The comment told us: do not make this tiny function be a standalone
> function, so???


I can't check that currently. But may be the caller of this m


>
> the same to do_IRQ_no_affinity.
>
> and, about the following function, I need to split the
> irq_enter()...irq_exit() block out.
>
> void ipi_decode(struct smtc_ipi *pipi)
> {
>        [...]
>        switch (type_copy) {
>        case SMTC_CLOCK_TICK:
>                irq_enter();
>                kstat_incr_irqs_this_cpu(irq, irq_to_desc(irq));
>                cd = &per_cpu(mips_clockevent_device, cpu);
>                cd->event_handler(cd);
>                irq_exit();
>                break;
>
>        case LINUX_SMP_IPI:
>                switch ((int)arg_copy) {
>                case SMP_RESCHEDULE_YOURSELF:
>                        ipi_resched_interrupt();
>                        break;
>                case SMP_CALL_FUNCTION:
>                        ipi_call_interrupt();
>                        break;
>        [...]
>
> Regards,
>        Wu Zhangjin
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

2009/10/27 Frederic Weisbecker <fweisbec.TakeThisOut@gmail.com>:
> 2009/10/26 Wu Zhangjin <wuzhangjin.TakeThisOut@gmail.com>:
>> ooh, Sorry, only this patch added(I stopped after fixing the compiling
>> errors, no more check! so lazy a guy!).
>>
>> Just checked the source code of MIPS, the do_IRQ() is defined as a
>> macro, so, I must move the macro to a C file, and also, there is a
>> irq_enter...irq_exit block in a "big" function, I need to split it out.
>>
>> [...]
>> /*
>>  * do_IRQ handles all normal device IRQ's (the special
>>  * SMP cross-CPU interrupts have their own specific
>>  * handlers).
>>  *
>>  * Ideally there should be away to get this into kernel/irq/handle.c to
>>  * avoid the overhead of a call for just a tiny function ...
>>  */
>> #define do_IRQ(irq)
>> \
>> do {
>> \
>>        irq_enter();
>> \
>>        __DO_IRQ_SMTC_HOOK(irq);
>> \
>>        generic_handle_irq(irq);
>> \
>>        irq_exit();
>> \
>> } while (0)
>> [...]
>>
>> But The comment told us: do not make this tiny function be a standalone
>> function, so???
>
>
> I can't check that currently. But may be the caller of this m


Sorry, my message has been sent in the middle. I'm dealing with a
strange keyboard where
I am (and also with my strange hands).

So, may be the caller of this macro can take the irqentry tag?


>
>
>>
>> the same to do_IRQ_no_affinity.
>>
>> and, about the following function, I need to split the
>> irq_enter()...irq_exit() block out.
>>
>> void ipi_decode(struct smtc_ipi *pipi)
>> {
>>        [...]
>>        switch (type_copy) {
>>        case SMTC_CLOCK_TICK:
>>                irq_enter();
>>                kstat_incr_irqs_this_cpu(irq, irq_to_desc(irq));
>>                cd = &per_cpu(mips_clockevent_device, cpu);
>>                cd->event_handler(cd);
>>                irq_exit();
>>                break;
>>
>>        case LINUX_SMP_IPI:
>>                switch ((int)arg_copy) {
>>                case SMP_RESCHEDULE_YOURSELF:
>>                        ipi_resched_interrupt();
>>                        break;
>>                case SMP_CALL_FUNCTION:
>>                        ipi_call_interrupt();
>>                        break;
>>        [...]
>>


Oh right, this one is more tricky. Well, if that's important for
someone, we can deal with that later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Mon, Oct 26, 2009 at 05:42:36PM +0800, Wu Zhangjin wrote:
> On Mon, 2009-10-26 at 01:27 +0100, Frederic Weisbecker wrote:
> > 2009/10/25 Wu Zhangjin <wuzhangjin.DeleteThis@gmail.com>:
> > > -static inline u64 mips_timecounter_read(void)
> > > +static inline u64 notrace mips_timecounter_read(void)
> >
> >
> > You don't need to set notrace functions, unless their addresses
> > are referenced somewhere, which unfortunately might happen
> > for some functions but this is rare.
> >
>
> Okay, Will remove it.



Oops, a word has escaped from my above sentence. I wanted to say:

"You don't need to set notrace to inline functions"


> > Hmm yeah this is not very nice to do that in core functions because
> > of a specific arch problem.
> > At least you have __notrace_funcgraph, this is a notrace
> > that only applies if CONFIG_FUNCTION_GRAPH_TRACER
> > so that it's still traceable by the function tracer in this case.
> >
> > But I would rather see a __mips_notrace on these two core functions.
>
> What about this: __arch_notrace? If the arch need this, define it,
> otherwise, ignore it! if only graph tracer need it, define it in "#ifdef
> CONFIG_FUNCTION_GRAPH_TRACER ... #endif".



The problem is that archs may want to disable tracing on different
places.
For example mips wants to disable tracing in timecounter_read_delta,
but another arch may want to disable tracing somewhere else.

We'll then have several unrelated __arch_notrace. One that is relevant
for mips, another that is relevant for arch_foo, but all of them will
apply for all arch that have defined a __arch_notrace.

It's true that __mips_notrace is not very elegant as it looks like
a specific arch annotation intruder.

But at least that gives us a per arch filter granularity.

If only static ftrace could disappear, we could keep only dynamic
ftrace and we would then be able to filter dynamically.
But I'm not sure it's a good idea for archs integration.

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

Hi,

On Mon, 2009-11-02 at 22:43 +0100, Frederic Weisbecker wrote:
[...]
> > > > -static inline u64 mips_timecounter_read(void)
> > > > +static inline u64 notrace mips_timecounter_read(void)
> > >
> > >
> > > You don't need to set notrace functions, unless their addresses
> > > are referenced somewhere, which unfortunately might happen
> > > for some functions but this is rare.
> > >
> >
> > Okay, Will remove it.
>
>
>
> Oops, a word has escaped from my above sentence. I wanted to say:
>
> "You don't need to set notrace to inline functions"
>
>

Thanks

I have got your meaning at that time, and have removed them with inline
functions.

> > > But I would rather see a __mips_notrace on these two core functions.
> >
> > What about this: __arch_notrace? If the arch need this, define it,
> > otherwise, ignore it! if only graph tracer need it, define it in "#ifdef
> > CONFIG_FUNCTION_GRAPH_TRACER ... #endif".
>
> The problem is that archs may want to disable tracing on different
> places.
> For example mips wants to disable tracing in timecounter_read_delta,
> but another arch may want to disable tracing somewhere else.
>
> We'll then have several unrelated __arch_notrace. One that is relevant
> for mips, another that is relevant for arch_foo, but all of them will
> apply for all arch that have defined a __arch_notrace.
>
> It's true that __mips_notrace is not very elegant as it looks like
> a specific arch annotation intruder.
>
> But at least that gives us a per arch filter granularity.
>
> If only static ftrace could disappear, we could keep only dynamic
> ftrace and we would then be able to filter dynamically.
> But I'm not sure it's a good idea for archs integration.
>

Got it.

Thanks & Regards,
Wu Zhangjin

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

Hi,

On Mon, 2009-11-02 at 22:43 +0100, Frederic Weisbecker wrote:
[...]
> > >
> > > But I would rather see a __mips_notrace on these two core functions.
> >
> > What about this: __arch_notrace? If the arch need this, define it,
> > otherwise, ignore it! if only graph tracer need it, define it in "#ifdef
> > CONFIG_FUNCTION_GRAPH_TRACER ... #endif".
>
>
>
> The problem is that archs may want to disable tracing on different
> places.
> For example mips wants to disable tracing in timecounter_read_delta,
> but another arch may want to disable tracing somewhere else.
>
> We'll then have several unrelated __arch_notrace. One that is relevant
> for mips, another that is relevant for arch_foo, but all of them will
> apply for all arch that have defined a __arch_notrace.
>
> It's true that __mips_notrace is not very elegant as it looks like
> a specific arch annotation intruder.
>
>
> But at least that gives us a per arch filter granularity.
>
> If only static ftrace could disappear, we could keep only dynamic
> ftrace and we would then be able to filter dynamically.
> But I'm not sure it's a good idea for archs integration.
>

I think if we use something like __mips_notrace here, we may get lots of
__ARCH_notraces here too, 'Cause some other platforms(at least, as I
know, Microblaze will do it too) may also need to add one here, it will
become:

__mips_notrace __ARCH1_notrace __ARCH2_notrace .... foo() {...}

A little ugly

and If a new platform need it's __ARCH_notrace, they need to touch the
common part of ftrace, more side-effects!

but with __arch_notrace, the archs only need to touch it's own part,
Although there is a side-effect as you mentioned above

So, what should we do?

Regards,
Wu Zhangjin

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

On Mon, Nov 09, 2009 at 12:31:12PM +0800, Wu Zhangjin wrote:
> I think if we use something like __mips_notrace here, we may get lots of
> __ARCH_notraces here too, 'Cause some other platforms(at least, as I
> know, Microblaze will do it too) may also need to add one here, it will
> become:
>
> __mips_notrace __ARCH1_notrace __ARCH2_notrace .... foo() {...}
>
> A little ugly


Yeah
I thought Mips would be the only one to do that.


> and If a new platform need it's __ARCH_notrace, they need to touch the
> common part of ftrace, more side-effects!
>
> but with __arch_notrace, the archs only need to touch it's own part,
> Although there is a side-effect as you mentioned above
>
> So, what should we do?
>
> Regards,
> Wu Zhangjin
>

Why not __time ?
As it's normal that such few functions that are used to read the timecounter
have fair chances to be __no_trace on archs like MIPS. Interested
archs would just need to override a default stub __time.

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

On Mon, 2009-11-09 at 12:53 +0100, Frederic Weisbecker wrote:
> On Mon, Nov 09, 2009 at 12:31:12PM +0800, Wu Zhangjin wrote:
> > I think if we use something like __mips_notrace here, we may get lots of
> > __ARCH_notraces here too, 'Cause some other platforms(at least, as I
> > know, Microblaze will do it too) may also need to add one here, it will
> > become:
> >
> > __mips_notrace __ARCH1_notrace __ARCH2_notrace .... foo() {...}
> >
> > A little ugly
>
>
> Yeah
> I thought Mips would be the only one to do that.
>
>
> > and If a new platform need it's __ARCH_notrace, they need to touch the
> > common part of ftrace, more side-effects!
> >
> > but with __arch_notrace, the archs only need to touch it's own part,
> > Although there is a side-effect as you mentioned above
> >
> > So, what should we do?
> >
> > Regards,
> > Wu Zhangjin
> >
>
> Why not __time ?
> As it's normal that such few functions that are used to read the timecounter
> have fair chances to be __no_trace on archs like MIPS. Interested
> archs would just need to override a default stub __time.
>

Good pointer, will apply it

Regards,
Wu Zhangjin

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

On Mon, 2009-11-09 at 12:31 +0800, Wu Zhangjin wrote:

> I think if we use something like __mips_notrace here, we may get lots of
> __ARCH_notraces here too, 'Cause some other platforms(at least, as I
> know, Microblaze will do it too) may also need to add one here, it will
> become:
>
> __mips_notrace __ARCH1_notrace __ARCH2_notrace .... foo() {...}
>
> A little ugly

I agree, that is ugly.

>
> and If a new platform need it's __ARCH_notrace, they need to touch the
> common part of ftrace, more side-effects!
>
> but with __arch_notrace, the archs only need to touch it's own part,
> Although there is a side-effect as you mentioned above
>
> So, what should we do?

Just do it in the Makefile. We can add __arch_notrace, and then in the
Makefile define it with the arch.

ifeq ($(ARCH), MIPS)
CFLAGS_foo.o = -D__arch_notrace=notrace
endif

And we can simply define __arch_notrace in a header:

#ifndef __arch_notrace
# define __arch_notrace
#endif

I much rather uglify the Makefile than the code.

-- Steve


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