FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor


 
 
LinkBack Thread Tools
 
Old 09-30-2010, 09:19 AM
Per Fransson
 
Default ARM SMP

Hi Dave,

This patch is an attempt to get the ball rolling on SMP support for ARM.

Regards,
Per


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, &notes_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
 
Old 09-30-2010, 01:12 PM
Mika Westerberg
 
Default ARM SMP

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.

> 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, &notes_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;


You leak notes_ptrs here.

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)

> + }
> +
> +
> + 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;

You leak per_cpu_offsets here.

> + }
> +
> + /* 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;
> -

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?

Anyway, if this this check is not needed anymore, I guess you can remove the
whole variable from machspec structure.

Best regards,
MW

> + 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
 
Old 09-30-2010, 01:16 PM
Dave Anderson
 
Default ARM SMP

----- "Per Fransson" <per.xx.fransson@stericsson.com> wrote:

> 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, &notes_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
 
Old 09-30-2010, 01:41 PM
Dave Anderson
 
Default ARM SMP

----- "Mika Westerberg" <ext-mika.1.westerberg@nokia.com> wrote:

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

> + panic_task_regs = GETBUF(kt->cpus*sizeof(*panic_task_regs));

> - ms->crash_task_regs = &panic_task_regs;
> + ms->crash_task_regs = panic_task_regs;

You'd have to use malloc() for the panic_task_regs assignment.

It looks like the other GETBUF() calls are for temporary buffers.

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
 
Old 10-01-2010, 02:33 PM
Dave Anderson
 
Default ARM SMP

----- "Mika Westerberg" <ext-mika.1.westerberg@nokia.com> wrote:

> 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
 
Old 10-04-2010, 12:43 PM
Per Fransson
 
Default 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

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, &notes_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
 
Old 10-04-2010, 12:55 PM
Dave Anderson
 
Default ARM SMP

----- "Per Fransson" <per.xx.fransson@stericsson.com> wrote:

> 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, &notes_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
 
Old 10-04-2010, 01:02 PM
Per Fransson
 
Default 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
 
Old 10-04-2010, 01:19 PM
Dave Anderson
 
Default ARM SMP

----- "Per Fransson" <per.fransson.ml@gmail.com> wrote:

> 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
 
Old 10-04-2010, 02:08 PM
Per Fransson
 
Default 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
 

Thread Tools




All times are GMT. The time now is 11:58 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org