[PATCH 0/2] Fix tracing with non-continuous cpu ids

[PATCH 0/2] Fix tracing with non-continuous cpu ids

[Gil Fruchter]

Currently exception occures due to access beyond buffer_iter
range while using index of cpu bigger than num_possible_cpus().

In order to fix buffer allocation size for non-continuous cpu ids
we allocate according to the max cpu id and not according to the
amount of possible cpus.

This series also includes a checkpatch warning fix for using
kzalloc with multiply instead of kcalloc.

Happy tracing,
Gil

Gil Fruchter (2):
  tracing: Prefer kcalloc over kzalloc with multiply
  tracing: Fix for non-continuous cpu ids

 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

[PATCH 1/2] tracing: Prefer kcalloc over kzalloc with multiply

[Gil Fruchter]

fix checkpatch warning for using kzalloc with multiply:
WARNING: Prefer kcalloc over kzalloc with multiply
+ iter->buffer_iter = kzalloc(sizeof(*iter->buffer_iter)
						* num_possible_cpus(),

Signed-off-by: Gil Fruchter <gilf@ezchip.com>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0533049..57ffc4a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3034,7 +3034,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 
-	iter->buffer_iter = kzalloc(sizeof(*iter->buffer_iter) * num_possible_cpus(),
+	iter->buffer_iter = kcalloc(num_possible_cpus(), sizeof(*iter->buffer_iter),
 				    GFP_KERNEL);
 	if (!iter->buffer_iter)
 		goto release;
-- 
1.7.1

Re: [PATCH 1/2] tracing: Prefer kcalloc over kzalloc with multiply

[Steven Rostedt]

On Tue, 2 Jun 2015 16:26:28 +0300
Gil Fruchter <gilf@ezchip.com> wrote:

> fix checkpatch warning for using kzalloc with multiply:
> WARNING: Prefer kcalloc over kzalloc with multiply
> + iter->buffer_iter = kzalloc(sizeof(*iter->buffer_iter)
> 						* num_possible_cpus(),

I'm not against applying this patch, but "fix checkpatch warning" is
never rational for a change.

This should say something like, "Use kcalloc for allocating an array
instead of a simple multiplier with kzalloc, as that is what kcalloc
is used for."

In other words, if checkpatch found something that should be change,
have the change log tell us why it should be changed. You can than add
a "Found with checkpatch" to give checkpatch the credit.

-- Steve

> 
> Signed-off-by: Gil Fruchter <gilf@ezchip.com>
> ---
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 0533049..57ffc4a 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3034,7 +3034,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  	if (!iter)
>  		return ERR_PTR(-ENOMEM);
>  
> -	iter->buffer_iter = kzalloc(sizeof(*iter->buffer_iter) * num_possible_cpus(),
> +	iter->buffer_iter = kcalloc(num_possible_cpus(), sizeof(*iter->buffer_iter),
>  				    GFP_KERNEL);
>  	if (!iter->buffer_iter)
>  		goto release;

RE: [PATCH 1/2] tracing: Prefer kcalloc over kzalloc with multiply

[Gil Fruchter]

On Tue, Jun 2, 2015 at 5:15 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2 Jun 2015 16:26:28 +0300
> Gil Fruchter <gilf@ezchip.com> wrote:
> 
>> fix checkpatch warning for using kzalloc with multiply:
>> WARNING: Prefer kcalloc over kzalloc with multiply
>> + iter->buffer_iter = kzalloc(sizeof(*iter->buffer_iter)
>> 						* num_possible_cpus(),
> 
> I'm not against applying this patch, but "fix checkpatch warning" is never rational for a change.
> 
> This should say something like, "Use kcalloc for allocating an array instead of a simple multiplier with kzalloc, as that is what kcalloc is used for."
> 
> In other words, if checkpatch found something that should be change, have the change log tell us why it should be changed. You can than add a "Found with checkpatch" to give checkpatch the credit.
> 
> -- Steve
> 

Thanks for the input Steve.
I will create a v2 with an appropriate explanation for this change.

Can I add a Reviewed-by tag by you?

Gil 

Re: [PATCH 1/2] tracing: Prefer kcalloc over kzalloc with multiply

[Steven Rostedt]

Sorry for the late reply, just got back from travels.

On Wed, 3 Jun 2015 11:53:59 +0000
Gil Fruchter <gilf@ezchip.com> wrote:

> 
> Thanks for the input Steve.
> I will create a v2 with an appropriate explanation for this change.
> 
> Can I add a Reviewed-by tag by you?
> 

Is this going to go through another tree other than tracing?

That is, if it is going to go through my tree, it will require my
Signed-off-by, and that implies Reviewed-by.

-- Steve

[PATCH 2/2] tracing: Fix for non-continuous cpu ids

[Gil Fruchter]

Currently exception occures due to access beyond buffer_iter
range while using index of cpu bigger than num_possible_cpus().
Below there is an example for such exception when we use
cpus 0,1,16,17.

In order to fix buffer allocation size for non-continuous cpu ids
we allocate according to the max cpu id and not according to the
amount of possible cpus.

Example:
  $ cat /sys/kernel/debug/tracing/per_cpu/cpu1/trace
  Path: /bin/busybox
  CPU: 0 PID: 82 Comm: cat Not tainted 4.0.0 #29
  task: 80734c80 ti: 80012000 task.ti: 80012000

  [ECR   ]: 0x00220100 => Invalid Read @ 0x00000000 by insn @ 0x800abafc
  [EFA   ]: 0x00000000
  [BLINK ]: ring_buffer_read_finish+0x24/0x64
  [ERET  ]: rb_check_pages+0x20/0x188
  [STAT32]: 0x00001a00 :
  BTA: 0x800abafc  SP: 0x80013f0c  FP: 0x57719cf8
  LPS: 0x200036b4 LPE: 0x200036b8 LPC: 0x00000000
  r00: 0x8002aca0 r01: 0x00001606 r02: 0x00000000
  r03: 0x00000001 r04: 0x00000000 r05: 0x804b4954
  r06: 0x00030003 r07: 0x8002a260 r08: 0x00000286
  r09: 0x00080002 r10: 0x00001006 r11: 0x807351a4
  r12: 0x00000001

  Stack Trace:
    rb_check_pages+0x20/0x188
    ring_buffer_read_finish+0x24/0x64
    tracing_release+0x4e/0x170
    __fput+0x62/0x158
    task_work_run+0xa2/0xd4
    do_notify_resume+0x52/0x7c
    resume_user_mode_begin+0xdc/0xe0

Signed-off-by: Noam Camus <noamc@ezchip.com>
Signed-off-by: Gil Fruchter <gilf@ezchip.com>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 57ffc4a..8c5b382 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3034,7 +3034,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 
-	iter->buffer_iter = kcalloc(num_possible_cpus(), sizeof(*iter->buffer_iter),
+	iter->buffer_iter = kcalloc(nr_cpu_ids, sizeof(*iter->buffer_iter),
 				    GFP_KERNEL);
 	if (!iter->buffer_iter)
 		goto release;
-- 
1.7.1

Re: [PATCH 2/2] tracing: Fix for non-continuous cpu ids

[Steven Rostedt]

On Tue, 2 Jun 2015 16:26:29 +0300
Gil Fruchter <gilf@ezchip.com> wrote:

> Currently exception occures due to access beyond buffer_iter
> range while using index of cpu bigger than num_possible_cpus().
> Below there is an example for such exception when we use
> cpus 0,1,16,17.

I'm curious, what broken boxes have non-continuous cpu ids?

> 
> In order to fix buffer allocation size for non-continuous cpu ids
> we allocate according to the max cpu id and not according to the
> amount of possible cpus.
> 
> Example:
>   $ cat /sys/kernel/debug/tracing/per_cpu/cpu1/trace
>   Path: /bin/busybox
>   CPU: 0 PID: 82 Comm: cat Not tainted 4.0.0 #29
>   task: 80734c80 ti: 80012000 task.ti: 80012000
> 
>   [ECR   ]: 0x00220100 => Invalid Read @ 0x00000000 by insn @ 0x800abafc
>   [EFA   ]: 0x00000000
>   [BLINK ]: ring_buffer_read_finish+0x24/0x64
>   [ERET  ]: rb_check_pages+0x20/0x188
>   [STAT32]: 0x00001a00 :
>   BTA: 0x800abafc  SP: 0x80013f0c  FP: 0x57719cf8
>   LPS: 0x200036b4 LPE: 0x200036b8 LPC: 0x00000000
>   r00: 0x8002aca0 r01: 0x00001606 r02: 0x00000000
>   r03: 0x00000001 r04: 0x00000000 r05: 0x804b4954
>   r06: 0x00030003 r07: 0x8002a260 r08: 0x00000286
>   r09: 0x00080002 r10: 0x00001006 r11: 0x807351a4
>   r12: 0x00000001
> 
>   Stack Trace:
>     rb_check_pages+0x20/0x188
>     ring_buffer_read_finish+0x24/0x64
>     tracing_release+0x4e/0x170
>     __fput+0x62/0x158
>     task_work_run+0xa2/0xd4
>     do_notify_resume+0x52/0x7c
>     resume_user_mode_begin+0xdc/0xe0

How common is this? Is this something that should be marked for stable?

-- Steve


> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> Signed-off-by: Gil Fruchter <gilf@ezchip.com>
> ---
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 57ffc4a..8c5b382 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3034,7 +3034,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  	if (!iter)
>  		return ERR_PTR(-ENOMEM);
>  
> -	iter->buffer_iter = kcalloc(num_possible_cpus(), sizeof(*iter->buffer_iter),
> +	iter->buffer_iter = kcalloc(nr_cpu_ids, sizeof(*iter->buffer_iter),
>  				    GFP_KERNEL);
>  	if (!iter->buffer_iter)
>  		goto release;

Re: [PATCH 2/2] tracing: Fix for non-continuous cpu ids

[Gilad Ben-Yossef]

On Tue, Jun 2, 2015 at 5:26 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2 Jun 2015 16:26:29 +0300
> Gil Fruchter <gilf@ezchip.com> wrote:
>
>> Currently exception occures due to access beyond buffer_iter
>> range while using index of cpu bigger than num_possible_cpus().
>> Below there is an example for such exception when we use
>> cpus 0,1,16,17.
>
> I'm curious, what broken boxes have non-continuous cpu ids?

The NPS-400 is a single SoC with 256 cores based on Synopsys Arc, each
one is capable to work as up to 16 SMT hardware threads or logical
cores. I gave a talk about it two years ago at Plumbers/LinuxCon.

Most of  the Linux support is in the process being up-streamed via the
Arc architecture maintainer (the patch set is at
https://github.com/EZchip/linux) but there are some none architecture
specific patches that we just happen to run into like this one.

As to the why, well, the CPU ids has a mapping to the topology - which
cluster (NUMA node), core and SMT thread you are looking at.

The thing is, the number of SMT threads per core is configurable, and
you can hot plug down a core, reconfigure it for a different number of
SMT thread and hot them back in. So the "empty slots" are possible
logical cores that don't happen to be active at the time, but could
hotplug up later on.

I hope this makes some sense.

Since the NPS-400 is not generally available yet I wouldn't worry too
much about stable kernel for now...

Thanks,

Gilad Ben-Yossef


-- 
Gilad Ben-Yossef
Chief Coffee Drinker
gilad@benyossef.com
Israel Cell: +972-52-8260388
US Cell: +1-973-8260388
http://benyossef.com

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

[PATCH v2 0/2] Fix tracing with non-continuous cpu ids

[Gil Fruchter]

Currently exception occures due to access beyond buffer_iter
range while using index of cpu bigger than num_possible_cpus().

In order to fix buffer allocation size for non-continuous cpu ids
we allocate according to the max cpu id and not according to the
amount of possible cpus.

This series also includes a checkpatch warning fix for using
kzalloc with multiply instead of kcalloc.

Happy tracing,
Gil

v2:
  Update commit description for patch 1/2.

Gil Fruchter (2):
  tracing: Prefer kcalloc over kzalloc with multiply
  tracing: Fix for non-continuous cpu ids

 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

[PATCH v2 1/2] tracing: Prefer kcalloc over kzalloc with multiply

[Gil Fruchter]

Use kcalloc for allocating an array instead of kzalloc with multiply,
as that is what kcalloc is used for.
Found with checkpatch.

Signed-off-by: Gil Fruchter <gilf@ezchip.com>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0533049..57ffc4a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3034,7 +3034,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 
-	iter->buffer_iter = kzalloc(sizeof(*iter->buffer_iter) * num_possible_cpus(),
+	iter->buffer_iter = kcalloc(num_possible_cpus(), sizeof(*iter->buffer_iter),
 				    GFP_KERNEL);
 	if (!iter->buffer_iter)
 		goto release;
-- 
1.7.1

[PATCH v2 2/2] tracing: Fix for non-continuous cpu ids

[Gil Fruchter]

Currently exception occures due to access beyond buffer_iter
range while using index of cpu bigger than num_possible_cpus().
Below there is an example for such exception when we use
cpus 0,1,16,17.

In order to fix buffer allocation size for non-continuous cpu ids
we allocate according to the max cpu id and not according to the
amount of possible cpus.

Example:
  $ cat /sys/kernel/debug/tracing/per_cpu/cpu1/trace
  Path: /bin/busybox
  CPU: 0 PID: 82 Comm: cat Not tainted 4.0.0 #29
  task: 80734c80 ti: 80012000 task.ti: 80012000

  [ECR   ]: 0x00220100 => Invalid Read @ 0x00000000 by insn @ 0x800abafc
  [EFA   ]: 0x00000000
  [BLINK ]: ring_buffer_read_finish+0x24/0x64
  [ERET  ]: rb_check_pages+0x20/0x188
  [STAT32]: 0x00001a00 :
  BTA: 0x800abafc  SP: 0x80013f0c  FP: 0x57719cf8
  LPS: 0x200036b4 LPE: 0x200036b8 LPC: 0x00000000
  r00: 0x8002aca0 r01: 0x00001606 r02: 0x00000000
  r03: 0x00000001 r04: 0x00000000 r05: 0x804b4954
  r06: 0x00030003 r07: 0x8002a260 r08: 0x00000286
  r09: 0x00080002 r10: 0x00001006 r11: 0x807351a4
  r12: 0x00000001

  Stack Trace:
    rb_check_pages+0x20/0x188
    ring_buffer_read_finish+0x24/0x64
    tracing_release+0x4e/0x170
    __fput+0x62/0x158
    task_work_run+0xa2/0xd4
    do_notify_resume+0x52/0x7c
    resume_user_mode_begin+0xdc/0xe0

Signed-off-by: Noam Camus <noamc@ezchip.com>
Signed-off-by: Gil Fruchter <gilf@ezchip.com>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 57ffc4a..8c5b382 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3034,7 +3034,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
 	if (!iter)
 		return ERR_PTR(-ENOMEM);
 
-	iter->buffer_iter = kcalloc(num_possible_cpus(), sizeof(*iter->buffer_iter),
+	iter->buffer_iter = kcalloc(nr_cpu_ids, sizeof(*iter->buffer_iter),
 				    GFP_KERNEL);
 	if (!iter->buffer_iter)
 		goto release;
-- 
1.7.1

Re: [PATCH v2 2/2] tracing: Fix for non-continuous cpu ids

[Steven Rostedt]

On Tue, 9 Jun 2015 10:32:35 +0300
Gil Fruchter <gilf@ezchip.com> wrote:

> Currently exception occures due to access beyond buffer_iter
> range while using index of cpu bigger than num_possible_cpus().
> Below there is an example for such exception when we use
> cpus 0,1,16,17.
> 
> In order to fix buffer allocation size for non-continuous cpu ids
> we allocate according to the max cpu id and not according to the
> amount of possible cpus.
> 
> Example:
>   $ cat /sys/kernel/debug/tracing/per_cpu/cpu1/trace
>   Path: /bin/busybox
>   CPU: 0 PID: 82 Comm: cat Not tainted 4.0.0 #29
>   task: 80734c80 ti: 80012000 task.ti: 80012000
> 
>   [ECR   ]: 0x00220100 => Invalid Read @ 0x00000000 by insn @ 0x800abafc
>   [EFA   ]: 0x00000000
>   [BLINK ]: ring_buffer_read_finish+0x24/0x64
>   [ERET  ]: rb_check_pages+0x20/0x188
>   [STAT32]: 0x00001a00 :
>   BTA: 0x800abafc  SP: 0x80013f0c  FP: 0x57719cf8
>   LPS: 0x200036b4 LPE: 0x200036b8 LPC: 0x00000000
>   r00: 0x8002aca0 r01: 0x00001606 r02: 0x00000000
>   r03: 0x00000001 r04: 0x00000000 r05: 0x804b4954
>   r06: 0x00030003 r07: 0x8002a260 r08: 0x00000286
>   r09: 0x00080002 r10: 0x00001006 r11: 0x807351a4
>   r12: 0x00000001
> 
>   Stack Trace:
>     rb_check_pages+0x20/0x188
>     ring_buffer_read_finish+0x24/0x64
>     tracing_release+0x4e/0x170
>     __fput+0x62/0x158
>     task_work_run+0xa2/0xd4
>     do_notify_resume+0x52/0x7c
>     resume_user_mode_begin+0xdc/0xe0
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>

Why Noam's SoB tag? Did he author it?

-- Steve

> Signed-off-by: Gil Fruchter <gilf@ezchip.com>
> ---
>  kernel/trace/trace.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 57ffc4a..8c5b382 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3034,7 +3034,7 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot)
>  	if (!iter)
>  		return ERR_PTR(-ENOMEM);
>  
> -	iter->buffer_iter = kcalloc(num_possible_cpus(), sizeof(*iter->buffer_iter),
> +	iter->buffer_iter = kcalloc(nr_cpu_ids, sizeof(*iter->buffer_iter),
>  				    GFP_KERNEL);
>  	if (!iter->buffer_iter)
>  		goto release;

RE: [PATCH v2 2/2] tracing: Fix for non-continuous cpu ids

[Gil Fruchter]

On Monday, July 13, 2015 7:12 PM
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 9 Jun 2015 10:32:35 +0300
> Gil Fruchter <gilf@ezchip.com> wrote:
>
> > Currently exception occures due to access beyond buffer_iter range 
> > while using index of cpu bigger than num_possible_cpus().
> > Below there is an example for such exception when we use cpus 
> >  0,1,16,17.
> >
> > In order to fix buffer allocation size for non-continuous cpu ids we 
> > allocate according to the max cpu id and not according to the amount 
> > of possible cpus.
> >
> > Example:
> >   $ cat /sys/kernel/debug/tracing/per_cpu/cpu1/trace
> >   Path: /bin/busybox
> >   CPU: 0 PID: 82 Comm: cat Not tainted 4.0.0 #29
> >   task: 80734c80 ti: 80012000 task.ti: 80012000
> >
> >   [ECR   ]: 0x00220100 => Invalid Read @ 0x00000000 by insn @ 0x800abafc
> >   [EFA   ]: 0x00000000
> >   [BLINK ]: ring_buffer_read_finish+0x24/0x64
> >   [ERET  ]: rb_check_pages+0x20/0x188
> >   [STAT32]: 0x00001a00 :
> >   BTA: 0x800abafc  SP: 0x80013f0c  FP: 0x57719cf8
> >   LPS: 0x200036b4 LPE: 0x200036b8 LPC: 0x00000000
> >   r00: 0x8002aca0 r01: 0x00001606 r02: 0x00000000
> >   r03: 0x00000001 r04: 0x00000000 r05: 0x804b4954
> >   r06: 0x00030003 r07: 0x8002a260 r08: 0x00000286
> >   r09: 0x00080002 r10: 0x00001006 r11: 0x807351a4
> >   r12: 0x00000001
> >
> >   Stack Trace:
> >     rb_check_pages+0x20/0x188
> >     ring_buffer_read_finish+0x24/0x64
> >     tracing_release+0x4e/0x170
> >     __fput+0x62/0x158
> >     task_work_run+0xa2/0xd4
> >     do_notify_resume+0x52/0x7c
> >     resume_user_mode_begin+0xdc/0xe0
> >
> > Signed-off-by: Noam Camus <noamc@ezchip.com>
>
> Why Noam's SoB tag? Did he author it?
>
> -- Steve
>

He didn't author it, but we worked on this issue together.

Gil.

Re: [PATCH v2 2/2] tracing: Fix for non-continuous cpu ids

[Steven Rostedt]

On Tue, 14 Jul 2015 10:12:47 +0000
Gil Fruchter <gilf@ezchip.com> wrote:

> > > Signed-off-by: Noam Camus <noamc@ezchip.com>
> >
> > Why Noam's SoB tag? Did he author it?
> >
> > -- Steve
> >
> 
> He didn't author it, but we worked on this issue together.

OK, thanks. I just like to know why someone's name appears without any
other comment.

-- Steve