Fix tracing with non-continuous cpu ids

PATCH Superseded Gil Fruchter 3 participants 14 messages Jun 2, 2015 – Jul 14, 2015 Tracing

Share & tools

Gil Fruchter

author

2015-06-02 13:43 UTC #0 lore raw

cover Fix tracing with non-continuous cpu ids

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(-)

Gil Fruchter

author

2015-06-02 13:41 UTC #1 lore raw

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

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 <[email protected]>

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

kernel/trace/trace.c +1 -1

index 0533049..57ffc4a 100644
@@ -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;

Signature · 1 line

1.7.1

Steven Rostedt

maintainer

2015-06-02 14:15 UTC #2 lore raw

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

Quoted 4 lines · On Tue, 2 Jun 2015 16:26:28 +0300 Gil Fruchter <[email protected]> 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

Quoted 19 lines

Signed-off-by Gil Fruchter <[email protected]>

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

kernel/trace/trace.c +1 -1

index 0533049..57ffc4a 100644
@@ -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;

Gil Fruchter

author

2015-06-03 11:54 UTC #3 lore raw

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

Quoted 8 lines · Steven Rostedt

Quoted 4 lines · On Tue, 2 Jun 2015 16:26:28 +0300 Gil Fruchter <[email protected]> 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(),

Quoted 8 lines

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

Steven Rostedt

maintainer

2015-06-08 15:22 UTC #4 lore raw

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

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

Quoted 6 lines · On Wed, 3 Jun 2015 11:53:59 +0000 Gil Fruchter <[email protected]> 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

Gil Fruchter

author

2015-06-02 13:42 UTC #5 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Gil Fruchter <[email protected]>

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

kernel/trace/trace.c +1 -1

index 57ffc4a..8c5b382 100644
@@ -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;

Signature · 1 line

1.7.1

Steven Rostedt

maintainer

2015-06-02 14:26 UTC #6 lore raw

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

Quoted 4 lines · On Tue, 2 Jun 2015 16:26:29 +0300 Gil Fruchter <[email protected]> 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?

Quoted 24 lines

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

Quoted 8 lines

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

Quoted 20 lines

Signed-off-by Noam Camus <[email protected]>

Signed-off-by Gil Fruchter <[email protected]>

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

kernel/trace/trace.c +1 -1

index 57ffc4a..8c5b382 100644
@@ -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;

Gilad Ben-Yossef

2015-06-02 15:47 UTC #7 lore raw

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

Quoted 9 lines · Steven Rostedt

Quoted 4 lines · On Tue, 2 Jun 2015 16:26:29 +0300 Gil Fruchter <[email protected]> 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

Signature · 10 lines

Gilad Ben-Yossef
Chief Coffee Drinker
[email protected]
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

Gil Fruchter

author

2015-06-09 07:33 UTC #8 lore raw

v2 cover Fix tracing with non-continuous cpu ids

In reply to 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(-)

Gil Fruchter

author

2015-06-09 07:34 UTC #9 lore raw

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

In reply to 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 <[email protected]>

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

kernel/trace/trace.c +1 -1

index 0533049..57ffc4a 100644
@@ -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;

Signature · 1 line

1.7.1

Gil Fruchter

author

2015-06-09 07:34 UTC #10 lore raw

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

In reply to Gil Fruchter

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

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 <[email protected]>

Signed-off-by Gil Fruchter <[email protected]>

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

kernel/trace/trace.c +1 -1

index 57ffc4a..8c5b382 100644
@@ -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;

Signature · 1 line

1.7.1

Steven Rostedt

maintainer

2015-07-13 16:12 UTC #11 lore raw

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

Quoted 37 lines · On Tue, 9 Jun 2015 10:32:35 +0300 Gil Fruchter <[email protected]> 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

Quoted 1 line

Signed-off-by Noam Camus <[email protected]>

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

-- Steve

Quoted 18 lines

Signed-off-by Gil Fruchter <[email protected]>

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

kernel/trace/trace.c +1 -1

index 57ffc4a..8c5b382 100644
@@ -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;

Gil Fruchter

author

2015-07-14 10:13 UTC #12 lore raw

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

Quoted 40 lines · Steven Rostedt

Quoted 37 lines · On Tue, 9 Jun 2015 10:32:35 +0300 Gil Fruchter <[email protected]> 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

Quoted 6 lines

Quoted 1 line
Signed-off-by Noam Camus <[email protected]>
Why Noam's SoB tag? Did he author it?
-- Steve

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

Gil.

Steven Rostedt

maintainer

2015-07-14 13:22 UTC #13 lore raw

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

Quoted 8 lines · On Tue, 14 Jul 2015 10:12:47 +0000 Gil Fruchter <[email protected]> wrote:

Quoted 6 lines
Quoted 1 line
Signed-off-by Noam Camus <[email protected]>
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