- /*
- * Do some sanity checks for this note before reading registers from it.
- */
- note = (Elf32_Nhdr *)buf;
- p = buf + sizeof(Elf32_Nhdr);
+ /*
+ * Do some sanity checks for this note before reading registers from it.
+ */
+ note = (Elf32_Nhdr *)buf;
+ p = buf + sizeof(Elf32_Nhdr);
/*
* And finally we have pid and registers for the crashed task. This is
* used later on when dumping backtrace.
*/
ms->crash_task_pid = *(ulong *)(p + OFFSET(elf_prstatus_pr_pid));
- ms->crash_task_regs = &panic_task_regs;
+ ms->crash_task_regs = panic_task_regs;
- if (tt->panic_task != bt->task || bt->tc->pid != ms->crash_task_pid)
- return FALSE;
-
+ if (!is_task_active(bt->task))
+ return FALSE;
+
/*
* We got registers for panic task from crash_notes. Just return them.
*/
- *nip = ms->crash_task_regs->ARM_pc;
- *ksp = ms->crash_task_regs->ARM_sp;
+ *nip = ms->crash_task_regs[bt->tc->processor].ARM_pc;
+ *ksp = ms->crash_task_regs[bt->tc->processor].ARM_sp;
/*
* Also store pointer to all registers in case unwinding code needs
* to access LR.
*/
- bt->machdep = ms->crash_task_regs;
+ bt->machdep = &(ms->crash_task_regs[bt->tc->processor]);
Just cosmetic thing but it seems that you are using 4 space indent (or is our
mailserver mangling that also). It would be better to be consistent with the style
used in that file (that is, tabs)
> Hi Dave,
>
> This patch is an attempt to get the ball rolling on SMP support for ARM.
>
> Regards,
> Per
Aside from a few minor issues when compiling arm.c with "make warn",
this patch looks fine to me, and runs OK on a sample UP vmlinux/vmcore
pair that Mika made available for me.
For a sanity check, though, can I get an ACK from the ARM maintainers
(Mika, Jan, and/or Thomas) prior to merging it?
Also, it would be helpful to me if you could make an ARM SMP vmlinux/vmcore
pair for me to have around as a reference. If you can do it, please make
the dumpfile as small in memory-size as possible, and put it somewhere that
I can download from. (you can send me the details off-list).
Thanks,
Dave
> diff --git a/arm.c b/arm.c
> index 06b2f1c..bf346df 100644
> --- a/arm.c
> +++ b/arm.c
> @@ -73,7 +73,7 @@ struct arm_cpu_context_save {
> /*
> * Holds registers during the crash.
> */
> -static struct arm_pt_regs panic_task_regs;
> +static struct arm_pt_regs *panic_task_regs;
>
> #define PGDIR_SIZE() (4 * PAGESIZE())
> #define PGDIR_OFFSET(X) (((ulong)(X)) & (PGDIR_SIZE() - 1))
> @@ -484,71 +484,103 @@ arm_get_crash_notes(void)
> Elf32_Nhdr *note;
> ulong ptr, offset;
> char *buf, *p;
> + ulong *notes_ptrs;
> + ulong per_cpu_offsets_addr;
> + ulong *per_cpu_offsets;
> + ulong i;
>
> if (!symbol_exists("crash_notes"))
> return FALSE;
>
> crash_notes = symbol_value("crash_notes");
>
> - if (kt->cpus > 1)
> - error(WARNING, "only one CPU is currently supported
");
> + notes_ptrs = GETBUF(kt->cpus*sizeof(notes_ptrs[0]));
>
> /*
> * Read crash_notes for the first CPU. crash_notes are in standard
> ELF
> * note format.
> */
> - if (!readmem(crash_notes, KVADDR, &ptr, sizeof(ptr), "crash_notes",
> + if (!readmem(crash_notes, KVADDR, ¬es_ptrs[kt->cpus-1],
> sizeof(notes_ptrs[kt->cpus-1]), "crash_notes",
> RETURN_ON_ERROR)) {
> - error(WARNING, "cannot read crash_notes
");
> + error(WARNING, "cannot read crash_notes
");
> + return FALSE;
> + }
> +
> +
> + if (symbol_exists("__per_cpu_offset")) {
> +
> + /* Get the __per_cpu_offset array */
> + per_cpu_offsets_addr = symbol_value("__per_cpu_offset");
> +
> + per_cpu_offsets = GETBUF(kt->cpus*sizeof(*per_cpu_offsets));
> +
> + if (!readmem(per_cpu_offsets_addr, KVADDR, per_cpu_offsets,
> kt->cpus*sizeof(*per_cpu_offsets), "per_cpu_offsets",
> + RETURN_ON_ERROR)) {
> + error(WARNING, "cannot read per_cpu_offsets
");
> return FALSE;
> + }
> +
> + /* Add __per_cpu_offset for each cpu to form the pointer to the
> notes */
> + for (i = 0; i<kt->cpus; i++) {
> + notes_ptrs[i] = notes_ptrs[kt->cpus-1] + per_cpu_offsets[i];
> + }
> + FREEBUF(per_cpu_offsets);
> }
>
> buf = GETBUF(SIZE(note_buf));
> + panic_task_regs = GETBUF(kt->cpus*sizeof(*panic_task_regs));
> +
> + for (i=0;i<kt->cpus;i++) {
>
> - if (!readmem(ptr, KVADDR, buf, SIZE(note_buf), "note_buf_t",
> - RETURN_ON_ERROR)) {
> + if (!readmem(notes_ptrs[i], KVADDR, buf, SIZE(note_buf),
> "note_buf_t",
> + RETURN_ON_ERROR)) {
> error(WARNING, "failed to read note_buf_t
");
> goto fail;
> - }
> + }
>
> - /*
> - * Do some sanity checks for this note before reading registers from
> it.
> - */
> - note = (Elf32_Nhdr *)buf;
> - p = buf + sizeof(Elf32_Nhdr);
> + /*
> + * Do some sanity checks for this note before reading registers
> from it.
> + */
> + note = (Elf32_Nhdr *)buf;
> + p = buf + sizeof(Elf32_Nhdr);
>
> - if (note->n_type != NT_PRSTATUS) {
> + if (note->n_type != NT_PRSTATUS) {
> error(WARNING, "invalid note (n_type != NT_PRSTATUS)
");
> goto fail;
> - }
> - if (p[0] != 'C' || p[1] != 'O' || p[2] != 'R' || p[3] != 'E') {
> + }
> + if (p[0] != 'C' || p[1] != 'O' || p[2] != 'R' || p[3] != 'E') {
> error(WARNING, "invalid note (name != "CORE"
");
> goto fail;
> - }
> + }
>
> - /*
> - * Find correct location of note data. This contains elf_prstatus
> - * structure which has registers etc. for the crashed task.
> - */
> - offset = sizeof(Elf32_Nhdr);
> - offset = roundup(offset + note->n_namesz, 4);
> - p = buf + offset; /* start of elf_prstatus */
> + /*
> + * Find correct location of note data. This contains
> elf_prstatus
> + * structure which has registers etc. for the crashed task.
> + */
> + offset = sizeof(Elf32_Nhdr);
> + offset = roundup(offset + note->n_namesz, 4);
> + p = buf + offset; /* start of elf_prstatus */
>
> - BCOPY(p + OFFSET(elf_prstatus_pr_reg), &panic_task_regs,
> - sizeof(panic_task_regs));
> + BCOPY(p + OFFSET(elf_prstatus_pr_reg), &panic_task_regs[i],
> + sizeof(panic_task_regs[i]));
> +
> + }
>
> /*
> * And finally we have pid and registers for the crashed task. This
> is
> * used later on when dumping backtrace.
> */
> ms->crash_task_pid = *(ulong *)(p + OFFSET(elf_prstatus_pr_pid));
> - ms->crash_task_regs = &panic_task_regs;
> + ms->crash_task_regs = panic_task_regs;
>
> FREEBUF(buf);
> + FREEBUF(notes_ptrs);
> return TRUE;
>
> fail:
> FREEBUF(buf);
> + FREEBUF(notes_ptrs);
> + FREEBUF(panic_task_regs);
> return FALSE;
> }
>
> @@ -996,20 +1028,20 @@ arm_get_dumpfile_stack_frame(struct bt_info
> *bt, ulong *nip, ulong *ksp)
> if (!ms->crash_task_regs)
> return FALSE;
>
> - if (tt->panic_task != bt->task || bt->tc->pid !=
> ms->crash_task_pid)
> - return FALSE;
> -
> + if (!is_task_active(bt->task))
> + return FALSE;
> +
> /*
> * We got registers for panic task from crash_notes. Just return
> them.
> */
> - *nip = ms->crash_task_regs->ARM_pc;
> - *ksp = ms->crash_task_regs->ARM_sp;
> + *nip = ms->crash_task_regs[bt->tc->processor].ARM_pc;
> + *ksp = ms->crash_task_regs[bt->tc->processor].ARM_sp;
>
> /*
> * Also store pointer to all registers in case unwinding code needs
> * to access LR.
> */
> - bt->machdep = ms->crash_task_regs;
> + bt->machdep = &(ms->crash_task_regs[bt->tc->processor]);
>
> return TRUE;
> }
> diff --git a/defs.h b/defs.h
> index d431d6e..8a2291a 100755
> --- a/defs.h
> +++ b/defs.h
> @@ -85,7 +85,7 @@
> #define NR_CPUS (64)
> #endif
> #ifdef ARM
> -#define NR_CPUS (1)
> +#define NR_CPUS (4)
> #endif
>
> #define BUFSIZE (1500)
>
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
> Hi Per,
>
> On Thu, Sep 30, 2010 at 11:19:28AM +0200, ext Per Fransson wrote:
> >
> > This patch is an attempt to get the ball rolling on SMP support for ARM.
>
> I noticed that this patch is line-wrapped so it doesn't apply cleanly (or is it
> our brilliant exchange server which mangled that).
No, I ran into the same problem...
Also, I now note that this piece below is incorrect. GETBUF() buffers are
transitory, because they all get freed as soon as free_all_bufs() is called
next: (such as in restore_sanity() prior to each command)
> Hi Per,
>
> On Thu, Sep 30, 2010 at 11:19:28AM +0200, ext Per Fransson wrote:
> >
> > This patch is an attempt to get the ball rolling on SMP support for ARM.
>
> I noticed that this patch is line-wrapped so it doesn't apply cleanly (or is it
> our brilliant exchange server which mangled that).
>
> Few questions, see below.
... [ cut ] ...
>
> > @@ -996,20 +1028,20 @@ arm_get_dumpfile_stack_frame(struct bt_info *bt, ulong *nip, ulong *ksp)
> > if (!ms->crash_task_regs)
> > return FALSE;
> >
> > - if (tt->panic_task != bt->task || bt->tc->pid != ms->crash_task_pid)
> > - return FALSE;
> > -
>
> Was there a reason to remove the check above? Is it so that when we have SMP
> machine, there is still only one panic'ing task?
Yes, there is still only one panic task.
> Anyway, if this this check is not needed anymore, I guess you can remove the
> whole variable from machspec structure.
Right -- in fact, in the patched arm_get_crash_notes() function, ms->crash_task_pid
gets set to whatever pid was running on the highest cpu -- whether it was the panic
task or not. But like you say, with the patch, it's become irrelevant anyway.
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
10-04-2010, 12:43 PM
Per Fransson
ARM SMP
Hi Dave and Mika,
Thanks for your input. Here's attempt number two. I have:
- eliminated the leaks
- removed 'crash_task_pid'
- fixed the formatting
- not used gmail, since it corrupts the patch
- used malloc/free for panic_task_regs
/*
- * And finally we have pid and registers for the crashed task. This is
+ * And finally we have the registers for the crashed task. This is
* used later on when dumping backtrace.
*/
- ms->crash_task_pid = *(ulong *)(p + OFFSET(elf_prstatus_pr_pid));
- ms->crash_task_regs = &panic_task_regs;
+ ms->crash_task_regs = panic_task_regs;
- if (tt->panic_task != bt->task || bt->tc->pid != ms->crash_task_pid)
+ if (!is_task_active(bt->task))
return FALSE;
-
+
/*
* We got registers for panic task from crash_notes. Just return them.
*/
- *nip = ms->crash_task_regs->ARM_pc;
- *ksp = ms->crash_task_regs->ARM_sp;
+ *nip = ms->crash_task_regs[bt->tc->processor].ARM_pc;
+ *ksp = ms->crash_task_regs[bt->tc->processor].ARM_sp;
/*
* Also store pointer to all registers in case unwinding code needs
* to access LR.
*/
- bt->machdep = ms->crash_task_regs;
+ bt->machdep = &(ms->crash_task_regs[bt->tc->processor]);
> Hi Dave and Mika,
>
> Thanks for your input. Here's attempt number two. I have:
>
> - eliminated the leaks
> - removed 'crash_task_pid'
> - fixed the formatting
> - not used gmail, since it corrupts the patch
> - used malloc/free for panic_task_regs
Thanks Per, I'll give it a test run.
No need to repost, but in the future can you also compile it
with "make warn" to clean it up a bit?
# make warn
...
cc -c -g -DARM -m32 -D_FILE_OFFSET_BITS=64 arm.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wp,-D_FORTIFY_SOURCE=2
arm.c: In function ‘arm_get_crash_notes’:
arm.c:496: warning: assignment from incompatible pointer type
arm.c:515: warning: assignment from incompatible pointer type
arm.c:484: warning: unused variable ‘ptr’
...
Thanks,
Dave
>
> Regards,
> Per
>
>
> diff --git a/arm.c b/arm.c
> index 06b2f1c..b3841c0 100644
> --- a/arm.c
> +++ b/arm.c
> @@ -73,7 +73,7 @@ struct arm_cpu_context_save {
> /*
> * Holds registers during the crash.
> */
> -static struct arm_pt_regs panic_task_regs;
> +static struct arm_pt_regs *panic_task_regs;
>
> #define PGDIR_SIZE() (4 * PAGESIZE())
> #define PGDIR_OFFSET(X) (((ulong)(X)) & (PGDIR_SIZE() - 1))
> @@ -392,7 +392,6 @@ arm_dump_machdep_table(ulong arg)
> fprintf(fp, " kernel_text_end: %lx
", ms->kernel_text_end);
> fprintf(fp, "exception_text_start: %lx
",
> ms->exception_text_start);
> fprintf(fp, " exception_text_end: %lx
", ms->exception_text_end);
> - fprintf(fp, " crash_task_pid: %ld
", ms->crash_task_pid);
> fprintf(fp, " crash_task_regs: %lx
",
> (ulong)ms->crash_task_regs);
> }
>
> @@ -484,71 +483,104 @@ arm_get_crash_notes(void)
> Elf32_Nhdr *note;
> ulong ptr, offset;
> char *buf, *p;
> + ulong *notes_ptrs;
> + ulong per_cpu_offsets_addr;
> + ulong *per_cpu_offsets;
> + ulong i;
>
> if (!symbol_exists("crash_notes"))
> return FALSE;
>
> crash_notes = symbol_value("crash_notes");
>
> - if (kt->cpus > 1)
> - error(WARNING, "only one CPU is currently supported
");
> + notes_ptrs = GETBUF(kt->cpus*sizeof(notes_ptrs[0]));
>
> /*
> * Read crash_notes for the first CPU. crash_notes are in standard
> ELF
> * note format.
> */
> - if (!readmem(crash_notes, KVADDR, &ptr, sizeof(ptr), "crash_notes",
> + if (!readmem(crash_notes, KVADDR, ¬es_ptrs[kt->cpus-1],
> sizeof(notes_ptrs[kt->cpus-1]), "crash_notes",
> RETURN_ON_ERROR)) {
> error(WARNING, "cannot read crash_notes
");
> + FREEBUF(notes_ptrs);
> return FALSE;
> }
> +
> +
> + if (symbol_exists("__per_cpu_offset")) {
> +
> + /* Get the __per_cpu_offset array */
> + per_cpu_offsets_addr = symbol_value("__per_cpu_offset");
> +
> + per_cpu_offsets = GETBUF(kt->cpus*sizeof(*per_cpu_offsets));
> +
> + if (!readmem(per_cpu_offsets_addr, KVADDR, per_cpu_offsets,
> kt->cpus*sizeof(*per_cpu_offsets), "per_cpu_offsets",
> + RETURN_ON_ERROR)) {
> + error(WARNING, "cannot read per_cpu_offsets
");
> + FREEBUF(per_cpu_offsets);
> + return FALSE;
> + }
> +
> + /* Add __per_cpu_offset for each cpu to form the pointer to the
> notes */
> + for (i = 0; i<kt->cpus; i++) {
> + notes_ptrs[i] = notes_ptrs[kt->cpus-1] + per_cpu_offsets[i];
> + }
> + FREEBUF(per_cpu_offsets);
> + }
>
> buf = GETBUF(SIZE(note_buf));
> + panic_task_regs = malloc(kt->cpus*sizeof(*panic_task_regs));
> +
> + for (i=0;i<kt->cpus;i++) {
> +
> + if (!readmem(notes_ptrs[i], KVADDR, buf, SIZE(note_buf),
> "note_buf_t",
> + RETURN_ON_ERROR)) {
> + error(WARNING, "failed to read note_buf_t
");
> + goto fail;
> + }
>
> - if (!readmem(ptr, KVADDR, buf, SIZE(note_buf), "note_buf_t",
> - RETURN_ON_ERROR)) {
> - error(WARNING, "failed to read note_buf_t
");
> - goto fail;
> - }
> + /*
> + * Do some sanity checks for this note before reading registers
> from it.
> + */
> + note = (Elf32_Nhdr *)buf;
> + p = buf + sizeof(Elf32_Nhdr);
>
> - /*
> - * Do some sanity checks for this note before reading registers from
> it.
> - */
> - note = (Elf32_Nhdr *)buf;
> - p = buf + sizeof(Elf32_Nhdr);
> + if (note->n_type != NT_PRSTATUS) {
> + error(WARNING, "invalid note (n_type != NT_PRSTATUS)
");
> + goto fail;
> + }
> + if (p[0] != 'C' || p[1] != 'O' || p[2] != 'R' || p[3] != 'E') {
> + error(WARNING, "invalid note (name != "CORE"
");
> + goto fail;
> + }
>
> - if (note->n_type != NT_PRSTATUS) {
> - error(WARNING, "invalid note (n_type != NT_PRSTATUS)
");
> - goto fail;
> - }
> - if (p[0] != 'C' || p[1] != 'O' || p[2] != 'R' || p[3] != 'E') {
> - error(WARNING, "invalid note (name != "CORE"
");
> - goto fail;
> - }
> + /*
> + * Find correct location of note data. This contains elf_prstatus
> + * structure which has registers etc. for the crashed task.
> + */
> + offset = sizeof(Elf32_Nhdr);
> + offset = roundup(offset + note->n_namesz, 4);
> + p = buf + offset; /* start of elf_prstatus */
>
> - /*
> - * Find correct location of note data. This contains elf_prstatus
> - * structure which has registers etc. for the crashed task.
> - */
> - offset = sizeof(Elf32_Nhdr);
> - offset = roundup(offset + note->n_namesz, 4);
> - p = buf + offset; /* start of elf_prstatus */
> + BCOPY(p + OFFSET(elf_prstatus_pr_reg), &panic_task_regs[i],
> + sizeof(panic_task_regs[i]));
>
> - BCOPY(p + OFFSET(elf_prstatus_pr_reg), &panic_task_regs,
> - sizeof(panic_task_regs));
> + }
>
> /*
> - * And finally we have pid and registers for the crashed task. This
> is
> + * And finally we have the registers for the crashed task. This is
> * used later on when dumping backtrace.
> */
> - ms->crash_task_pid = *(ulong *)(p + OFFSET(elf_prstatus_pr_pid));
> - ms->crash_task_regs = &panic_task_regs;
> + ms->crash_task_regs = panic_task_regs;
>
> FREEBUF(buf);
> + FREEBUF(notes_ptrs);
> return TRUE;
>
> fail:
> FREEBUF(buf);
> + FREEBUF(notes_ptrs);
> + free(panic_task_regs);
> return FALSE;
> }
>
> @@ -996,20 +1028,20 @@ arm_get_dumpfile_stack_frame(struct bt_info
> *bt, ulong *nip, ulong *ksp)
> if (!ms->crash_task_regs)
> return FALSE;
>
> - if (tt->panic_task != bt->task || bt->tc->pid !=
> ms->crash_task_pid)
> + if (!is_task_active(bt->task))
> return FALSE;
> -
> +
> /*
> * We got registers for panic task from crash_notes. Just return
> them.
> */
> - *nip = ms->crash_task_regs->ARM_pc;
> - *ksp = ms->crash_task_regs->ARM_sp;
> + *nip = ms->crash_task_regs[bt->tc->processor].ARM_pc;
> + *ksp = ms->crash_task_regs[bt->tc->processor].ARM_sp;
>
> /*
> * Also store pointer to all registers in case unwinding code needs
> * to access LR.
> */
> - bt->machdep = ms->crash_task_regs;
> + bt->machdep = &(ms->crash_task_regs[bt->tc->processor]);
>
> return TRUE;
> }
> diff --git a/defs.h b/defs.h
> index d431d6e..6e0c8cc 100755
> --- a/defs.h
> +++ b/defs.h
> @@ -85,7 +85,7 @@
> #define NR_CPUS (64)
> #endif
> #ifdef ARM
> -#define NR_CPUS (1)
> +#define NR_CPUS (4)
> #endif
>
> #define BUFSIZE (1500)
> @@ -4062,7 +4062,6 @@ struct machine_specific {
> ulong kernel_text_end;
> ulong exception_text_start;
> ulong exception_text_end;
> - ulong crash_task_pid;
> struct arm_pt_regs *crash_task_regs;
> };
>
>
> --
> Crash-utility mailing list
> Crash-utility@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
10-04-2010, 01:02 PM
Per Fransson
ARM SMP
On Mon, Oct 4, 2010 at 2:55 PM, Dave Anderson <anderson@redhat.com> wrote:
>
> No need to repost, but in the future can you also compile it
> with "make warn" to clean it up a bit?
>
Oh right, sorry. I'll keep that in mind.
/Per
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
> On Mon, Oct 4, 2010 at 2:55 PM, Dave Anderson <anderson@redhat.com>
> wrote:
> >
> > No need to repost, but in the future can you also compile it
> > with "make warn" to clean it up a bit?
> >
>
> Oh right, sorry. I'll keep that in mind.
>
> /Per
OK, it looks and works fine for me with my sample UP vmlinux/vmcore.
With a cleanup for the warnings, and a fatal error in the very unlikely
case of a malloc() failure -- and as long as Mika doesn't NAK it -- consider
it queued for the next release.
BTW, will you be able to make and create a sample (small) SMP vmlinux/vmcore
pair for me to download that I can keep on hand for future testing?
Thanks,
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
10-04-2010, 02:08 PM
Per Fransson
ARM SMP
On Mon, Oct 4, 2010 at 3:19 PM, Dave Anderson <anderson@redhat.com> wrote:
> BTW, will you be able to make and create a sample (small) SMP vmlinux/vmcore
> pair for me to download that I can keep on hand for future testing?
>
I hope to, but I'll have to get back to you on that.
/Per
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
ARM SMP
