Re: race between flush_to_ldisc and pty_cleanup

[Keun-O Park]

Hi,

02/01/2019 07:31 PM에 Maninder Singh 이(가) 쓴 글:
> Hi,
>
>> On Fri, Feb 01, 2019 at 07:03:26PM +0530, Maninder Singh wrote:
>>> Hi,
>>>
>>>
>>> There is some race condition between tty_port_put and flush_to_ldisc
>>> which lead to use after free case:
>>> (Kernel 4.1)
>>>
>>> [1403.5130] Unable to handle kernel paging request at virtual address 
>>> 6b6b6b83
>>> ...
>>> ...
>>> ...
>>>
>>> [1403.5132] [] (ldsem_down_read_trylock) from [] 
>>> (tty_ldisc_ref+0x24/0x60)
>>> [1403.5132] [] (tty_ldisc_ref) from [] 
>>> (flush_to_ldisc+0x6c/0x21c)
>>> [1403.5132]  r5:dbcd4a84 r4:
>>> [1403.5132] [] (flush_to_ldisc) from [] 
>>> (process_one_work+0x214/0x570)
>>> [1403.5132]  r10: r9:ddab r8:e3d6e000 r7: r6:e453f740 
>>> r5:cb37b780
>>> [1403.5132]  r4:dbcd4a84
>>> [1403.5132] [] (process_one_work) from [] 
>>> (worker_thread+0x60/0x580)
>>> [1403.5132]  r10:e453f740 r9:ddab r8:e453f764 r7:0088 r6:e453f740 
>>> r5:cb37b798
>>> [1403.5132]  r4:cb37b780
>>> [1403.5132] [] (worker_thread) from [] 
>>> (kthread+0xec/0x104)
>>> [1403.5132]  r10: r9: r8: r7:c004a274 r6:cb37b780 
>>> r5:d8a3fc80
>>> [1403.5132]  r4:
>>> [1403.5132] [] (kthread) from [] 
>>> (ret_from_fork+0x14/0x3c)
>>>
>>>
>>> for checking further we entered some debug prints and added delay in 
>>> flush_to_ldisc to reproduce
>>> and seems there is some issue with workqueue implementation of TTY:
>>>
>>> bool tty_buffer_cancel_work(struct tty_port *port)
>>> {
>>> bool ret;
>>> ret = cancel_work_sync(>buf.work); // Check return value of 
>>> cancel_work_sync
>>> pr_emerg("Work cancelled is 0x%x %pS %d\n", (unsigned 
>>> int)>buf.work, (void *)_RET_IP_, ret);
>>> return ret;
>>> }
>>>
>>> static void flush_to_ldisc(struct work_struct *work)
>>> {
>>> ...
>>> mdelay(100);   // Added Delay to reproduce race
>>>
>>> if (flag_work_cancel) {
>>> pr_emerg("scheduled work after stopping work %x\n", 
>>> (unsigned int)work);
>>>
>>> 
>>> }
>>>
>>> static void pty_cleanup(struct tty_struct *tty)
>>> {
>>> ...
>>> flag_work_cancel = 1;
>>> ...
>>> }
>>>
>>>
>>> [1403.4158]Work cancelled is dbcd4a84 tty_port_destroy+0x1c/0x6c  0   
>>> // Since return is 0 so no work is pending
>>>
>>> [1403.5129] scheduled work after stopping work dbcd4a84// Still same 
>>> work is scheduled after cancelled
>>> [1403.5130] Unable to handle kernel paging request at virtual address 
>>> 6b6b6b83   // Kernel OOPs occured because of use after free
>> Ok, after my initial "use a newer kernel" comment, this really does look
>> strange.  There has also been a lot of workqueue fixes and rework since
>> 4.1, and that might be the thing that fixes this issue here.
>>
>> However, are you sure you are not just calling flush_to_ldisc() directly
>> through some codepath somehow?  If you look at the stack in the
> Yes, there is no call path for flush_to_disc directly. It is all aligned with 
> kernel 4.1.
>
>
>> pr_emerg() message, where did it come from?  From the same workqueue
>> that you already stopped?
> We added debug prints to check "work" in pty_cleanup() & flush_to_ldisc
>
>> Testing on a newer kernel would be great, if possible.
> We are facing it hard, but currently we have 4.1 and able to reproduce on 
> that.
> Not really possible to have the latest kernel on the same target and may be 
> reproduce the same race.
> Tried to track for changes in the other stable branches, but no change looks 
> really relevant for this race.
> I might be wrong, please help if there is any commit related with this.
Please find & review the attached patch for this issue.

I think the tty which called pty_cleanup may be different from the tty
which called flush_to_ldisc.
Please check if those two are same or different. I don't think this is a
workqueue problem.
Thanks.

Best Regards,
Sahara

>
>> thanks,
>>
>> greg k-h
> Our initial debugging direction was with "tty" but looks some issue in 
> workqueue.
> Also, the most surprising looks to be th

Re: [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder

[Keun-O Park]

Hi Kees,

On Thu, Apr 5, 2018 at 3:11 AM, Kees Cook  wrote:
> [resending with the CCs I forgot...]
>
> On Thu, Mar 1, 2018 at 2:19 AM,   wrote:
>> From: Sahara 
>>
>> The old arch_within_stack_frames which used the frame pointer is
>> now reimplemented to use frame pointer unwinder apis. So the main
>> functionality is same as before.
>>
>> Signed-off-by: Sahara 
>
> This will result in slightly more expensive stack checking for
> hardened usercopy, but I think that'd be okay if this could also be
> made to be unwinder-agnostic. Then it would work for ORC too, and
> wouldn't have to depend on just FRAME_POINTER. Without that, I'm not
> sure what the benefit is in changing this?

Exactly. It's the only reason not to depend on the FRAME_POINTER only.
And, it will be better if it would work for ORC.

>
> Further notes below...
>
>> ---
>>  arch/x86/include/asm/unwind.h  |  5 +++
>>  arch/x86/kernel/stacktrace.c   | 77 
>> +-
>>  arch/x86/kernel/unwind_frame.c |  4 +--
>>  3 files changed, 60 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
>> index 1f86e1b..6f04906f 100644
>> --- a/arch/x86/include/asm/unwind.h
>> +++ b/arch/x86/include/asm/unwind.h
>> @@ -87,6 +87,11 @@ void unwind_init(void);
>>  void unwind_module_init(struct module *mod, void *orc_ip, size_t 
>> orc_ip_size,
>> void *orc, size_t orc_size);
>>  #else
>> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>> +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
>> +size_t regs_size(struct pt_regs *regs);
>> +#endif
>> +
>>  static inline void unwind_init(void) {}
>>  static inline
>>  void unwind_module_init(struct module *mod, void *orc_ip, size_t 
>> orc_ip_size,
>> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
>> index f433a33..c26eb55 100644
>> --- a/arch/x86/kernel/stacktrace.c
>> +++ b/arch/x86/kernel/stacktrace.c
>> @@ -12,6 +12,37 @@
>>  #include 
>>
>>
>> +static inline void *get_cur_frame(struct unwind_state *state)
>> +{
>> +   void *frame = NULL;
>> +
>> +#if defined(CONFIG_UNWINDER_ORC)
>> +#elif defined(CONFIG_UNWINDER_FRAME_POINTER)
>> +   if (state->regs)
>> +   frame = (void *)state->regs;
>> +   else
>> +   frame = (void *)state->bp;
>> +#else
>> +#endif
>> +   return frame;
>> +}
>
> What's going on here with the #if statement? Shouldn't this just be:
>
> +static inline void *get_cur_frame(struct unwind_state *state)
> +{
> +   void *frame = NULL;
> +
> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
> +   if (state->regs)
> +   frame = (void *)state->regs;
> +   else
> +   frame = (void *)state->bp;
> +#endif
> +   return frame;
> +}
>
> ?

Removed the unused #ifdef.



>
>> +
>> +static inline void *get_frame_end(struct unwind_state *state)
>> +{
>> +   void *frame_end = NULL;
>> +
>> +#if defined(CONFIG_UNWINDER_ORC)
>> +#elif defined(CONFIG_UNWINDER_FRAME_POINTER)
>> +   if (state->regs) {
>> +   frame_end = (void *)state->regs + regs_size(state->regs);
>> +   } else {
>> +   frame_end = (void *)state->bp + FRAME_HEADER_SIZE;
>> +   }
>> +#else
>> +#endif
>> +   return frame_end;
>> +}
>
> Same thing above?

Removed the unused #ifdef.

>
>> +
>>  /*
>>   * Walks up the stack frames to make sure that the specified object is
>>   * entirely contained by a single stack frame.
>> @@ -25,31 +56,31 @@ int arch_within_stack_frames(const void * const stack,
>>  const void * const stackend,
>>  const void *obj, unsigned long len)
>>  {
>> -#if defined(CONFIG_FRAME_POINTER)
>> -   const void *frame = NULL;
>> -   const void *oldframe;
>> -
>> -   oldframe = __builtin_frame_address(2);
>> -   if (oldframe)
>> -   frame = __builtin_frame_address(3);
>> +#if defined(CONFIG_UNWINDER_FRAME_POINTER)
>> +   struct unwind_state state;
>> +   void *prev_frame_end = NULL;
>> /*
>> -* low --> high
>> -* [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> -* ^^
>> -*   allow copies only within here
>
> I think it's worth keeping this diagram: it explains what region is
> being checked...

Kept the comment in v2 patch.


>
>> +* Skip 3 non-inlined frames: arch_within_stack_frames(),
>> +* check_stack_object() and __check_object_size().
>> +*
>>  */
>> -   while (stack <= frame && frame < stackend) {
>> -   /*
>> -* If obj + len extends past the last frame, this
>> -* check won't pass and the next frame will be 0,
>> -* causing us to bail out and correctly report
>> - 

Re: [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder

[Keun-O Park]

Hi Kees,

On Thu, Apr 5, 2018 at 3:11 AM, Kees Cook  wrote:
> [resending with the CCs I forgot...]
>
> On Thu, Mar 1, 2018 at 2:19 AM,   wrote:
>> From: Sahara 
>>
>> The old arch_within_stack_frames which used the frame pointer is
>> now reimplemented to use frame pointer unwinder apis. So the main
>> functionality is same as before.
>>
>> Signed-off-by: Sahara 
>
> This will result in slightly more expensive stack checking for
> hardened usercopy, but I think that'd be okay if this could also be
> made to be unwinder-agnostic. Then it would work for ORC too, and
> wouldn't have to depend on just FRAME_POINTER. Without that, I'm not
> sure what the benefit is in changing this?

Exactly. It's the only reason not to depend on the FRAME_POINTER only.
And, it will be better if it would work for ORC.

>
> Further notes below...
>
>> ---
>>  arch/x86/include/asm/unwind.h  |  5 +++
>>  arch/x86/kernel/stacktrace.c   | 77 
>> +-
>>  arch/x86/kernel/unwind_frame.c |  4 +--
>>  3 files changed, 60 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
>> index 1f86e1b..6f04906f 100644
>> --- a/arch/x86/include/asm/unwind.h
>> +++ b/arch/x86/include/asm/unwind.h
>> @@ -87,6 +87,11 @@ void unwind_init(void);
>>  void unwind_module_init(struct module *mod, void *orc_ip, size_t 
>> orc_ip_size,
>> void *orc, size_t orc_size);
>>  #else
>> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>> +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
>> +size_t regs_size(struct pt_regs *regs);
>> +#endif
>> +
>>  static inline void unwind_init(void) {}
>>  static inline
>>  void unwind_module_init(struct module *mod, void *orc_ip, size_t 
>> orc_ip_size,
>> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
>> index f433a33..c26eb55 100644
>> --- a/arch/x86/kernel/stacktrace.c
>> +++ b/arch/x86/kernel/stacktrace.c
>> @@ -12,6 +12,37 @@
>>  #include 
>>
>>
>> +static inline void *get_cur_frame(struct unwind_state *state)
>> +{
>> +   void *frame = NULL;
>> +
>> +#if defined(CONFIG_UNWINDER_ORC)
>> +#elif defined(CONFIG_UNWINDER_FRAME_POINTER)
>> +   if (state->regs)
>> +   frame = (void *)state->regs;
>> +   else
>> +   frame = (void *)state->bp;
>> +#else
>> +#endif
>> +   return frame;
>> +}
>
> What's going on here with the #if statement? Shouldn't this just be:
>
> +static inline void *get_cur_frame(struct unwind_state *state)
> +{
> +   void *frame = NULL;
> +
> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
> +   if (state->regs)
> +   frame = (void *)state->regs;
> +   else
> +   frame = (void *)state->bp;
> +#endif
> +   return frame;
> +}
>
> ?

Removed the unused #ifdef.



>
>> +
>> +static inline void *get_frame_end(struct unwind_state *state)
>> +{
>> +   void *frame_end = NULL;
>> +
>> +#if defined(CONFIG_UNWINDER_ORC)
>> +#elif defined(CONFIG_UNWINDER_FRAME_POINTER)
>> +   if (state->regs) {
>> +   frame_end = (void *)state->regs + regs_size(state->regs);
>> +   } else {
>> +   frame_end = (void *)state->bp + FRAME_HEADER_SIZE;
>> +   }
>> +#else
>> +#endif
>> +   return frame_end;
>> +}
>
> Same thing above?

Removed the unused #ifdef.

>
>> +
>>  /*
>>   * Walks up the stack frames to make sure that the specified object is
>>   * entirely contained by a single stack frame.
>> @@ -25,31 +56,31 @@ int arch_within_stack_frames(const void * const stack,
>>  const void * const stackend,
>>  const void *obj, unsigned long len)
>>  {
>> -#if defined(CONFIG_FRAME_POINTER)
>> -   const void *frame = NULL;
>> -   const void *oldframe;
>> -
>> -   oldframe = __builtin_frame_address(2);
>> -   if (oldframe)
>> -   frame = __builtin_frame_address(3);
>> +#if defined(CONFIG_UNWINDER_FRAME_POINTER)
>> +   struct unwind_state state;
>> +   void *prev_frame_end = NULL;
>> /*
>> -* low --> high
>> -* [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> -* ^^
>> -*   allow copies only within here
>
> I think it's worth keeping this diagram: it explains what region is
> being checked...

Kept the comment in v2 patch.


>
>> +* Skip 3 non-inlined frames: arch_within_stack_frames(),
>> +* check_stack_object() and __check_object_size().
>> +*
>>  */
>> -   while (stack <= frame && frame < stackend) {
>> -   /*
>> -* If obj + len extends past the last frame, this
>> -* check won't pass and the next frame will be 0,
>> -* causing us to bail out and correctly report
>> -* the copy as invalid.
>> -*/
>
> Also seems like we should keep the comment 

Re: [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames

[Keun-O Park]

Hi Kees,

On Thu, Apr 5, 2018 at 2:55 AM, Kees Cook  wrote:
> On Thu, Mar 1, 2018 at 2:19 AM,   wrote:
>> From: James Morse 
>>
>> This implements arch_within_stack_frames() for arm64 that should
>> validate if a given object is contained by a kernel stack frame.
>>
>> Signed-off-by: James Morse 
>> Reviewed-by: Sahara 
>
> Looks good to me. Does this end up passing the LKDTM
> USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM tests?

Yes, this passes those two LKDTM tests.
Thanks.

BR
Sahara

Re: [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames

[Keun-O Park]

Hi Kees,

On Thu, Apr 5, 2018 at 2:55 AM, Kees Cook  wrote:
> On Thu, Mar 1, 2018 at 2:19 AM,   wrote:
>> From: James Morse 
>>
>> This implements arch_within_stack_frames() for arm64 that should
>> validate if a given object is contained by a kernel stack frame.
>>
>> Signed-off-by: James Morse 
>> Reviewed-by: Sahara 
>
> Looks good to me. Does this end up passing the LKDTM
> USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM tests?

Yes, this passes those two LKDTM tests.
Thanks.

BR
Sahara

Re: [PATCH] pty: cancel pty slave port buf's work in tty_release

[Keun-O Park]

On Wed, Dec 13, 2017 at 12:23 PM, Greg KH  wrote:
> On Wed, Dec 13, 2017 at 09:10:48AM +0400, kpark3...@gmail.com wrote:
>> From: Sahara 
>
> I need a "Full" name here, I doubt you sign legal documents with just
> the single name, right?

For a long time, I have been using this single name as my signature in
my company, ex-company, and open communities.
If you don't mind, I want to use this single name in this life.


>
>>
>> In case that CONFIG_SLUB_DEBUG is on and pty is used, races between
>> release_one_tty and flush_to_ldisc work threads may happen and lead
>> to use-after-free condition on tty->link->port. Because SLUB_DEBUG
>> is turned on, freed tty->link->port is filled with POISON_FREE value.
>> So far without SLUB_DEBUG, port was filled with zero and flush_to_ldisc
>> could return without a problem by checking if tty is NULL.
>>
>> CPU 0 CPU 1
>> - -
>> release_tty   pty_write
>>cancel_work_sync(tty) to = tty->link
>>tty_kref_put(tty->link)   tty_schedule_flip(to->port)
>>   << workqueue >> ...
>>   release_one_tty ...
>>  pty_cleanup  ...
>> kfree(tty->link->port)   << workqueue >>
>>  flush_to_ldisc
>> tty = READ_ONCE(port->itty)
>> tty is 0x6b6b6b6b6b6b6b6b
>> !!PANIC!! access tty->ldisc
>>
>>  Unable to handle kernel paging request at virtual address 6b6b6b6b6b6b6b93
>>  pgd = ffc0eb1c3000
>>  [6b6b6b6b6b6b6b93] *pgd=, *pud=
>>  [ cut here ]
>>  Kernel BUG at ff800851154c [verbose debug info unavailable]
>>  Internal error: Oops - BUG: 9604 [#1] PREEMPT SMP
>>  CPU: 3 PID: 265 Comm: kworker/u8:9 Tainted: GW 3.18.31-g0a58eeb #1
>>  Hardware name: Qualcomm Technologies, Inc. MSM 8996pro v1.1 + PMI8996 
>> Carbide (DT)
>>  Workqueue: events_unbound flush_to_ldisc
>>  task: ffc0ed610ec0 ti: ffc0ed624000 task.ti: ffc0ed624000
>>  PC is at ldsem_down_read_trylock+0x0/0x4c
>>  LR is at tty_ldisc_ref+0x24/0x4c
>>  pc : [] lr : [] pstate: 80400145
>>  sp : ffc0ed627cd0
>>  x29: ffc0ed627cd0 x28: 
>>  x27: ff8009e05000 x26: ffc0d382cfa0
>>  x25:  x24: ff800a012f08
>>  x23:  x22: ffc0703fbc88
>>  x21: 6b6b6b6b6b6b6b6b x20: 6b6b6b6b6b6b6b93
>>  x19:  x18: 0001
>>  x17: 00e8f80d6f53 x16: 0001
>>  x15: 007f7d826fff x14: 00a0
>>  x13:  x12: 0109
>>  x11:  x10: 
>>  x9 : ffc0ed624000 x8 : ffc0ed611580
>>  x7 :  x6 : ff800a42e000
>>  x5 : 03fc x4 : 03bd1201
>>  x3 : 0001 x2 : 0001
>>  x1 : ff800851004c x0 : 6b6b6b6b6b6b6b93
>>
>> Signed-off-by: Sahara 
>
> Same here :)

Same comment. I hope you don't mind this.

>
>> ---
>>  drivers/tty/tty_io.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index dc60aee..a6ca634 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -1476,6 +1476,8 @@ static void release_tty(struct tty_struct *tty, int 
>> idx)
>>   if (tty->link)
>>   tty->link->port->itty = NULL;
>>   tty_buffer_cancel_work(tty->port);
>> + if (tty->link)
>> + tty_buffer_cancel_work(tty->link->port);
>
> Your oops above is from 3.18, which is a _very_ old kernel version.  Are
> you sure this isn't already fixed in latest kernel release?

Right. Unfortunately I am not sure if this is fixed in latest kernel
with other newly introduced routines.
But, logically it seems the latest kernel also has the same chance to
meet this issue by reading the code.
And, frankly I thought with your broad insight about tty/pty this
could be easily adopted or abandoned.
If not, then probably need more efforts to reproduce the same issue on
latest kernel by implementing pty test code.
Thanks.


>
> thanks,
>
> greg k-h

Re: [PATCH] pty: cancel pty slave port buf's work in tty_release

[Keun-O Park]

On Wed, Dec 13, 2017 at 12:23 PM, Greg KH  wrote:
> On Wed, Dec 13, 2017 at 09:10:48AM +0400, kpark3...@gmail.com wrote:
>> From: Sahara 
>
> I need a "Full" name here, I doubt you sign legal documents with just
> the single name, right?

For a long time, I have been using this single name as my signature in
my company, ex-company, and open communities.
If you don't mind, I want to use this single name in this life.


>
>>
>> In case that CONFIG_SLUB_DEBUG is on and pty is used, races between
>> release_one_tty and flush_to_ldisc work threads may happen and lead
>> to use-after-free condition on tty->link->port. Because SLUB_DEBUG
>> is turned on, freed tty->link->port is filled with POISON_FREE value.
>> So far without SLUB_DEBUG, port was filled with zero and flush_to_ldisc
>> could return without a problem by checking if tty is NULL.
>>
>> CPU 0 CPU 1
>> - -
>> release_tty   pty_write
>>cancel_work_sync(tty) to = tty->link
>>tty_kref_put(tty->link)   tty_schedule_flip(to->port)
>>   << workqueue >> ...
>>   release_one_tty ...
>>  pty_cleanup  ...
>> kfree(tty->link->port)   << workqueue >>
>>  flush_to_ldisc
>> tty = READ_ONCE(port->itty)
>> tty is 0x6b6b6b6b6b6b6b6b
>> !!PANIC!! access tty->ldisc
>>
>>  Unable to handle kernel paging request at virtual address 6b6b6b6b6b6b6b93
>>  pgd = ffc0eb1c3000
>>  [6b6b6b6b6b6b6b93] *pgd=, *pud=
>>  [ cut here ]
>>  Kernel BUG at ff800851154c [verbose debug info unavailable]
>>  Internal error: Oops - BUG: 9604 [#1] PREEMPT SMP
>>  CPU: 3 PID: 265 Comm: kworker/u8:9 Tainted: GW 3.18.31-g0a58eeb #1
>>  Hardware name: Qualcomm Technologies, Inc. MSM 8996pro v1.1 + PMI8996 
>> Carbide (DT)
>>  Workqueue: events_unbound flush_to_ldisc
>>  task: ffc0ed610ec0 ti: ffc0ed624000 task.ti: ffc0ed624000
>>  PC is at ldsem_down_read_trylock+0x0/0x4c
>>  LR is at tty_ldisc_ref+0x24/0x4c
>>  pc : [] lr : [] pstate: 80400145
>>  sp : ffc0ed627cd0
>>  x29: ffc0ed627cd0 x28: 
>>  x27: ff8009e05000 x26: ffc0d382cfa0
>>  x25:  x24: ff800a012f08
>>  x23:  x22: ffc0703fbc88
>>  x21: 6b6b6b6b6b6b6b6b x20: 6b6b6b6b6b6b6b93
>>  x19:  x18: 0001
>>  x17: 00e8f80d6f53 x16: 0001
>>  x15: 007f7d826fff x14: 00a0
>>  x13:  x12: 0109
>>  x11:  x10: 
>>  x9 : ffc0ed624000 x8 : ffc0ed611580
>>  x7 :  x6 : ff800a42e000
>>  x5 : 03fc x4 : 03bd1201
>>  x3 : 0001 x2 : 0001
>>  x1 : ff800851004c x0 : 6b6b6b6b6b6b6b93
>>
>> Signed-off-by: Sahara 
>
> Same here :)

Same comment. I hope you don't mind this.

>
>> ---
>>  drivers/tty/tty_io.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index dc60aee..a6ca634 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -1476,6 +1476,8 @@ static void release_tty(struct tty_struct *tty, int 
>> idx)
>>   if (tty->link)
>>   tty->link->port->itty = NULL;
>>   tty_buffer_cancel_work(tty->port);
>> + if (tty->link)
>> + tty_buffer_cancel_work(tty->link->port);
>
> Your oops above is from 3.18, which is a _very_ old kernel version.  Are
> you sure this isn't already fixed in latest kernel release?

Right. Unfortunately I am not sure if this is fixed in latest kernel
with other newly introduced routines.
But, logically it seems the latest kernel also has the same chance to
meet this issue by reading the code.
And, frankly I thought with your broad insight about tty/pty this
could be easily adopted or abandoned.
If not, then probably need more efforts to reproduce the same issue on
latest kernel by implementing pty test code.
Thanks.


>
> thanks,
>
> greg k-h

Re: [PATCH] earlyprintk: re-enable earlyprintk calling early_param

[Keun-O Park]

FYI only if you missed my previous 2 replies which didn't reach the
mailing list.
Thanks.



On Mon, Aug 18, 2014 at 12:13 PM, Sahara  wrote:
>
> 2014년 08월 16일 03:34, Rusty Russell 쓴 글:
>
>> kpark3...@gmail.com writes:
>>>
>>> From: Sahara 
>>>
>>> Although there are many obs_kernel_param and its names are
>>> earlyprintk and also EARLY_PRINTK is also enabled, we could not
>>> see the early_printk output properly until now. This patch
>>> considers earlycon as well as earlyprintk.
>>
>> Hmm, the initial "earlycon" hack slipped in when I wasn't looking.
>> I don't think we should extend it.
>>
>> Why not make the thing(s) you want early_param()s?
>>
>> Cheers,
>> Rusty.
>
> The earlycon and the earlyprintk are scattered and used in many
> architectures.
> It looks earlycon just could be a subset of earlyprintk.
> The earlycon is for uart specific, while the earlyprintk is to support vga,
> efi, xen, serial, and so on. Especially ARM uses earlyprintk in many places.
> And, I am not sure if this is a good chance to replace all the earlyprintk
> with the earlycon. As of now, it's fair for both earlycon and earlyprintk.
> Or perhaps removing case#2, see in my previous email to Andrew Morton, is
> better?, so users be forced to specify earlycon and earlyprintk in cmdline
> if they want to see early_printk() output.
>
> Thanks.
>
> Best Regards,
> Sahara.
>
>
>>
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -426,7 +426,8 @@ static int __init do_early_param(char *param, char
>>> *val, const char *unused)
>>> for (p = __setup_start; p < __setup_end; p++) {
>>> if ((p->early && parameq(param, p->str)) ||
>>> (strcmp(param, "console") == 0 &&
>>> -strcmp(p->str, "earlycon") == 0)
>>> +((strcmp(p->str, "earlycon") == 0) ||
>>> +(strcmp(p->str, "earlyprintk") == 0)))
>>> ) {
>>> if (p->setup_func(val) != 0)
>>> pr_warn("Malformed early option '%s'\n",
>>> param);
>>> --
>>> 1.7.9.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] earlyprintk: re-enable earlyprintk calling early_param

[Keun-O Park]

FYI only if you missed my previous 2 replies which didn't reach the
mailing list.
Thanks.



On Mon, Aug 18, 2014 at 12:13 PM, Sahara keun-o.p...@windriver.com wrote:

 2014년 08월 16일 03:34, Rusty Russell 쓴 글:

 kpark3...@gmail.com writes:

 From: Sahara keun-o.p...@windriver.com

 Although there are many obs_kernel_param and its names are
 earlyprintk and also EARLY_PRINTK is also enabled, we could not
 see the early_printk output properly until now. This patch
 considers earlycon as well as earlyprintk.

 Hmm, the initial earlycon hack slipped in when I wasn't looking.
 I don't think we should extend it.

 Why not make the thing(s) you want early_param()s?

 Cheers,
 Rusty.

 The earlycon and the earlyprintk are scattered and used in many
 architectures.
 It looks earlycon just could be a subset of earlyprintk.
 The earlycon is for uart specific, while the earlyprintk is to support vga,
 efi, xen, serial, and so on. Especially ARM uses earlyprintk in many places.
 And, I am not sure if this is a good chance to replace all the earlyprintk
 with the earlycon. As of now, it's fair for both earlycon and earlyprintk.
 Or perhaps removing case#2, see in my previous email to Andrew Morton, is
 better?, so users be forced to specify earlycon and earlyprintk in cmdline
 if they want to see early_printk() output.

 Thanks.

 Best Regards,
 Sahara.



 --- a/init/main.c
 +++ b/init/main.c
 @@ -426,7 +426,8 @@ static int __init do_early_param(char *param, char
 *val, const char *unused)
 for (p = __setup_start; p  __setup_end; p++) {
 if ((p-early  parameq(param, p-str)) ||
 (strcmp(param, console) == 0 
 -strcmp(p-str, earlycon) == 0)
 +((strcmp(p-str, earlycon) == 0) ||
 +(strcmp(p-str, earlyprintk) == 0)))
 ) {
 if (p-setup_func(val) != 0)
 pr_warn(Malformed early option '%s'\n,
 param);
 --
 1.7.9.5


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

Re: [PATCH v2] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 2:34 PM,   wrote:
> From: Sahara 
>
> Somehow tracepoint_entry_add_probe function allows a null probe function.
> And, this may lead to unexpected result since the number of probe
> functions in an entry can be counted by checking whether probe is null
> or not in for-loop.
> This patch prevents the null probe from being added.
> In tracepoint_entry_remove_probe function, checking probe parameter
> within for-loop is moved out for code efficiency leaving the null probe
> feature which removes all probe functions in the entry.
>
> Signed-off-by: Sahara 
> Reviewed-by: Steven Rostedt 
> Reviewed-by: Mathieu Desnoyers 
> ---
>  kernel/tracepoint.c |   18 ++
>  1 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 0c05a45..7d69348 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
> int nr_probes = 0;
> struct tracepoint_func *old, *new;
>
> -   WARN_ON(!probe);
> +   if (WARN_ON(!probe))
> +   return ERR_PTR(-EINVAL);
>
> debug_print_probes(entry);
> old = entry->funcs;
> @@ -152,13 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
> *entry,
>
> debug_print_probes(entry);
> /* (N -> M), (N > 1, M >= 0) probes */
> -   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> -   if (!probe ||
> -   (old[nr_probes].func == probe &&
> -old[nr_probes].data == data))
> -   nr_del++;
> +   if (probe) {
> +   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> +   if (old[nr_probes].func == probe &&
> +old[nr_probes].data == data)
> +   nr_del++;
> +   }
> }
>
> +   /* If probe is NULL, all funcs in the entry will be removed. */
> if (nr_probes - nr_del == 0) {
> /* N -> 0, (N > 1) */
> entry->funcs = NULL;
> @@ -173,8 +176,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
> *entry,
> if (new == NULL)
> return ERR_PTR(-ENOMEM);
> for (i = 0; old[i].func; i++)
> -   if (probe &&
> -   (old[i].func != probe || old[i].data != data))
> +   if (old[i].func != probe || old[i].data != data)
> new[j++] = old[i];
> new[nr_probes - nr_del].func = NULL;
> entry->refcount = nr_probes - nr_del;
> --
> 1.7.1
>

Hi Steve,
Please check out this v2 patch. Seemingly I got no response from you.
Thanks.

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

Re: [PATCH v2] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 2:34 PM,  kpark3...@gmail.com wrote:
 From: Sahara keun-o.p...@windriver.com

 Somehow tracepoint_entry_add_probe function allows a null probe function.
 And, this may lead to unexpected result since the number of probe
 functions in an entry can be counted by checking whether probe is null
 or not in for-loop.
 This patch prevents the null probe from being added.
 In tracepoint_entry_remove_probe function, checking probe parameter
 within for-loop is moved out for code efficiency leaving the null probe
 feature which removes all probe functions in the entry.

 Signed-off-by: Sahara keun-o.p...@windriver.com
 Reviewed-by: Steven Rostedt rost...@goodmis.org
 Reviewed-by: Mathieu Desnoyers mathieu.desnoy...@efficios.com
 ---
  kernel/tracepoint.c |   18 ++
  1 files changed, 10 insertions(+), 8 deletions(-)

 diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
 index 0c05a45..7d69348 100644
 --- a/kernel/tracepoint.c
 +++ b/kernel/tracepoint.c
 @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
 int nr_probes = 0;
 struct tracepoint_func *old, *new;

 -   WARN_ON(!probe);
 +   if (WARN_ON(!probe))
 +   return ERR_PTR(-EINVAL);

 debug_print_probes(entry);
 old = entry-funcs;
 @@ -152,13 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
 *entry,

 debug_print_probes(entry);
 /* (N - M), (N  1, M = 0) probes */
 -   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
 -   if (!probe ||
 -   (old[nr_probes].func == probe 
 -old[nr_probes].data == data))
 -   nr_del++;
 +   if (probe) {
 +   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
 +   if (old[nr_probes].func == probe 
 +old[nr_probes].data == data)
 +   nr_del++;
 +   }
 }

 +   /* If probe is NULL, all funcs in the entry will be removed. */
 if (nr_probes - nr_del == 0) {
 /* N - 0, (N  1) */
 entry-funcs = NULL;
 @@ -173,8 +176,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
 *entry,
 if (new == NULL)
 return ERR_PTR(-ENOMEM);
 for (i = 0; old[i].func; i++)
 -   if (probe 
 -   (old[i].func != probe || old[i].data != data))
 +   if (old[i].func != probe || old[i].data != data)
 new[j++] = old[i];
 new[nr_probes - nr_del].func = NULL;
 entry-refcount = nr_probes - nr_del;
 --
 1.7.1


Hi Steve,
Please check out this v2 patch. Seemingly I got no response from you.
Thanks.

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

Re: [PATCH] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 12:33 PM, Mathieu Desnoyers
 wrote:
> * Keun-O Park (kpark3...@gmail.com) wrote:
>> On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
>>  wrote:
>> > * Keun-O Park (kpark3...@gmail.com) wrote:
>> >> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt  
>> >> wrote:
>> >> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>> >> >> * Steven Rostedt (rost...@goodmis.org) wrote:
>> >> >> > On Wed, 2013-03-20 at 12:18 +0900, kpark3...@gmail.com wrote:
>> >> >> > > From: Sahara 
>> >> >> > >
>> >> >> > > Somehow tracepoint_entry_add/remove_probe functions allow a null 
>> >> >> > > probe
>> >> >> > > function.
>> >> >> >
>> >> >> > You actually hit this in practice, or is this just something that you
>> >> >> > observe from code review?
>> >> >> >
>> >> >> > >  Especially on getting a null probe in remove function, it seems
>> >> >> > > to be used to remove all probe functions in the entry.
>> >> >> >
>> >> >> > Hmm, that actually sounds like a feature.
>> >> >>
>> >> >> Yep. It's been a long time since I wrote this code, but the removal 
>> >> >> code
>> >> >> seems to use NULL probe pointer to remove all probes for a given
>> >> >> tracepoint.
>> >> >>
>> >> >> I'd be tempted to just validate non-NULL probe within
>> >> >> tracepoint_entry_add_probe() and let other sites as is, just in case
>> >> >> anyone would be using this feature.
>> >> >>
>> >> >> I cannot say that I have personally used this "remove all" feature much
>> >> >> though.
>> >> >>
>> >> >
>> >> > I agree. I don't see anything wrong in leaving the null probe feature in
>> >> > the removal code. But updating the add code looks like a proper change.
>> >> >
>> >> > -- Steve
>> >> >
>> >> >
>> >>
>> >> Hello Steve & Mathieu,
>> >> If we want to leave the null probe feature enabled, I think it would
>> >> be better modifying the code like the following for code efficiency.
>> >>
>> >> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry 
>> >> *entry,
>> >> int nr_probes = 0;
>> >> struct tracepoint_func *old, *new;
>> >>
>> >> -   WARN_ON(!probe);
>> >> +   if (WARN_ON(!probe))
>> >> +   return ERR_PTR(-EINVAL);
>> >>
>> >> debug_print_probes(entry);
>> >> old = entry->funcs;
>> >> @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct 
>> >> tracepoint_entry *ent
>> >>
>> >> debug_print_probes(entry);
>> >> /* (N -> M), (N > 1, M >= 0) probes */
>> >> -   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
>> >> -   if (!probe ||
>> >> -   (old[nr_probes].func == probe &&
>> >> -old[nr_probes].data == data))
>> >> -   nr_del++;
>> >> +   if (probe) {
>> >> +   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
>> >> +   if (old[nr_probes].func == probe &&
>> >> +old[nr_probes].data == data)
>> >> +   nr_del++;
>> >> +   }
>> >> }
>> >>
>> >> -   if (nr_probes - nr_del == 0) {
>> >> +   if (!probe || nr_probes - nr_del == 0) {
>> >
>> > We might want to do:
>> >
>> > if (probe) {
>> >   ...
>> > } else {
>> >   nr_del = nr_probes;
>> > }
>> >
>> > if (nr_probes - nr_del == 0) {
>> >...
>> > }
>>
>> This code has a problem.
>> nr_probes is initialized as zero.
>
> yes,
>
>> And, in order to get correct count of probes,
>> we need to go through the for-loop even though probe is null.
>> So with above code, nr_del will be zero. Anyhow, the code will fall
>> through if-clause(nr_probes-nr_del==0).
>> It looks odd to me.
>
> Ah, I see what you mean: the nr_del = nr_probes assignment is useless,
> because both nr_probes and nr_del are equal to 0. So we could go for:
>
> if (probe) {
> for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> if (old[nr_probes].func == probe &&
>  old[nr_probes].data == data)
> nr_del++;
> }
> }
>
> if (nr_probes - nr_del == 0) {
> ...
> } else {
> ...
> }
>
> Does it look better ?
>
> Thanks,
>
> Mathieu

Yes, it does, only if you don't think this code is hard to follow. ;-)
Thanks.

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

Re: [PATCH] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
 wrote:
> * Keun-O Park (kpark3...@gmail.com) wrote:
>> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt  wrote:
>> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>> >> * Steven Rostedt (rost...@goodmis.org) wrote:
>> >> > On Wed, 2013-03-20 at 12:18 +0900, kpark3...@gmail.com wrote:
>> >> > > From: Sahara 
>> >> > >
>> >> > > Somehow tracepoint_entry_add/remove_probe functions allow a null probe
>> >> > > function.
>> >> >
>> >> > You actually hit this in practice, or is this just something that you
>> >> > observe from code review?
>> >> >
>> >> > >  Especially on getting a null probe in remove function, it seems
>> >> > > to be used to remove all probe functions in the entry.
>> >> >
>> >> > Hmm, that actually sounds like a feature.
>> >>
>> >> Yep. It's been a long time since I wrote this code, but the removal code
>> >> seems to use NULL probe pointer to remove all probes for a given
>> >> tracepoint.
>> >>
>> >> I'd be tempted to just validate non-NULL probe within
>> >> tracepoint_entry_add_probe() and let other sites as is, just in case
>> >> anyone would be using this feature.
>> >>
>> >> I cannot say that I have personally used this "remove all" feature much
>> >> though.
>> >>
>> >
>> > I agree. I don't see anything wrong in leaving the null probe feature in
>> > the removal code. But updating the add code looks like a proper change.
>> >
>> > -- Steve
>> >
>> >
>>
>> Hello Steve & Mathieu,
>> If we want to leave the null probe feature enabled, I think it would
>> be better modifying the code like the following for code efficiency.
>>
>> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry 
>> *entry,
>> int nr_probes = 0;
>> struct tracepoint_func *old, *new;
>>
>> -   WARN_ON(!probe);
>> +   if (WARN_ON(!probe))
>> +   return ERR_PTR(-EINVAL);
>>
>> debug_print_probes(entry);
>> old = entry->funcs;
>> @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
>> *ent
>>
>> debug_print_probes(entry);
>> /* (N -> M), (N > 1, M >= 0) probes */
>> -   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
>> -   if (!probe ||
>> -   (old[nr_probes].func == probe &&
>> -old[nr_probes].data == data))
>> -   nr_del++;
>> +   if (probe) {
>> +   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
>> +   if (old[nr_probes].func == probe &&
>> +old[nr_probes].data == data)
>> +   nr_del++;
>> +   }
>> }
>>
>> -   if (nr_probes - nr_del == 0) {
>> +   if (!probe || nr_probes - nr_del == 0) {
>
> We might want to do:
>
> if (probe) {
>   ...
> } else {
>   nr_del = nr_probes;
> }
>
> if (nr_probes - nr_del == 0) {
>...
> }

This code has a problem.
nr_probes is initialized as zero. And, in order to get correct count of probes,
we need to go through the for-loop even though probe is null.
So with above code, nr_del will be zero. Anyhow, the code will fall
through if-clause(nr_probes-nr_del==0).
It looks odd to me.

-- Kpark

>
> rather than:
>
> if (probe) {
>   ...
> }
>
> if (!probe || nr_probes - nr_del == 0) {
>...
> }
>
> Using nr_del makes the code easier to follow IMHO.
>
> Thanks,
>
> Mathieu
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 10:39 AM, Keun-O Park  wrote:
> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt  wrote:
>> On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>>> * Steven Rostedt (rost...@goodmis.org) wrote:
>>> > On Wed, 2013-03-20 at 12:18 +0900, kpark3...@gmail.com wrote:
>>> > > From: Sahara 
>>> > >
>>> > > Somehow tracepoint_entry_add/remove_probe functions allow a null probe
>>> > > function.
>>> >
>>> > You actually hit this in practice, or is this just something that you
>>> > observe from code review?
>>> >
>>> > >  Especially on getting a null probe in remove function, it seems
>>> > > to be used to remove all probe functions in the entry.
>>> >
>>> > Hmm, that actually sounds like a feature.
>>>
>>> Yep. It's been a long time since I wrote this code, but the removal code
>>> seems to use NULL probe pointer to remove all probes for a given
>>> tracepoint.
>>>
>>> I'd be tempted to just validate non-NULL probe within
>>> tracepoint_entry_add_probe() and let other sites as is, just in case
>>> anyone would be using this feature.
>>>
>>> I cannot say that I have personally used this "remove all" feature much
>>> though.
>>>
>>
>> I agree. I don't see anything wrong in leaving the null probe feature in
>> the removal code. But updating the add code looks like a proper change.
>>
>> -- Steve
>>
>>
>
> Hello Steve & Mathieu,
> If we want to leave the null probe feature enabled, I think it would
> be better modifying the code like the following for code efficiency.
>
> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
> int nr_probes = 0;
> struct tracepoint_func *old, *new;
>
> -   WARN_ON(!probe);
> +   if (WARN_ON(!probe))
> +   return ERR_PTR(-EINVAL);
>
> debug_print_probes(entry);
> old = entry->funcs;
> @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
> *ent
>
> debug_print_probes(entry);
> /* (N -> M), (N > 1, M >= 0) probes */
> -   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> -   if (!probe ||
> -   (old[nr_probes].func == probe &&
> -old[nr_probes].data == data))
> -   nr_del++;
> +   if (probe) {
> +   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> +   if (old[nr_probes].func == probe &&
> +old[nr_probes].data == data)
> +   nr_del++;
> +   }
> }
>
> -   if (nr_probes - nr_del == 0) {
> +   if (!probe || nr_probes - nr_del == 0) {
> /* N -> 0, (N > 1) */
> entry->funcs = NULL;
> entry->refcount = 0;
>
> Because we know handing over the null probe to
> tracepoint_entry_add_probe is not possible,
> we don't have to check if the probe is null or not within for loop. If

Hmm. I described this wrong. :-(
For code efficiency, I replaced 'checking null probe in every
iteration within for-loop' with 'checking once outside the loop'.

> the probe is null, it's just enough to add !probe in
> 'if(nr_probes-nr_del==0)'. And, with additional if-clause wrapping
> for-loop, falling through for-loop can be prevented when probe is
> null.
>
> @@ -173,8 +172,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
> *entry
> if (new == NULL)
> return ERR_PTR(-ENOMEM);
> for (i = 0; old[i].func; i++)
> -   if (probe &&
> -   (old[i].func != probe || old[i].data != data))
> +   if (old[i].func != probe || old[i].data != data)
> new[j++] = old[i];
> new[nr_probes - nr_del].func = NULL;
> entry->refcount = nr_probes - nr_del;
>
> We don't have to check the probe here too. We know probe is always true here.
> Thanks.
>
> -- Kpark
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt  wrote:
> On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>> * Steven Rostedt (rost...@goodmis.org) wrote:
>> > On Wed, 2013-03-20 at 12:18 +0900, kpark3...@gmail.com wrote:
>> > > From: Sahara 
>> > >
>> > > Somehow tracepoint_entry_add/remove_probe functions allow a null probe
>> > > function.
>> >
>> > You actually hit this in practice, or is this just something that you
>> > observe from code review?
>> >
>> > >  Especially on getting a null probe in remove function, it seems
>> > > to be used to remove all probe functions in the entry.
>> >
>> > Hmm, that actually sounds like a feature.
>>
>> Yep. It's been a long time since I wrote this code, but the removal code
>> seems to use NULL probe pointer to remove all probes for a given
>> tracepoint.
>>
>> I'd be tempted to just validate non-NULL probe within
>> tracepoint_entry_add_probe() and let other sites as is, just in case
>> anyone would be using this feature.
>>
>> I cannot say that I have personally used this "remove all" feature much
>> though.
>>
>
> I agree. I don't see anything wrong in leaving the null probe feature in
> the removal code. But updating the add code looks like a proper change.
>
> -- Steve
>
>

Hello Steve & Mathieu,
If we want to leave the null probe feature enabled, I think it would
be better modifying the code like the following for code efficiency.

@@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
int nr_probes = 0;
struct tracepoint_func *old, *new;

-   WARN_ON(!probe);
+   if (WARN_ON(!probe))
+   return ERR_PTR(-EINVAL);

debug_print_probes(entry);
old = entry->funcs;
@@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *ent

debug_print_probes(entry);
/* (N -> M), (N > 1, M >= 0) probes */
-   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
-   if (!probe ||
-   (old[nr_probes].func == probe &&
-old[nr_probes].data == data))
-   nr_del++;
+   if (probe) {
+   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
+   if (old[nr_probes].func == probe &&
+old[nr_probes].data == data)
+   nr_del++;
+   }
}

-   if (nr_probes - nr_del == 0) {
+   if (!probe || nr_probes - nr_del == 0) {
/* N -> 0, (N > 1) */
entry->funcs = NULL;
entry->refcount = 0;

Because we know handing over the null probe to
tracepoint_entry_add_probe is not possible,
we don't have to check if the probe is null or not within for loop. If
the probe is null, it's just enough to add !probe in
'if(nr_probes-nr_del==0)'. And, with additional if-clause wrapping
for-loop, falling through for-loop can be prevented when probe is
null.

@@ -173,8 +172,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry
if (new == NULL)
return ERR_PTR(-ENOMEM);
for (i = 0; old[i].func; i++)
-   if (probe &&
-   (old[i].func != probe || old[i].data != data))
+   if (old[i].func != probe || old[i].data != data)
new[j++] = old[i];
new[nr_probes - nr_del].func = NULL;
entry->refcount = nr_probes - nr_del;

We don't have to check the probe here too. We know probe is always true here.
Thanks.

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

Re: [PATCH] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt rost...@goodmis.org wrote:
 On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
 * Steven Rostedt (rost...@goodmis.org) wrote:
  On Wed, 2013-03-20 at 12:18 +0900, kpark3...@gmail.com wrote:
   From: Sahara keun-o.p...@windriver.com
  
   Somehow tracepoint_entry_add/remove_probe functions allow a null probe
   function.
 
  You actually hit this in practice, or is this just something that you
  observe from code review?
 
Especially on getting a null probe in remove function, it seems
   to be used to remove all probe functions in the entry.
 
  Hmm, that actually sounds like a feature.

 Yep. It's been a long time since I wrote this code, but the removal code
 seems to use NULL probe pointer to remove all probes for a given
 tracepoint.

 I'd be tempted to just validate non-NULL probe within
 tracepoint_entry_add_probe() and let other sites as is, just in case
 anyone would be using this feature.

 I cannot say that I have personally used this remove all feature much
 though.


 I agree. I don't see anything wrong in leaving the null probe feature in
 the removal code. But updating the add code looks like a proper change.

 -- Steve



Hello Steve  Mathieu,
If we want to leave the null probe feature enabled, I think it would
be better modifying the code like the following for code efficiency.

@@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
int nr_probes = 0;
struct tracepoint_func *old, *new;

-   WARN_ON(!probe);
+   if (WARN_ON(!probe))
+   return ERR_PTR(-EINVAL);

debug_print_probes(entry);
old = entry-funcs;
@@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *ent

debug_print_probes(entry);
/* (N - M), (N  1, M = 0) probes */
-   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
-   if (!probe ||
-   (old[nr_probes].func == probe 
-old[nr_probes].data == data))
-   nr_del++;
+   if (probe) {
+   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
+   if (old[nr_probes].func == probe 
+old[nr_probes].data == data)
+   nr_del++;
+   }
}

-   if (nr_probes - nr_del == 0) {
+   if (!probe || nr_probes - nr_del == 0) {
/* N - 0, (N  1) */
entry-funcs = NULL;
entry-refcount = 0;

Because we know handing over the null probe to
tracepoint_entry_add_probe is not possible,
we don't have to check if the probe is null or not within for loop. If
the probe is null, it's just enough to add !probe in
'if(nr_probes-nr_del==0)'. And, with additional if-clause wrapping
for-loop, falling through for-loop can be prevented when probe is
null.

@@ -173,8 +172,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry
if (new == NULL)
return ERR_PTR(-ENOMEM);
for (i = 0; old[i].func; i++)
-   if (probe 
-   (old[i].func != probe || old[i].data != data))
+   if (old[i].func != probe || old[i].data != data)
new[j++] = old[i];
new[nr_probes - nr_del].func = NULL;
entry-refcount = nr_probes - nr_del;

We don't have to check the probe here too. We know probe is always true here.
Thanks.

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

Re: [PATCH] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 10:39 AM, Keun-O Park kpark3...@gmail.com wrote:
 On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt rost...@goodmis.org wrote:
 On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
 * Steven Rostedt (rost...@goodmis.org) wrote:
  On Wed, 2013-03-20 at 12:18 +0900, kpark3...@gmail.com wrote:
   From: Sahara keun-o.p...@windriver.com
  
   Somehow tracepoint_entry_add/remove_probe functions allow a null probe
   function.
 
  You actually hit this in practice, or is this just something that you
  observe from code review?
 
Especially on getting a null probe in remove function, it seems
   to be used to remove all probe functions in the entry.
 
  Hmm, that actually sounds like a feature.

 Yep. It's been a long time since I wrote this code, but the removal code
 seems to use NULL probe pointer to remove all probes for a given
 tracepoint.

 I'd be tempted to just validate non-NULL probe within
 tracepoint_entry_add_probe() and let other sites as is, just in case
 anyone would be using this feature.

 I cannot say that I have personally used this remove all feature much
 though.


 I agree. I don't see anything wrong in leaving the null probe feature in
 the removal code. But updating the add code looks like a proper change.

 -- Steve



 Hello Steve  Mathieu,
 If we want to leave the null probe feature enabled, I think it would
 be better modifying the code like the following for code efficiency.

 @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
 int nr_probes = 0;
 struct tracepoint_func *old, *new;

 -   WARN_ON(!probe);
 +   if (WARN_ON(!probe))
 +   return ERR_PTR(-EINVAL);

 debug_print_probes(entry);
 old = entry-funcs;
 @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
 *ent

 debug_print_probes(entry);
 /* (N - M), (N  1, M = 0) probes */
 -   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
 -   if (!probe ||
 -   (old[nr_probes].func == probe 
 -old[nr_probes].data == data))
 -   nr_del++;
 +   if (probe) {
 +   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
 +   if (old[nr_probes].func == probe 
 +old[nr_probes].data == data)
 +   nr_del++;
 +   }
 }

 -   if (nr_probes - nr_del == 0) {
 +   if (!probe || nr_probes - nr_del == 0) {
 /* N - 0, (N  1) */
 entry-funcs = NULL;
 entry-refcount = 0;

 Because we know handing over the null probe to
 tracepoint_entry_add_probe is not possible,
 we don't have to check if the probe is null or not within for loop. If

Hmm. I described this wrong. :-(
For code efficiency, I replaced 'checking null probe in every
iteration within for-loop' with 'checking once outside the loop'.

 the probe is null, it's just enough to add !probe in
 'if(nr_probes-nr_del==0)'. And, with additional if-clause wrapping
 for-loop, falling through for-loop can be prevented when probe is
 null.

 @@ -173,8 +172,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
 *entry
 if (new == NULL)
 return ERR_PTR(-ENOMEM);
 for (i = 0; old[i].func; i++)
 -   if (probe 
 -   (old[i].func != probe || old[i].data != data))
 +   if (old[i].func != probe || old[i].data != data)
 new[j++] = old[i];
 new[nr_probes - nr_del].func = NULL;
 entry-refcount = nr_probes - nr_del;

 We don't have to check the probe here too. We know probe is always true here.
 Thanks.

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

Re: [PATCH] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
mathieu.desnoy...@efficios.com wrote:
 * Keun-O Park (kpark3...@gmail.com) wrote:
 On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt rost...@goodmis.org wrote:
  On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
  * Steven Rostedt (rost...@goodmis.org) wrote:
   On Wed, 2013-03-20 at 12:18 +0900, kpark3...@gmail.com wrote:
From: Sahara keun-o.p...@windriver.com
   
Somehow tracepoint_entry_add/remove_probe functions allow a null probe
function.
  
   You actually hit this in practice, or is this just something that you
   observe from code review?
  
 Especially on getting a null probe in remove function, it seems
to be used to remove all probe functions in the entry.
  
   Hmm, that actually sounds like a feature.
 
  Yep. It's been a long time since I wrote this code, but the removal code
  seems to use NULL probe pointer to remove all probes for a given
  tracepoint.
 
  I'd be tempted to just validate non-NULL probe within
  tracepoint_entry_add_probe() and let other sites as is, just in case
  anyone would be using this feature.
 
  I cannot say that I have personally used this remove all feature much
  though.
 
 
  I agree. I don't see anything wrong in leaving the null probe feature in
  the removal code. But updating the add code looks like a proper change.
 
  -- Steve
 
 

 Hello Steve  Mathieu,
 If we want to leave the null probe feature enabled, I think it would
 be better modifying the code like the following for code efficiency.

 @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry 
 *entry,
 int nr_probes = 0;
 struct tracepoint_func *old, *new;

 -   WARN_ON(!probe);
 +   if (WARN_ON(!probe))
 +   return ERR_PTR(-EINVAL);

 debug_print_probes(entry);
 old = entry-funcs;
 @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry 
 *ent

 debug_print_probes(entry);
 /* (N - M), (N  1, M = 0) probes */
 -   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
 -   if (!probe ||
 -   (old[nr_probes].func == probe 
 -old[nr_probes].data == data))
 -   nr_del++;
 +   if (probe) {
 +   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
 +   if (old[nr_probes].func == probe 
 +old[nr_probes].data == data)
 +   nr_del++;
 +   }
 }

 -   if (nr_probes - nr_del == 0) {
 +   if (!probe || nr_probes - nr_del == 0) {

 We might want to do:

 if (probe) {
   ...
 } else {
   nr_del = nr_probes;
 }

 if (nr_probes - nr_del == 0) {
...
 }

This code has a problem.
nr_probes is initialized as zero. And, in order to get correct count of probes,
we need to go through the for-loop even though probe is null.
So with above code, nr_del will be zero. Anyhow, the code will fall
through if-clause(nr_probes-nr_del==0).
It looks odd to me.

-- Kpark


 rather than:

 if (probe) {
   ...
 }

 if (!probe || nr_probes - nr_del == 0) {
...
 }

 Using nr_del makes the code easier to follow IMHO.

 Thanks,

 Mathieu

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

Re: [PATCH] tracepoints: prevents null probe from being added

[Keun-O Park]

On Thu, Mar 21, 2013 at 12:33 PM, Mathieu Desnoyers
mathieu.desnoy...@efficios.com wrote:
 * Keun-O Park (kpark3...@gmail.com) wrote:
 On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
 mathieu.desnoy...@efficios.com wrote:
  * Keun-O Park (kpark3...@gmail.com) wrote:
  On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt rost...@goodmis.org 
  wrote:
   On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
   * Steven Rostedt (rost...@goodmis.org) wrote:
On Wed, 2013-03-20 at 12:18 +0900, kpark3...@gmail.com wrote:
 From: Sahara keun-o.p...@windriver.com

 Somehow tracepoint_entry_add/remove_probe functions allow a null 
 probe
 function.
   
You actually hit this in practice, or is this just something that you
observe from code review?
   
  Especially on getting a null probe in remove function, it seems
 to be used to remove all probe functions in the entry.
   
Hmm, that actually sounds like a feature.
  
   Yep. It's been a long time since I wrote this code, but the removal 
   code
   seems to use NULL probe pointer to remove all probes for a given
   tracepoint.
  
   I'd be tempted to just validate non-NULL probe within
   tracepoint_entry_add_probe() and let other sites as is, just in case
   anyone would be using this feature.
  
   I cannot say that I have personally used this remove all feature much
   though.
  
  
   I agree. I don't see anything wrong in leaving the null probe feature in
   the removal code. But updating the add code looks like a proper change.
  
   -- Steve
  
  
 
  Hello Steve  Mathieu,
  If we want to leave the null probe feature enabled, I think it would
  be better modifying the code like the following for code efficiency.
 
  @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry 
  *entry,
  int nr_probes = 0;
  struct tracepoint_func *old, *new;
 
  -   WARN_ON(!probe);
  +   if (WARN_ON(!probe))
  +   return ERR_PTR(-EINVAL);
 
  debug_print_probes(entry);
  old = entry-funcs;
  @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct 
  tracepoint_entry *ent
 
  debug_print_probes(entry);
  /* (N - M), (N  1, M = 0) probes */
  -   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
  -   if (!probe ||
  -   (old[nr_probes].func == probe 
  -old[nr_probes].data == data))
  -   nr_del++;
  +   if (probe) {
  +   for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
  +   if (old[nr_probes].func == probe 
  +old[nr_probes].data == data)
  +   nr_del++;
  +   }
  }
 
  -   if (nr_probes - nr_del == 0) {
  +   if (!probe || nr_probes - nr_del == 0) {
 
  We might want to do:
 
  if (probe) {
...
  } else {
nr_del = nr_probes;
  }
 
  if (nr_probes - nr_del == 0) {
 ...
  }

 This code has a problem.
 nr_probes is initialized as zero.

 yes,

 And, in order to get correct count of probes,
 we need to go through the for-loop even though probe is null.
 So with above code, nr_del will be zero. Anyhow, the code will fall
 through if-clause(nr_probes-nr_del==0).
 It looks odd to me.

 Ah, I see what you mean: the nr_del = nr_probes assignment is useless,
 because both nr_probes and nr_del are equal to 0. So we could go for:

 if (probe) {
 for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
 if (old[nr_probes].func == probe 
  old[nr_probes].data == data)
 nr_del++;
 }
 }

 if (nr_probes - nr_del == 0) {
 ...
 } else {
 ...
 }

 Does it look better ?

 Thanks,

 Mathieu

Yes, it does, only if you don't think this code is hard to follow. ;-)
Thanks.

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

Re: [for-next][PATCH 9/9] tracing: Update debugfs README file

[Keun-O Park]

On Mon, Mar 18, 2013 at 5:22 PM, Namhyung Kim  wrote:
> Hi Steve,
>
> On Fri, 15 Mar 2013 23:24:21 -0400, Steven Rostedt wrote:
>> From: "Steven Rostedt (Red Hat)" 
>>
>> Update the README file in debugfs/tracing to something more useful.
>> What's currently in the file is very old and what it shows doesn't
>> have much use. Heck, it tells you how to mount debugfs! But to read
>> this file you would have already needed to mount it.
>>
>> Replace the file with current up-to-date information. It's rather
>> limited, but what do you expect from a pseudo README file.
>>
>> Signed-off-by: Steven Rostedt 
>> ---
>>  kernel/trace/trace.c |   89 
>> ++
>>  1 file changed, 75 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 3dc7999..83ad596 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -3300,20 +3300,81 @@ static const struct file_operations 
>> tracing_iter_fops = {
>>
>>  static const char readme_msg[] =
>>   "tracing mini-HOWTO:\n\n"
>> - "# mount -t debugfs nodev /sys/kernel/debug\n\n"
>> - "# cat /sys/kernel/debug/tracing/available_tracers\n"
>> - "wakeup wakeup_rt preemptirqsoff preemptoff irqsoff function nop\n\n"
>> - "# cat /sys/kernel/debug/tracing/current_tracer\n"
>> - "nop\n"
>> - "# echo wakeup > /sys/kernel/debug/tracing/current_tracer\n"
>> - "# cat /sys/kernel/debug/tracing/current_tracer\n"
>> - "wakeup\n"
>> - "# cat /sys/kernel/debug/tracing/trace_options\n"
>> - "noprint-parent nosym-offset nosym-addr noverbose\n"
>> - "# echo print-parent > /sys/kernel/debug/tracing/trace_options\n"
>> - "# echo 1 > /sys/kernel/debug/tracing/tracing_on\n"
>> - "# cat /sys/kernel/debug/tracing/trace > /tmp/trace.txt\n"
>> - "# echo 0 > /sys/kernel/debug/tracing/tracing_on\n"
>> + "# echo 0 > tracing_on : quick way to disable tracing\n"
>> + "# echo 1 > tracing_on : quick way to re-enable tracing\n\n"
>> + " Important files:\n"
>> + "  trace\t\t\t- The static contents of the buffer\n"
>> + "\t\t\t  To clear the buffer write into this file: echo > trace\n"
>> + "  trace_pipe\t\t- A consuming read to see the contents of the 
>> buffer\n"
>> + "  current_tracer\t- function and latency tracers\n"
>> + "  available_tracers\t- list of configured tracers for 
>> current_tracer\n"
>> + "  buffer_size_kb\t- view and modify size of per cpu buffer\n"
>> + "  buffer_total_size_kb  - view total size of all cpu buffers\n\n"
>> + "  trace_clock\t\t-change the clock used to order events\n"
>> + "   local:   Per cpu clock but may not be synced across CPUS\n"
>> + "  global:   Synced across CPUs but slows tracing down.\n"
>> + " counter:   Not a clock, but just an increment\n"
>> + "  uptime:   Jiffy counter from time of boot\n"
>> + "perf:   Same clock that perf events use\n"
>> +#ifdef CONFIG_X86_64
>> + " x86-tsc:   TSC cycle counter\n"
>> +#endif
>> + "\n  trace_marker\t\t- Writes into this file writes into the kernel 
>> buffer\n"
>> + "  tracing_cpumask\t- Limit which CPUs to trace\n"
>> + "  instances\t\t- Make sub-buffers with: mkdir instances/foo\n"
>> + "\t\t\t  Remove sub-buffer with rmdir\n"
>> + "  trace_options\t\t- Set format or modify how tracing happens\n"
>> + "\t\t\t  Disable an option by adding a suffix 'no' to the option 
>> name\n"
>> +#ifdef CONFIG_DYNAMIC_FTRACE
>> + "\n  available_filter_functions - list of functions that can be 
>> filtered on\n"
>> + "  set_ftrace_filter\t- echo function name in here to only trace these 
>> functions\n"
>> + "accepts: func_full_name, *func_end, func_begin*, 
>> *func_middle*\n"
>> + "modules: Can select a group via module\n"
>> + " Format: :mod:\n"
>> + " example: echo :mod:ext3 > set_ftrace_filter\n"
>> + "triggers: a command to perform when function is hit\n"
>> + "  Format: :[:count]\n"
>> + " trigger: traceon, traceoff\n"
>> + "  enable_event::\n"
>> + "  disable_event::\n"
>> +#ifdef CONFIG_STACKTRACE
>> + "  stacktrace\n"
>> +#endif
>> +#ifdef CONFIG_TRACER_SNAPSHOT
>> + "  snapshot\n"
>> +#endif
>> + " example: echo do_fault:traceoff > set_ftrace_filter\n"
>> + "  echo do_trap:traceoff:3 > set_ftrace_filter\n"
>> + " The first one will disable tracing everytime do_fault is 
>> hit\n"
>> + " The second will disable tracing at most 3 times whne 
>> do_trap is hit\n"
>
> s/whne/when/
>
> It's slightly confusing for me to read the "at most" part.  Did you mean
> it by "The second will disable tracing after do_trace is hit 3 times"?
>
> Thanks,
> Namhyung
>
>
>> + " 

Re: [for-next][PATCH 9/9] tracing: Update debugfs README file

[Keun-O Park]

On Mon, Mar 18, 2013 at 5:22 PM, Namhyung Kim namhy...@kernel.org wrote:
 Hi Steve,

 On Fri, 15 Mar 2013 23:24:21 -0400, Steven Rostedt wrote:
 From: Steven Rostedt (Red Hat) rost...@goodmis.org

 Update the README file in debugfs/tracing to something more useful.
 What's currently in the file is very old and what it shows doesn't
 have much use. Heck, it tells you how to mount debugfs! But to read
 this file you would have already needed to mount it.

 Replace the file with current up-to-date information. It's rather
 limited, but what do you expect from a pseudo README file.

 Signed-off-by: Steven Rostedt rost...@goodmis.org
 ---
  kernel/trace/trace.c |   89 
 ++
  1 file changed, 75 insertions(+), 14 deletions(-)

 diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
 index 3dc7999..83ad596 100644
 --- a/kernel/trace/trace.c
 +++ b/kernel/trace/trace.c
 @@ -3300,20 +3300,81 @@ static const struct file_operations 
 tracing_iter_fops = {

  static const char readme_msg[] =
   tracing mini-HOWTO:\n\n
 - # mount -t debugfs nodev /sys/kernel/debug\n\n
 - # cat /sys/kernel/debug/tracing/available_tracers\n
 - wakeup wakeup_rt preemptirqsoff preemptoff irqsoff function nop\n\n
 - # cat /sys/kernel/debug/tracing/current_tracer\n
 - nop\n
 - # echo wakeup  /sys/kernel/debug/tracing/current_tracer\n
 - # cat /sys/kernel/debug/tracing/current_tracer\n
 - wakeup\n
 - # cat /sys/kernel/debug/tracing/trace_options\n
 - noprint-parent nosym-offset nosym-addr noverbose\n
 - # echo print-parent  /sys/kernel/debug/tracing/trace_options\n
 - # echo 1  /sys/kernel/debug/tracing/tracing_on\n
 - # cat /sys/kernel/debug/tracing/trace  /tmp/trace.txt\n
 - # echo 0  /sys/kernel/debug/tracing/tracing_on\n
 + # echo 0  tracing_on : quick way to disable tracing\n
 + # echo 1  tracing_on : quick way to re-enable tracing\n\n
 +  Important files:\n
 +   trace\t\t\t- The static contents of the buffer\n
 + \t\t\t  To clear the buffer write into this file: echo  trace\n
 +   trace_pipe\t\t- A consuming read to see the contents of the 
 buffer\n
 +   current_tracer\t- function and latency tracers\n
 +   available_tracers\t- list of configured tracers for 
 current_tracer\n
 +   buffer_size_kb\t- view and modify size of per cpu buffer\n
 +   buffer_total_size_kb  - view total size of all cpu buffers\n\n
 +   trace_clock\t\t-change the clock used to order events\n
 +local:   Per cpu clock but may not be synced across CPUS\n
 +   global:   Synced across CPUs but slows tracing down.\n
 +  counter:   Not a clock, but just an increment\n
 +   uptime:   Jiffy counter from time of boot\n
 + perf:   Same clock that perf events use\n
 +#ifdef CONFIG_X86_64
 +  x86-tsc:   TSC cycle counter\n
 +#endif
 + \n  trace_marker\t\t- Writes into this file writes into the kernel 
 buffer\n
 +   tracing_cpumask\t- Limit which CPUs to trace\n
 +   instances\t\t- Make sub-buffers with: mkdir instances/foo\n
 + \t\t\t  Remove sub-buffer with rmdir\n
 +   trace_options\t\t- Set format or modify how tracing happens\n
 + \t\t\t  Disable an option by adding a suffix 'no' to the option 
 name\n
 +#ifdef CONFIG_DYNAMIC_FTRACE
 + \n  available_filter_functions - list of functions that can be 
 filtered on\n
 +   set_ftrace_filter\t- echo function name in here to only trace these 
 functions\n
 + accepts: func_full_name, *func_end, func_begin*, 
 *func_middle*\n
 + modules: Can select a group via module\n
 +  Format: :mod:module-name\n
 +  example: echo :mod:ext3  set_ftrace_filter\n
 + triggers: a command to perform when function is hit\n
 +   Format: function:trigger[:count]\n
 +  trigger: traceon, traceoff\n
 +   enable_event:system:event\n
 +   disable_event:system:event\n
 +#ifdef CONFIG_STACKTRACE
 +   stacktrace\n
 +#endif
 +#ifdef CONFIG_TRACER_SNAPSHOT
 +   snapshot\n
 +#endif
 +  example: echo do_fault:traceoff  set_ftrace_filter\n
 +   echo do_trap:traceoff:3  set_ftrace_filter\n
 +  The first one will disable tracing everytime do_fault is 
 hit\n
 +  The second will disable tracing at most 3 times whne 
 do_trap is hit\n

 s/whne/when/

 It's slightly confusing for me to read the at most part.  Did you mean
 it by The second will disable tracing after do_trace is hit 3 times?

 Thanks,
 Namhyung


 +  To remove trigger without count:\n
 +echo '!function:trigger  set_ftrace_filter\n
 +  To remove trigger with a count:\n
 +echo '!function:trigger:0  set_ftrace_filter\n
 + 

Re: [PATCH 19/19] [INCOMPLETE] ARM: make return_address available for ARM_UNWIND

[Keun-O Park]

On Mon, Jan 28, 2013 at 9:50 PM, Dave Martin  wrote:
> On Mon, Jan 28, 2013 at 11:33:11AM +0900, Keun-O Park wrote:
>> Hello guys,
>>
>> Could you please review the patch of fixing bug first of returning
>> wrong address when using frame pointer?
>> I am wondering if the first patch is not delivered to the mailing.
>
> I posted a similar patch to alkml a couple of months ago, but I got
> no response and it looks like I forgot about it.
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129381.html

Yes, same except initialization of data.addr. :)
This means there might be no one interested in using
ftrace-irqsoff/premptoff in ARM during a couple of months?


>
> [...]
>
>>
>> ~snip~
>> From 3a60b536d22a2043d735c890a9aac9e7cb72de8f Mon Sep 17 00:00:00 2001
>> From: sahara 
>> Date: Thu, 3 Jan 2013 17:12:37 +0900
>> Subject: [PATCH 1/2] arm: fix returning wrong CALLER_ADDRx
>>
>> This makes return_address return correct value for ftrace feature.
>> unwind_frame does not update frame->lr but frame->pc for backtrace.
>> And, the initialization for data.addr was missing so that wrong value
>> returned when unwind_frame failed.
>>
>> Signed-off-by: sahara 
>> ---
>>  arch/arm/kernel/return_address.c |5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/kernel/return_address.c 
>> b/arch/arm/kernel/return_address.c
>> index 8085417..fafedd8 100644
>> --- a/arch/arm/kernel/return_address.c
>> +++ b/arch/arm/kernel/return_address.c
>> @@ -26,7 +26,7 @@ static int save_return_addr(struct stackframe *frame, void 
>> *d)
>> struct return_address_data *data = d;
>>
>> if (!data->level) {
>> -   data->addr = (void *)frame->lr;
>> +   data->addr = (void *)frame->pc;
>>
>> return 1;
>> } else {
>> @@ -41,7 +41,8 @@ void *return_address(unsigned int level)
>> struct stackframe frame;
>> register unsigned long current_sp asm ("sp");
>>
>> -   data.level = level + 1;
>> +   data.level = level + 2;
>> +   data.addr = NULL;
>
> Can you explain why this is needed?  I think I concluded it wasn't
> necessary, but you may be right -- I think if walk_stackframe()
> fails to unwind the next frame just after data.level reaches zero,
> then data.addr can remain unset and return_address() may return
> uninitialised garbage.

That's correct.
Here is the examples of reproducing the problem.
I added one line printk for test in wakeup_flusher_threads() in
fs/fs-writeback.c.
And then after boot up, I synced.

[TEST#1 : print CALLER_ADDR0, 1 and 2]
Without initialization of data.addr:
~ # sync
TEST: CALLER_ADDR0=(sys_sync+0x34/0xac),
CALLER_ADDR1=(ret_fast_syscall+0x0/0x48),
CALLER_ADDR2=(ret_fast_syscall+0x0/0x48)
With initialization of data.addr:
~ # sync
TEST: CALLER_ADDR0=(sys_sync+0x34/0xac),
CALLER_ADDR1=(ret_fast_syscall+0x0/0x48), CALLER_ADDR2=(  (null))

[TEST#2 : print CALLER_ADDR0 and 2]
Without initialization of data.addr:
~ # sync
TEST: CALLER_ADDR0=(sys_sync+0x34/0xac), CALLER_ADDR2=(0x872fffb0)
With initialization of data.addr:
~ # sync
TEST: CALLER_ADDR0=(sys_sync+0x34/0xac), CALLER_ADDR2=((null))

As you see, when unwind_fame() fails right after data.level reaches zero,
the routine returns data.addr which has uninitialized garbage value.

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

Re: [PATCH 19/19] [INCOMPLETE] ARM: make return_address available for ARM_UNWIND

[Keun-O Park]

On Mon, Jan 28, 2013 at 9:50 PM, Dave Martin dave.mar...@linaro.org wrote:
 On Mon, Jan 28, 2013 at 11:33:11AM +0900, Keun-O Park wrote:
 Hello guys,

 Could you please review the patch of fixing bug first of returning
 wrong address when using frame pointer?
 I am wondering if the first patch is not delivered to the mailing.

 I posted a similar patch to alkml a couple of months ago, but I got
 no response and it looks like I forgot about it.

 http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129381.html

Yes, same except initialization of data.addr. :)
This means there might be no one interested in using
ftrace-irqsoff/premptoff in ARM during a couple of months?



 [...]


 ~snip~
 From 3a60b536d22a2043d735c890a9aac9e7cb72de8f Mon Sep 17 00:00:00 2001
 From: sahara keun-o.p...@windriver.com
 Date: Thu, 3 Jan 2013 17:12:37 +0900
 Subject: [PATCH 1/2] arm: fix returning wrong CALLER_ADDRx

 This makes return_address return correct value for ftrace feature.
 unwind_frame does not update frame-lr but frame-pc for backtrace.
 And, the initialization for data.addr was missing so that wrong value
 returned when unwind_frame failed.

 Signed-off-by: sahara keun-o.p...@windriver.com
 ---
  arch/arm/kernel/return_address.c |5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)

 diff --git a/arch/arm/kernel/return_address.c 
 b/arch/arm/kernel/return_address.c
 index 8085417..fafedd8 100644
 --- a/arch/arm/kernel/return_address.c
 +++ b/arch/arm/kernel/return_address.c
 @@ -26,7 +26,7 @@ static int save_return_addr(struct stackframe *frame, void 
 *d)
 struct return_address_data *data = d;

 if (!data-level) {
 -   data-addr = (void *)frame-lr;
 +   data-addr = (void *)frame-pc;

 return 1;
 } else {
 @@ -41,7 +41,8 @@ void *return_address(unsigned int level)
 struct stackframe frame;
 register unsigned long current_sp asm (sp);

 -   data.level = level + 1;
 +   data.level = level + 2;
 +   data.addr = NULL;

 Can you explain why this is needed?  I think I concluded it wasn't
 necessary, but you may be right -- I think if walk_stackframe()
 fails to unwind the next frame just after data.level reaches zero,
 then data.addr can remain unset and return_address() may return
 uninitialised garbage.

That's correct.
Here is the examples of reproducing the problem.
I added one line printk for test in wakeup_flusher_threads() in
fs/fs-writeback.c.
And then after boot up, I synced.

[TEST#1 : print CALLER_ADDR0, 1 and 2]
Without initialization of data.addr:
~ # sync
TEST: CALLER_ADDR0=(sys_sync+0x34/0xac),
CALLER_ADDR1=(ret_fast_syscall+0x0/0x48),
CALLER_ADDR2=(ret_fast_syscall+0x0/0x48)
With initialization of data.addr:
~ # sync
TEST: CALLER_ADDR0=(sys_sync+0x34/0xac),
CALLER_ADDR1=(ret_fast_syscall+0x0/0x48), CALLER_ADDR2=(  (null))

[TEST#2 : print CALLER_ADDR0 and 2]
Without initialization of data.addr:
~ # sync
TEST: CALLER_ADDR0=(sys_sync+0x34/0xac), CALLER_ADDR2=(0x872fffb0)
With initialization of data.addr:
~ # sync
TEST: CALLER_ADDR0=(sys_sync+0x34/0xac), CALLER_ADDR2=((null))

As you see, when unwind_fame() fails right after data.level reaches zero,
the routine returns data.addr which has uninitialized garbage value.

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

Re: [PATCH 19/19] [INCOMPLETE] ARM: make return_address available for ARM_UNWIND

[Keun-O Park]

Hello guys,

Could you please review the patch of fixing bug first of returning
wrong address when using frame pointer?
I am wondering if the first patch is not delivered to the mailing.

~snip~
>From 3a60b536d22a2043d735c890a9aac9e7cb72de8f Mon Sep 17 00:00:00 2001
From: sahara 
Date: Thu, 3 Jan 2013 17:12:37 +0900
Subject: [PATCH 1/2] arm: fix returning wrong CALLER_ADDRx

This makes return_address return correct value for ftrace feature.
unwind_frame does not update frame->lr but frame->pc for backtrace.
And, the initialization for data.addr was missing so that wrong value
returned when unwind_frame failed.

Signed-off-by: sahara 
---
 arch/arm/kernel/return_address.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index 8085417..fafedd8 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -26,7 +26,7 @@ static int save_return_addr(struct stackframe *frame, void *d)
struct return_address_data *data = d;

if (!data->level) {
-   data->addr = (void *)frame->lr;
+   data->addr = (void *)frame->pc;

return 1;
} else {
@@ -41,7 +41,8 @@ void *return_address(unsigned int level)
struct stackframe frame;
register unsigned long current_sp asm ("sp");

-   data.level = level + 1;
+   data.level = level + 2;
+   data.addr = NULL;

frame.fp = (unsigned long)__builtin_frame_address(0);
frame.sp = current_sp;
-- 
1.7.1
~snip~

Without this patch, when I added the following printk line and did
sync command,
it returned wrong return addresses using frame pointer.

Added line in __bdi_start_writeback():
+ printk("TEST: CALLER_ADDR0=(%pS), CALLER_ADDR1=(%pS),
CALLER_ADDR2=(%pS)\n", (void *)CALLER_ADDR0, (void *)CALLER_ADDR1,
(void *)CALLER_ADDR2);

Call sequence:
sys_sync() -> wakeup_flusher_threads() -> __bdi_start_writeback()

Result of sync after boot up:
~ # sync
TEST: CALLER_ADDR0=(wakeup_flusher_threads+0x9c/0xb8),
CALLER_ADDR1=(__bdi_start_writeback+0x30/0x120),
CALLER_ADDR2=(__bdi_start_writeback+0x3c/0x120)

As you see, the result of CALLER_ADDR1 and CALLER_ADDR2 is wrong.

With this patch, you will be able to see the following result.
~ # sync
TEST: CALLER_ADDR0=(wakeup_flusher_threads+0x9c/0xb8),
CALLER_ADDR1=(sys_sync+0x34/0xac),
CALLER_ADDR2=(ret_fast_syscall+0x0/0x48)

Based on this patch, if you apply the second patch which enables the
arm unwind,
and turning on CONFIG_ARM_UNWIND, you will see the correct result.

What I am currently concerning is if I use unwind info and ftrace's irqsoff,
I presume the ftrace might need architecture specific function to make
irqsoff work correctly.
For example, when I tried to test irqsoff, I got the message from
trace like the following.

 cat-563 0d...2us+: __irq_svc <-_raw_spin_unlock_irqrestore

Actually I could see the call sequence from the end of the trace.
~~snip~~
 => gic_handle_irq
 => __irq_svc
 => _raw_spin_unlock_irqrestore
 => uart_start
 => uart_write
~~snip~~
Seeing this call sequences, the output need to be
'_raw_spin_unlock_irqrestore <- uart_start'.
But, the CALLER_ADDR1 in trace_hardirqs_off() returned correct value.
So there's no problem in output.
I think trace_hardirqs_off() should call CALLER_ADDR1 and CALLER_ADDR2
respectively for its arguments for start_critical_timing(). This
thought leads me to the necessity of creating architecture specific
trace_hardirqs_off function. Any opinion on this?

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

Re: [PATCH 19/19] [INCOMPLETE] ARM: make return_address available for ARM_UNWIND

[Keun-O Park]

Hello guys,

Could you please review the patch of fixing bug first of returning
wrong address when using frame pointer?
I am wondering if the first patch is not delivered to the mailing.

~snip~
From 3a60b536d22a2043d735c890a9aac9e7cb72de8f Mon Sep 17 00:00:00 2001
From: sahara keun-o.p...@windriver.com
Date: Thu, 3 Jan 2013 17:12:37 +0900
Subject: [PATCH 1/2] arm: fix returning wrong CALLER_ADDRx

This makes return_address return correct value for ftrace feature.
unwind_frame does not update frame-lr but frame-pc for backtrace.
And, the initialization for data.addr was missing so that wrong value
returned when unwind_frame failed.

Signed-off-by: sahara keun-o.p...@windriver.com
---
 arch/arm/kernel/return_address.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index 8085417..fafedd8 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -26,7 +26,7 @@ static int save_return_addr(struct stackframe *frame, void *d)
struct return_address_data *data = d;

if (!data-level) {
-   data-addr = (void *)frame-lr;
+   data-addr = (void *)frame-pc;

return 1;
} else {
@@ -41,7 +41,8 @@ void *return_address(unsigned int level)
struct stackframe frame;
register unsigned long current_sp asm (sp);

-   data.level = level + 1;
+   data.level = level + 2;
+   data.addr = NULL;

frame.fp = (unsigned long)__builtin_frame_address(0);
frame.sp = current_sp;
-- 
1.7.1
~snip~

Without this patch, when I added the following printk line and did
sync command,
it returned wrong return addresses using frame pointer.

Added line in __bdi_start_writeback():
+ printk(TEST: CALLER_ADDR0=(%pS), CALLER_ADDR1=(%pS),
CALLER_ADDR2=(%pS)\n, (void *)CALLER_ADDR0, (void *)CALLER_ADDR1,
(void *)CALLER_ADDR2);

Call sequence:
sys_sync() - wakeup_flusher_threads() - __bdi_start_writeback()

Result of sync after boot up:
~ # sync
TEST: CALLER_ADDR0=(wakeup_flusher_threads+0x9c/0xb8),
CALLER_ADDR1=(__bdi_start_writeback+0x30/0x120),
CALLER_ADDR2=(__bdi_start_writeback+0x3c/0x120)

As you see, the result of CALLER_ADDR1 and CALLER_ADDR2 is wrong.

With this patch, you will be able to see the following result.
~ # sync
TEST: CALLER_ADDR0=(wakeup_flusher_threads+0x9c/0xb8),
CALLER_ADDR1=(sys_sync+0x34/0xac),
CALLER_ADDR2=(ret_fast_syscall+0x0/0x48)

Based on this patch, if you apply the second patch which enables the
arm unwind,
and turning on CONFIG_ARM_UNWIND, you will see the correct result.

What I am currently concerning is if I use unwind info and ftrace's irqsoff,
I presume the ftrace might need architecture specific function to make
irqsoff work correctly.
For example, when I tried to test irqsoff, I got the message from
trace like the following.

 cat-563 0d...2us+: __irq_svc -_raw_spin_unlock_irqrestore

Actually I could see the call sequence from the end of the trace.
~~snip~~
 = gic_handle_irq
 = __irq_svc
 = _raw_spin_unlock_irqrestore
 = uart_start
 = uart_write
~~snip~~
Seeing this call sequences, the output need to be
'_raw_spin_unlock_irqrestore - uart_start'.
But, the CALLER_ADDR1 in trace_hardirqs_off() returned correct value.
So there's no problem in output.
I think trace_hardirqs_off() should call CALLER_ADDR1 and CALLER_ADDR2
respectively for its arguments for start_critical_timing(). This
thought leads me to the necessity of creating architecture specific
trace_hardirqs_off function. Any opinion on this?

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

Re: [PATCH 2/2] arm: make return_address available for ARM_UNWIND

[Keun-O Park]

On Mon, Jan 7, 2013 at 10:41 PM, Steven Rostedt  wrote:
>
> On Fri, 2013-01-04 at 17:45 +0900, Keun-O Park wrote:
>
> > With "CFLAGS_REMOVE_unwind.o = -pg" and with CONFIG_PROVE_LOCKING
> > turned on, I confirmed that
> > there's no trace output like Steve mentioned.
> > However, if I turn off CONFIG_PROVE_LOCKING, "irqsoff" and
> > "preemptirqsoff" ftracer prints these lines :
> >
> > kernel_text_address <-unwind_frame
> > core_kernel_text <-unwind_frame
> > search_index <-unwind_frame
> >
> > While I investigated the reason, I just found out there's two function
> > with same name, trace_hardirqs_off.
> > One in kernel/trace/trace_irqsoff.c and another in kernel/lockdep.c.
> > And both symbols are exported.
> > I am wondering whether it is intentionally maintained code to
> > manipulate ftrace and lockdep or not.
> > I guess it's probably not.
>
> Both the irqsoff tracer from ftrace and lockdep came from the -rt patch.
> The two were very integrated at the time. When they were ported over to
> mainline, they still used the same infrastructure to hook into the
> locations of interrupts being disabled or enabled.
>
> trace_hardirqs_on/off() is the function that is called for both lockdep
> and ftrace's irqsoff tracer. So this was intentional. That way we didn't
> need to have two different callers at every location. But if lockdep
> isn't defined and ftrace irqsoff is, then ftrace would define the
> trace_hardirqs_on/off() routines, otherwise lockdep does. (These
> routines are within CONFIG_PROVE_LOCKING #ifdefs in
> kernel/trace/trace_irqsoff.c)
>
> When both lockdep and irqsoff tracer are configured, then lockdep
> defines the trace_hardirqs_off_caller(), and calls time_hardirqs_off()
> in trace_irqsoff.c which does the same thing as the trace_hardirqs_off()
> does without lockdep.
>
> I'm not sure why one would add the annotations while adding
> PROVE_LOCKING doesn't. They both seems to use CALLER_ADDR0, but maybe
> there's another path that uses CALLER_ADDR1 without it.
>
> -- Steve
>
>

Hello Steve,

Much obliged for your explaining this.
As your guess, there's another path that uses CALLER_ADDR1 without
CONFIG_PROVE_LOCKING though both lockdep and ftrace's irqsoff tracer
is turned on. In this case, ftrace's trace_hardirqs_on/off() would be
called.
And, as you said to me, if lockdep is off and ftrace's irqsoff is on,
ftrace's trace_hardirqs_on/off() would be called.
I think the proper way to avoid calling CALLER_ADDR1 will be calling
trace_hardirqs_off_caller() instead of calling stop_critical_timing()
directly in trace_hardirqs_off(). And, this will suppress the message
outputs of unwind_frame.
If you think this idea is okay, I will push the patch v2 to you.
In my test result, it looks it's functionally correct.

Thanks.

-- kpark


diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index f89515a..3552ad9 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -32,13 +32,11 @@ extern void ftrace_call_old(void);

 #ifndef __ASSEMBLY__

-#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
+#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND)
 /*
  * return_address uses walk_stackframe to do it's work.  If both
  * CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind
- * information.  For this to work in the function tracer many functions would
- * have to be marked with __notrace.  So for now just depend on
- * !CONFIG_ARM_UNWIND.
+ * information.
  */

 void *return_address(unsigned int);
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 5bbec7b..675fd62 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -9,6 +9,9 @@ ifdef CONFIG_FUNCTION_TRACER
 CFLAGS_REMOVE_ftrace.o = -pg
 CFLAGS_REMOVE_insn.o = -pg
 CFLAGS_REMOVE_patch.o = -pg
+ifdef CONFIG_ARM_UNWIND
+CFLAGS_REMOVE_unwind.o = -pg
+endif
 endif

 CFLAGS_REMOVE_return_address.o = -pg
diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index fafedd8..f202bc0 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -11,7 +11,7 @@
 #include 
 #include 

-#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
+#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND)
 #include 

 #include 
@@ -57,17 +57,13 @@ void *return_address(unsigned int level)
return NULL;
 }

-#else /* if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) */
-
-#if defined(CONFIG_ARM_UNWIND)
-#warning "TODO: return_address should use unwind tables"
-#endif
+#else /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */

 void *return_address(unsigned int level)
 {
return NULL;
 }

-#

Re: [PATCH 2/2] arm: make return_address available for ARM_UNWIND

[Keun-O Park]

On Mon, Jan 7, 2013 at 10:41 PM, Steven Rostedt rost...@goodmis.org wrote:

 On Fri, 2013-01-04 at 17:45 +0900, Keun-O Park wrote:

  With CFLAGS_REMOVE_unwind.o = -pg and with CONFIG_PROVE_LOCKING
  turned on, I confirmed that
  there's no trace output like Steve mentioned.
  However, if I turn off CONFIG_PROVE_LOCKING, irqsoff and
  preemptirqsoff ftracer prints these lines :
 
  kernel_text_address -unwind_frame
  core_kernel_text -unwind_frame
  search_index -unwind_frame
 
  While I investigated the reason, I just found out there's two function
  with same name, trace_hardirqs_off.
  One in kernel/trace/trace_irqsoff.c and another in kernel/lockdep.c.
  And both symbols are exported.
  I am wondering whether it is intentionally maintained code to
  manipulate ftrace and lockdep or not.
  I guess it's probably not.

 Both the irqsoff tracer from ftrace and lockdep came from the -rt patch.
 The two were very integrated at the time. When they were ported over to
 mainline, they still used the same infrastructure to hook into the
 locations of interrupts being disabled or enabled.

 trace_hardirqs_on/off() is the function that is called for both lockdep
 and ftrace's irqsoff tracer. So this was intentional. That way we didn't
 need to have two different callers at every location. But if lockdep
 isn't defined and ftrace irqsoff is, then ftrace would define the
 trace_hardirqs_on/off() routines, otherwise lockdep does. (These
 routines are within CONFIG_PROVE_LOCKING #ifdefs in
 kernel/trace/trace_irqsoff.c)

 When both lockdep and irqsoff tracer are configured, then lockdep
 defines the trace_hardirqs_off_caller(), and calls time_hardirqs_off()
 in trace_irqsoff.c which does the same thing as the trace_hardirqs_off()
 does without lockdep.

 I'm not sure why one would add the annotations while adding
 PROVE_LOCKING doesn't. They both seems to use CALLER_ADDR0, but maybe
 there's another path that uses CALLER_ADDR1 without it.

 -- Steve



Hello Steve,

Much obliged for your explaining this.
As your guess, there's another path that uses CALLER_ADDR1 without
CONFIG_PROVE_LOCKING though both lockdep and ftrace's irqsoff tracer
is turned on. In this case, ftrace's trace_hardirqs_on/off() would be
called.
And, as you said to me, if lockdep is off and ftrace's irqsoff is on,
ftrace's trace_hardirqs_on/off() would be called.
I think the proper way to avoid calling CALLER_ADDR1 will be calling
trace_hardirqs_off_caller() instead of calling stop_critical_timing()
directly in trace_hardirqs_off(). And, this will suppress the message
outputs of unwind_frame.
If you think this idea is okay, I will push the patch v2 to you.
In my test result, it looks it's functionally correct.

Thanks.

-- kpark


diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index f89515a..3552ad9 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -32,13 +32,11 @@ extern void ftrace_call_old(void);

 #ifndef __ASSEMBLY__

-#if defined(CONFIG_FRAME_POINTER)  !defined(CONFIG_ARM_UNWIND)
+#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND)
 /*
  * return_address uses walk_stackframe to do it's work.  If both
  * CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind
- * information.  For this to work in the function tracer many functions would
- * have to be marked with __notrace.  So for now just depend on
- * !CONFIG_ARM_UNWIND.
+ * information.
  */

 void *return_address(unsigned int);
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 5bbec7b..675fd62 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -9,6 +9,9 @@ ifdef CONFIG_FUNCTION_TRACER
 CFLAGS_REMOVE_ftrace.o = -pg
 CFLAGS_REMOVE_insn.o = -pg
 CFLAGS_REMOVE_patch.o = -pg
+ifdef CONFIG_ARM_UNWIND
+CFLAGS_REMOVE_unwind.o = -pg
+endif
 endif

 CFLAGS_REMOVE_return_address.o = -pg
diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index fafedd8..f202bc0 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -11,7 +11,7 @@
 #include linux/export.h
 #include linux/ftrace.h

-#if defined(CONFIG_FRAME_POINTER)  !defined(CONFIG_ARM_UNWIND)
+#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND)
 #include linux/sched.h

 #include asm/stacktrace.h
@@ -57,17 +57,13 @@ void *return_address(unsigned int level)
return NULL;
 }

-#else /* if defined(CONFIG_FRAME_POINTER)  !defined(CONFIG_ARM_UNWIND) */
-
-#if defined(CONFIG_ARM_UNWIND)
-#warning TODO: return_address should use unwind tables
-#endif
+#else /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */

 void *return_address(unsigned int level)
 {
return NULL;
 }

-#endif /* if defined(CONFIG_FRAME_POINTER) 
!defined(CONFIG_ARM_UNWIND) / else */
+#endif /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */

 EXPORT_SYMBOL_GPL(return_address);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 00f79e5

[PATCH] arm: fix returning wrong CALLER_ADDRx

[Keun-O Park]

From: sahara 

This makes return_address return correct value for ftrace feature.
unwind_frame does not update frame->lr but frame->pc for backtrace.
And, the initialization for data.addr was missing so that wrong value
returned when unwind_frame failed.

Signed-off-by: sahara 
---
 arch/arm/kernel/return_address.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index 8085417..fafedd8 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -26,7 +26,7 @@ static int save_return_addr(struct stackframe *frame, void *d)
struct return_address_data *data = d;
 
if (!data->level) {
-   data->addr = (void *)frame->lr;
+   data->addr = (void *)frame->pc;
 
return 1;
} else {
@@ -41,7 +41,8 @@ void *return_address(unsigned int level)
struct stackframe frame;
register unsigned long current_sp asm ("sp");
 
-   data.level = level + 1;
+   data.level = level + 2;
+   data.addr = NULL;
 
frame.fp = (unsigned long)__builtin_frame_address(0);
frame.sp = current_sp;
-- 
1.7.1

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

[PATCH] arm: fix returning wrong CALLER_ADDRx

[Keun-O Park]

From: sahara keun-o.p...@windriver.com

This makes return_address return correct value for ftrace feature.
unwind_frame does not update frame-lr but frame-pc for backtrace.
And, the initialization for data.addr was missing so that wrong value
returned when unwind_frame failed.

Signed-off-by: sahara keun-o.p...@windriver.com
---
 arch/arm/kernel/return_address.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index 8085417..fafedd8 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -26,7 +26,7 @@ static int save_return_addr(struct stackframe *frame, void *d)
struct return_address_data *data = d;
 
if (!data-level) {
-   data-addr = (void *)frame-lr;
+   data-addr = (void *)frame-pc;
 
return 1;
} else {
@@ -41,7 +41,8 @@ void *return_address(unsigned int level)
struct stackframe frame;
register unsigned long current_sp asm (sp);
 
-   data.level = level + 1;
+   data.level = level + 2;
+   data.addr = NULL;
 
frame.fp = (unsigned long)__builtin_frame_address(0);
frame.sp = current_sp;
-- 
1.7.1

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

Re: [PATCH 2/2] arm: make return_address available for ARM_UNWIND

[Keun-O Park]

On Fri, Jan 4, 2013 at 1:58 AM, Steven Rostedt  wrote:
> On Thu, 2013-01-03 at 16:23 +, Russell King - ARM Linux wrote:
>> On Thu, Jan 03, 2013 at 11:03:58AM -0500, Steven Rostedt wrote:
>> > On Thu, 2013-01-03 at 14:13 +, Russell King - ARM Linux wrote:
>> > > In summary, from what I can see in the patch, the reason why the ifdefs
>> > > are the way they are, and the reason the warning is there has not been
>> > > addressed; these patches just seem to be aimed just at removing a 
>> > > #warning
>> > > statement to make the warning go away.
>> >
>> > You're correct that this patch does not solve any of theses issues. Now,
>> > I'm thinking that ftrace has matured where these issues don't exist, and
>> > where they do, it will only cause noise in the trace than anything
>> > serious. But, this needs to be looked deeper to make sure.
>>
>> Looking back in the archives, it seems that we had a problem with ftrace
>> and the unwinder polluting the trace information:
>>
>> http://lists.arm.linux.org.uk/lurker/message/20090604.201745.1c41ee6c.en.html
>>
>> There's quite a bit of discussion in that thread about this which details
>> why we came up with what we have today.
>
> Thanks for the link. In fact, I see I was even involved :-)
>
> http://lists.arm.linux.org.uk/lurker/message/20090609.213448.b4e4d096.en.html
>
> Just as I explained, the problem isn't a recursion bug, but just noise
> in the trace.
>
> Some of what is called by unwind_frame() will always be traced, and I
> wouldn't want 'notrace' annotation on those.
>
> If anything, I would just suggest another config option that lets the
> user decide if they don't mind the messiness of the trace for the added
> benefit of where the latency output came from.
>
> Also, it's rather trivial to make the entire unwind.c not traced, by
> adding in the Makefile:
>
> CFLAGS_REMOVE_unwind.o = -pg
>
> But that only stops the tracing of the unwind_*. Looks to be a lot of
> those. But it wont stop the tracing of:
>
>  kernel_text_address <-unwind_frame
>  core_kernel_text <-unwind_frame
>  search_index <-unwind_frame
>  etc.
>
> Heck, the user could just keep those from being traced by doing:
>
>  echo kernel_text_address core_kernel_text search_index > \
>/sys/kernel/debug/tracing/set_ftrace_notrace
>
> If DYNAMIC_FTRACE is defined.
>
> -- Steve
>
>

With "CFLAGS_REMOVE_unwind.o = -pg" and with CONFIG_PROVE_LOCKING
turned on, I confirmed that
there's no trace output like Steve mentioned.
However, if I turn off CONFIG_PROVE_LOCKING, "irqsoff" and
"preemptirqsoff" ftracer prints these lines :

kernel_text_address <-unwind_frame
core_kernel_text <-unwind_frame
search_index <-unwind_frame

While I investigated the reason, I just found out there's two function
with same name, trace_hardirqs_off.
One in kernel/trace/trace_irqsoff.c and another in kernel/lockdep.c.
And both symbols are exported.
I am wondering whether it is intentionally maintained code to
manipulate ftrace and lockdep or not.
I guess it's probably not.

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

Re: [PATCH 2/2] arm: make return_address available for ARM_UNWIND

[Keun-O Park]

On Fri, Jan 4, 2013 at 1:58 AM, Steven Rostedt rost...@goodmis.org wrote:
 On Thu, 2013-01-03 at 16:23 +, Russell King - ARM Linux wrote:
 On Thu, Jan 03, 2013 at 11:03:58AM -0500, Steven Rostedt wrote:
  On Thu, 2013-01-03 at 14:13 +, Russell King - ARM Linux wrote:
   In summary, from what I can see in the patch, the reason why the ifdefs
   are the way they are, and the reason the warning is there has not been
   addressed; these patches just seem to be aimed just at removing a 
   #warning
   statement to make the warning go away.
 
  You're correct that this patch does not solve any of theses issues. Now,
  I'm thinking that ftrace has matured where these issues don't exist, and
  where they do, it will only cause noise in the trace than anything
  serious. But, this needs to be looked deeper to make sure.

 Looking back in the archives, it seems that we had a problem with ftrace
 and the unwinder polluting the trace information:

 http://lists.arm.linux.org.uk/lurker/message/20090604.201745.1c41ee6c.en.html

 There's quite a bit of discussion in that thread about this which details
 why we came up with what we have today.

 Thanks for the link. In fact, I see I was even involved :-)

 http://lists.arm.linux.org.uk/lurker/message/20090609.213448.b4e4d096.en.html

 Just as I explained, the problem isn't a recursion bug, but just noise
 in the trace.

 Some of what is called by unwind_frame() will always be traced, and I
 wouldn't want 'notrace' annotation on those.

 If anything, I would just suggest another config option that lets the
 user decide if they don't mind the messiness of the trace for the added
 benefit of where the latency output came from.

 Also, it's rather trivial to make the entire unwind.c not traced, by
 adding in the Makefile:

 CFLAGS_REMOVE_unwind.o = -pg

 But that only stops the tracing of the unwind_*. Looks to be a lot of
 those. But it wont stop the tracing of:

  kernel_text_address -unwind_frame
  core_kernel_text -unwind_frame
  search_index -unwind_frame
  etc.

 Heck, the user could just keep those from being traced by doing:

  echo kernel_text_address core_kernel_text search_index  \
/sys/kernel/debug/tracing/set_ftrace_notrace

 If DYNAMIC_FTRACE is defined.

 -- Steve



With CFLAGS_REMOVE_unwind.o = -pg and with CONFIG_PROVE_LOCKING
turned on, I confirmed that
there's no trace output like Steve mentioned.
However, if I turn off CONFIG_PROVE_LOCKING, irqsoff and
preemptirqsoff ftracer prints these lines :

kernel_text_address -unwind_frame
core_kernel_text -unwind_frame
search_index -unwind_frame

While I investigated the reason, I just found out there's two function
with same name, trace_hardirqs_off.
One in kernel/trace/trace_irqsoff.c and another in kernel/lockdep.c.
And both symbols are exported.
I am wondering whether it is intentionally maintained code to
manipulate ftrace and lockdep or not.
I guess it's probably not.

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