[PATCH v3 0/2] Crash-Utility: crash enhancement to support s390x diskdump
by Mahesh J Salgaonkar
Hi All,
There was problem in my earlier posting. I had miss-spelled my id in the
From: field. Hence resending it again.
Please find the version 3 of crash patchset that enables crash utility to read
s390x diskdump (DUMPFILE generated by makedumpfile).
The first patch enhances crash utility to read elf notes section from
DUMPFILE if Kdump header >= 4.
The second patch adds crash support to read s390x diskdump. This patch also
implements new arch dependent interface process_elf_notes() to read CPU
register infromation stored in ELF notes section.
Change v3:
- Re-based to crash-5.0.9
Change v2:
- Re-based to crash-5.0.8
- Modified __diskdump_memory_dump() function to display contents of
all 4 additional fields in the kdump_sub_header.
- Modified __diskdump_memory_dump() to dump the vmcoreinfo ASCII strings.
Please review the changes and send in your comments.
Thanks,
-Mahesh.
--
Signed-off-by: Mahesh Salgaonkar <mahesh(a)linux.vnet.ibm.com>
15 years
[PATCH v3 2/2] Crash-Utility: Add support for s390x diskdump
by Mahesh J Salgaonkar
This patch adds support for reading diskdumps (generated by makedumpfile) for
s390x architecture.
Signed-off-by: Mahesh Salgaonkar <mahesh(a)linux.vnet.ibm.com>
Signed-off-by: Michael Holzheu <holzheu(a)de.ibm.com>
---
diskdump.c | 12 ++++++++++++
s390x.c | 22 ++++++++++++++++++++++
2 files changed, 34 insertions(+)
Index: crash-5.0.9/diskdump.c
===================================================================
--- crash-5.0.9.orig/diskdump.c
+++ crash-5.0.9/diskdump.c
@@ -253,6 +253,9 @@ restart:
else if (STRNEQ(header->utsname.machine, "arm") &&
machine_type_mismatch(file, "ARM", NULL, 0))
goto err;
+ else if (STRNEQ(header->utsname.machine, "s390x") &&
+ machine_type_mismatch(file, "S390X", NULL, 0))
+ goto err;
if (header->block_size != block_size) {
block_size = header->block_size;
@@ -349,6 +352,10 @@ restart:
dd->machine_type = EM_IA_64;
else if (machine_type("PPC64"))
dd->machine_type = EM_PPC64;
+ else if (machine_type("S390"))
+ dd->machine_type = EM_S390;
+ else if (machine_type("S390X"))
+ dd->machine_type = EM_S390;
else {
error(INFO, "%s: unsupported machine type: %s\n",
DISKDUMP_VALID() ? "diskdump" : "compressed kdump",
@@ -778,6 +785,9 @@ get_diskdump_regs(struct bt_info *bt, ul
case EM_X86_64:
return get_netdump_regs_x86_64(bt, eip, esp);
break;
+ case EM_S390:
+ return machdep->get_stack_frame(bt, eip, esp);
+ break;
default:
error(FATAL, "%s: unsupported machine type: %s\n",
@@ -893,6 +903,8 @@ __diskdump_memory_dump(FILE *fp)
fprintf(fp, "(EM_IA_64)\n"); break;
case EM_PPC64:
fprintf(fp, "(EM_PPC64)\n"); break;
+ case EM_S390:
+ fprintf(fp, "(EM_S390)\n"); break;
default:
fprintf(fp, "(unknown)\n"); break;
}
Index: crash-5.0.9/s390x.c
===================================================================
--- crash-5.0.9.orig/s390x.c
+++ crash-5.0.9/s390x.c
@@ -272,6 +272,26 @@ static void s390x_elf_note_add(int elf_c
}
}
+static void s390x_process_elf_notes(void *note_ptr, unsigned long size_note)
+{
+ Elf64_Nhdr *note = NULL;
+ size_t tot, len;
+ static int num_prstatus_notes = 0;
+
+ for (tot = 0; tot < size_note; tot += len) {
+ note = note_ptr + tot;
+
+ if (note->n_type == NT_PRSTATUS)
+ num_prstatus_notes++;
+
+ machdep->dumpfile_init(num_prstatus_notes, note);
+
+ len = sizeof(Elf64_Nhdr);
+ len = roundup(len + note->n_namesz, 4);
+ len = roundup(len + note->n_descsz, 4);
+ }
+}
+
/*
* Do all necessary machine-specific setup here. This is called several
* times during initialization.
@@ -283,6 +303,7 @@ s390x_init(int when)
{
case SETUP_ENV:
machdep->dumpfile_init = s390x_elf_note_add;
+ machdep->process_elf_notes = s390x_process_elf_notes;
break;
case PRE_SYMTAB:
machdep->verify_symbol = s390x_verify_symbol;
@@ -399,6 +420,7 @@ s390x_dump_machdep_table(ulong arg)
fprintf(fp, " init_kernel_pgd: NULL\n");
fprintf(fp, " value_to_symbol: generic_machdep_value_to_symbol()\n");
fprintf(fp, " dumpfile_init: s390x_elf_note_add()\n");
+ fprintf(fp, " process_elf_notes: s390x_process_elf_notes()\n");
fprintf(fp, " line_number_hooks: s390x_line_number_hooks\n");
fprintf(fp, " last_pgd_read: %lx\n", machdep->last_pgd_read);
fprintf(fp, " last_pmd_read: %lx\n", machdep->last_pmd_read);
15 years
[PATCH v3 1/2] Crash-Utility: Enhance crash utilty to read elf notes content from DUMPFILE
by Mahesh J Salgaonkar
This patch enables crash utility to handle elf notes content in DUMPFILE if
kdump hedaer version >= 4. The processing of data content will be architecture
dependent.
makedumpfile tool is enhanced to store elf notes content from vmcore to
DUMPFILE starting from kdump header version 4.
---------------------------------------------------------------
commit 1fb344e934f87b9c124e487a21e111c7bdc977ba
Author: Ken'ichi Ohmichi <oomichi(a)mxs.nes.nec.co.jp>
Date: Fri Oct 8 09:18:59 2010 +0900
[PATCH] Add ELF note section to the kdump-compressed format.
---------------------------------------------------------------
A new arch dependent interface has been added to machdep_table structure which
needs to be populated during machdep_init(SETUP_ENV) call.
struct machdep_table {
...
...
void (*process_elf_notes)(void *, unsigned long);
}
ChangeLog:
- Modified __diskdump_memory_dump() function to display contents of
all 4 additional fields in the kdump_sub_header.
- Modified __diskdump_memory_dump() to dump the vmcoreinfo ASCII strings.
Signed-off-by: Mahesh Salgaonkar <mahesh(a)linux.vnet.ibm.com>
---
defs.h | 1
diskdump.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
diskdump.h | 4 +++
3 files changed, 86 insertions(+)
Index: crash-5.0.9/diskdump.c
===================================================================
--- crash-5.0.9.orig/diskdump.c
+++ crash-5.0.9/diskdump.c
@@ -189,6 +189,7 @@ static int read_dump_header(char *file)
struct disk_dump_header *header = NULL;
struct disk_dump_sub_header *sub_header = NULL;
struct kdump_sub_header *sub_header_kdump = NULL;
+ unsigned char *notes_buf = NULL;
int bitmap_len;
int block_size = (int)sysconf(_SC_PAGESIZE);
off_t offset;
@@ -355,6 +356,31 @@ restart:
goto err;
}
+ /* process elf notes data */
+ if (KDUMP_CMPRS_VALID() && (dd->header->header_version >= 4) &&
+ (sub_header_kdump->offset_note) &&
+ (sub_header_kdump->size_note) && (machdep->process_elf_notes)) {
+ unsigned long size_note = sub_header_kdump->size_note;
+ offset = sub_header_kdump->offset_note;
+
+ if (lseek(dd->dfd, offset, SEEK_SET) == failed) {
+ error(INFO, "compressed kdump: cannot lseek dump elf"
+ " notes\n");
+ goto err;
+ }
+
+ if ((notes_buf = malloc(size_note)) == NULL)
+ error(FATAL, "compressed kdump: cannot malloc notes"
+ " buffer\n");
+
+ if (read(dd->dfd, notes_buf, size_note) < size_note) {
+ error(INFO, "compressed kdump: cannot read notes data"
+ "\n");
+ goto err;
+ }
+ machdep->process_elf_notes(notes_buf, size_note);
+ }
+
/* For split dumpfile */
if (KDUMP_CMPRS_VALID()) {
is_split = ((dd->header->header_version >= 2) &&
@@ -399,6 +425,8 @@ err:
free(sub_header);
if (sub_header_kdump)
free(sub_header_kdump);
+ if (notes_buf)
+ free(notes_buf);
if (dd->bitmap)
free(dd->bitmap);
if (dd->dumpable_bitmap)
@@ -786,6 +814,43 @@ int diskdump_memory_used(void)
return 0;
}
+void dump_vmcoreinfo(FILE *fp)
+{
+ char *buf = NULL;
+ unsigned long i = 0;
+ unsigned long size_vmcoreinfo = dd->sub_header_kdump->size_vmcoreinfo;
+ off_t offset = dd->sub_header_kdump->offset_vmcoreinfo;
+ const off_t failed = (off_t)-1;
+
+ if (lseek(dd->dfd, offset, SEEK_SET) == failed) {
+ error(INFO, "compressed kdump: cannot lseek dump vmcoreinfo\n");
+ return;
+ }
+
+ if ((buf = malloc(size_vmcoreinfo)) == NULL) {
+ error(FATAL, "compressed kdump: cannot malloc vmcoreinfo"
+ " buffer\n");
+ }
+
+ if (read(dd->dfd, buf, size_vmcoreinfo) < size_vmcoreinfo) {
+ error(INFO, "compressed kdump: cannot read vmcoreinfo data\n");
+ goto err;
+ }
+
+ fprintf(fp, " ");
+ for (i = 0; i < size_vmcoreinfo; i++) {
+ fprintf(fp, "%c", buf[i]);
+ if (buf[i] == '\n')
+ fprintf(fp, " ");
+ }
+ if (buf[i - 1] != '\n')
+ fprintf(fp, "\n");
+err:
+ if (buf)
+ free(buf);
+ return;
+}
+
/*
* This function is dump-type independent, and could be used
* to dump the diskdump_data structure contents and perhaps
@@ -954,6 +1019,22 @@ __diskdump_memory_dump(FILE *fp)
fprintf(fp, " start_pfn: %lu\n", dd->sub_header_kdump->start_pfn);
fprintf(fp, " end_pfn: %lu\n", dd->sub_header_kdump->end_pfn);
}
+ if (dh->header_version >= 3) {
+ fprintf(fp, " offset_vmcoreinfo: %lx\n",
+ dd->sub_header_kdump->offset_vmcoreinfo);
+ fprintf(fp, " size_vmcoreinfo: %lu\n",
+ dd->sub_header_kdump->size_vmcoreinfo);
+ if (dd->sub_header_kdump->offset_vmcoreinfo &&
+ dd->sub_header_kdump->size_vmcoreinfo) {
+ dump_vmcoreinfo(fp);
+ }
+ }
+ if (dh->header_version >= 4) {
+ fprintf(fp, " offset_note: %lx\n",
+ dd->sub_header_kdump->offset_note);
+ fprintf(fp, " size_note: %lu\n",
+ dd->sub_header_kdump->size_note);
+ }
fprintf(fp, "\n");
} else
fprintf(fp, "(n/a)\n\n");
Index: crash-5.0.9/diskdump.h
===================================================================
--- crash-5.0.9.orig/diskdump.h
+++ crash-5.0.9/diskdump.h
@@ -63,6 +63,10 @@ struct kdump_sub_header {
int split; /* header_version 2 and later */
unsigned long start_pfn; /* header_version 2 and later */
unsigned long end_pfn; /* header_version 2 and later */
+ off_t offset_vmcoreinfo;/* header_version 3 and later */
+ unsigned long size_vmcoreinfo; /* header_version 3 and later */
+ off_t offset_note; /* header_version 4 and later */
+ unsigned long size_note; /* header_version 4 and later */
};
/* page flags */
Index: crash-5.0.9/defs.h
===================================================================
--- crash-5.0.9.orig/defs.h
+++ crash-5.0.9/defs.h
@@ -807,6 +807,7 @@ struct machdep_table {
int (*xen_kdump_p2m_create)(struct xen_kdump_data *);
int (*in_alternate_stack)(int, ulong);
void (*dumpfile_init)(int, void *);
+ void (*process_elf_notes)(void *, unsigned long);
};
/*
15 years
Re: [Crash-utility] [PATCH 0/4] To support module percpu symbol
by Dave Anderson
----- "Toshikazu Nakayama" <nakayama.ts(a)ncos.nec.co.jp> wrote:
> Hello Dave,
>
> >Hello Toshi,
> >
> >I made several changes to your last patch as we discussed before,
> >and successfully tested it on the x86, x86_64, ia64, s390 and s390x
> >architectures.
> >
> >I've attached the patch as it is queued for the next release.
> >
> >Because I was working against my own updated tree, the patch also
> >contains a couple other symbol-handling related changes that are
> >also queued for the next release.
> >
> >Thanks for getting the ball rolling for this feature,
> > Dave
>
> Because your patch could not apply to crash-5.0.9 with patch command,
> I manually apply them and they can run well over my environs.
Hmmm -- works for me:
# tar xzf crash-5.0.9.tar.gz
# cd crash-5.0.9
# patch -p1 < $HOME/symbols.patch
patching file help.c
patching file kernel.c
patching file alpha.c
patching file ppc.c
patching file s390.c
patching file s390x.c
patching file ppc64.c
patching file x86_64.c
patching file symbols.c
patching file lkcd_x86_trace.c
patching file unwind.c
patching file defs.h
#
Maybe you were trying to apply them on top of your last patch?
Anyway, as long as you're happy with the end result...
> I'll use crash-5.0.9 with your patch
> until crash-5.0.10 got available.
>
> I saw that you've updated whole places where per-cpu or MODULE_SYMBOL
> should be applicable in crash utility.
>
Thanks again Toshi,
Dave
15 years
Re: [Crash-utility] [PATCH 0/4] To support module percpu symbol
by Dave Anderson
----- "Dave Anderson" <anderson(a)redhat.com> wrote:
> ----- "Toshikazu Nakayama" <nakayama.ts(a)ncos.nec.co.jp> wrote:
> >
> > I'll fix remained problems and send next patch with updated portion only.
> > I would like to follow your advisement.
>
> OK, I'll take a look.
>
> Thanks,
> Dave
Hello Toshi,
I made several changes to your last patch as we discussed before,
and successfully tested it on the x86, x86_64, ia64, s390 and s390x
architectures.
I've attached the patch as it is queued for the next release.
Because I was working against my own updated tree, the patch also
contains a couple other symbol-handling related changes that are
also queued for the next release.
Thanks for getting the ball rolling for this feature,
Dave
15 years
Re: [Crash-utility] [PATCH 0/4] To support module percpu symbol
by Dave Anderson
----- "Toshikazu Nakayama" <nakayama.ts(a)ncos.nec.co.jp> wrote:
> > To fix that, perhaps the syment structures for all module symbols could
> > have an indication that they are module-only symbols? There are a couple
> > of unused fields in the structure -- perhaps "pad1" could be changed to be
> > a "flags" field that could have a "MODULE_SYMBOL" bit set in it:
> >
> > struct syment {
> > ulong value;
> > char *name;
> > struct syment *val_hash_next;
> > struct syment *name_hash_next;
> > char type;
> > unsigned char cnt;
> > unsigned char pad1;
> > unsigned char pad2;
> > };
> >
> > If that were true, then get_mod_percpu_sym_owner() could reject
> > any syment that doesn't have the MODULE_SYMBOL flag set, and then
> > there should be no problem with "symbol overlap". What do you
> > think about doing something like that?
>
> Sounds fine.
> pad1 is already used by patch_kernel_symbol().
> I will rename pad2.
Yes, but pad1 is only (temporarily) used by gdb_session_init() when a System.map
file is entered on the crash command line -- and long before module_init() is called.
And given that it's a boolean value in patch_kernel_symbol(), I'll change "pad1" to
be "flags", and have two flags defined for use there. Then "pad2" can be left alone
for possible future use.
> >Also, I don't quite understand why it was necessary in your patch to
> >modify cmd_p() like this:
> >
> > --- a/symbols.c
> > +++ b/symbols.c
> > @@ -5636,7 +5636,10 @@ cmd_p(void)
> > leader = strlen(buf2);
> > if (module_symbol(sp->value, NULL, NULL, NULL, *gdb_output_radix))
> > do_load_module_filter = TRUE;
> > - } else if (st->flags & LOAD_MODULE_SYMS)
> > + } else if ((percpu_sp = per_cpu_symbol_search(args[optind])) &&
> > + display_per_cpu_info(percpu_sp))
> > + return;
> > + else if (st->flags & LOAD_MODULE_SYMS)
> > do_load_module_filter = TRUE;
> >
> > if (leader || do_load_module_filter)
> >
> > If the symbol could not be found by symbol_search() in line 5669,
> > then how could it be found later by the second per_cpu_symbol_search()
> > added above?
>
> Because per_cpu_symbol_search() can search percpu symbol with
> per_cpu__ prefix which is automatically renamed.
>
> sprintf(old, "per_cpu__%s", symbol);
> if ((sp = symbol_search(old)))
> return sp;
>
> This retry is valid for old kernel percpu symbol naming model.
>
> DEFINE_PER_CPU(struct kernel_stat, kstat);
>
> We try to enter "p per_cpu__kstat" in old kernel or "p kstat" in new kernel
> and further, enter "struct kernel_stat <virtual address>".
> Someone who does not know real DEFINE_PER_CPU(X) declared symbol name
> force to try "sym X -> possible alternatives" or check kernel code,
> System.map.
>
> I thought that per_cpu_symbol_search() aims to resolve UI gap
> between tow kernel percpu model from above implementation
> but present cmd_p() is not worked well because of symbol_search() failed.
> ("p kstat" works well in the old kernel with this patch)
That is true -- but my original intent was that when per_cpu_symbol_search()
gets passed a hard-coded symbol, then it would make the switch if necessary.
That way all of the currently-existing callers wouldn't have to call
symbol_search() twice, i.e. with and without the "per_cpu__" prefix.
However, cmd_p() is the only caller of the function that passes an unknown
string pointer as an argument, and I presumed that the user would only use
existing, *actual*, symbol names.
However, re-thinking the matter, I guess there should be no problem with
letting the user of the "p" command to "cheat" by leaving off, or adding,
the "per_cpu__" symbol prefix. As it is now, the command would just fail,
but with your patch, at least it would "try" to do the right thing.
So that change looks OK to me.
> > And for for that matter, in the very few modules that have percpu sections
> > in my test machines/dumpfiles, I don't see any actual per-cpu symbols listed
> > by "sym -[mM]". How do you actually "see" the name of a module's percpu symbol?
>
> Truth, I've also been touched first one when
> I tried to make KVM command (based on linux-2.6.35 & CONFIG_KVM=m)
> over private extension module.
> So I actually "see" the name of KVM modules.
> Old kernel dose not have such a module, so I made module for test.
>
> Although extension module want to obtain KVM percpu symbol addresses
> via crash symbol API, it can not answer.
>
> My work is blocked by module's per-cpu (CONFIG_KVM=y is not prefer solution).
> I send patch to crash utility with this background issue.
My mistake -- I can see them, although they show up out of order in the
"sym -[mM] listing. For example:
crash> sym -m ip_conntrack
ffffffff8041f540 (D) per_cpu__ip_conntrack_ecache
ffffffff8041f560 (D) per_cpu__ip_conntrack_stat
ffffffff88d9e000 MODULE START: ip_conntrack
ffffffff88d9e000 (t) kill_proto
ffffffff88d9e00f (t) ct_get_next
ffffffff88d9e052 (t) ct_seq_next
ffffffff88d9e057 (t) exp_seq_next
ffffffff88d9e06b (t) ct_cpu_seq_stop
...
I'll look into fixing that...
> >Not really. But I don't see a problem with the way that you did it -- which
> >seems to work just fine. The only thing that needs to be changed is where
> >add_symbol_file_percpu() calls RESIZEBUF() -- it needs to have "req" passed
> >as an argument instead of "req->buf", like this:
>
> Oh, entire bug!
>
> I'll fix remained problems and send next patch with updated portion only.
> I would like to follow your advisement.
OK, I'll take a look.
Thanks,
Dave
15 years, 1 month
Re: [Crash-utility] [PATCH 0/4] To support module percpu symbol
by Dave Anderson
----- "Toshikazu Nakayama" <nakayama.ts(a)ncos.nec.co.jp> wrote:
> Hi Dave,
>
> ...
> > And I haven't even bothered to look into how it would affect
> > the operation of all of the other architectures...
> >
> > Perhaps the patch can be re-done so that the code can handle it on a
> > per-module basis without inadvertently affecting everything else?
> >
> > In any case, there's no way I can accept the patch as it is
> > currently designed.
>
> I handled module symbol in kernel scope (target is not System.map).
>
> In kernel, percpu data can't access directly with mapped virtual address.
> Related kernel macros are sometimes arch depends or looks compiler depend.
> (I can not understand enough...)
> Since percpu data area is entirely dynamic allocation,
> additionally some arch locate part of them at VMALLOC area.
> They are not straight-mapped so some arch are probably overlapped.
Right -- and because of that overlap, I still believe that it is possible
that cmd_p() may still (inadvertently) do the wrong thing -- in the same
way that the "p in_suspend" attempt failed using your second patch.
In other words, the code path taken for a symbol that is just beyond the
kernel's " __per_cpu_end" would be found OK by the symbol_search() call
in line 5669 -- but it would also be misconstrued as a module percpu
symbol by per_cpu_symbol_search() in line 5670:
5669 if ((sp = symbol_search(args[optind])) && !args[optind+1]) {
5670 if ((percpu_sp = per_cpu_symbol_search(args[optind])) &&
5671 display_per_cpu_info(percpu_sp))
5672 return;
5673 sprintf(buf2, "%s = ", args[optind]);
5674 leader = strlen(buf2);
5675 if (module_symbol(sp->value, NULL, NULL, NULL, *gdb_output_radix))
5676 do_load_module_filter = TRUE;
5677 } else if ((percpu_sp = per_cpu_symbol_search(args[optind])) &&
5678 display_per_cpu_info(percpu_sp))
5679 return;
5680 else if (st->flags & LOAD_MODULE_SYMS)
5681 do_load_module_filter = TRUE;
because your IN_MODULE_PCPU() macro would only check whether the symbol value was
located in a module percpu offset range.
To fix that, perhaps the syment structures for all module symbols could
have an indication that they are module-only symbols? There are a couple
of unused fields in the structure -- perhaps "pad1" could be changed to be
a "flags" field that could have a "MODULE_SYMBOL" bit set in it:
struct syment {
ulong value;
char *name;
struct syment *val_hash_next;
struct syment *name_hash_next;
char type;
unsigned char cnt;
unsigned char pad1;
unsigned char pad2;
};
If that were true, then get_mod_percpu_sym_owner() could reject
any syment that doesn't have the MODULE_SYMBOL flag set, and then
there should be no problem with "symbol overlap". What do you
think about doing something like that?
Also, I don't quite understand why it was necessary in your patch to
modify cmd_p() like this:
--- a/symbols.c
+++ b/symbols.c
@@ -5636,7 +5636,10 @@ cmd_p(void)
leader = strlen(buf2);
if (module_symbol(sp->value, NULL, NULL, NULL, *gdb_output_radix))
do_load_module_filter = TRUE;
- } else if (st->flags & LOAD_MODULE_SYMS)
+ } else if ((percpu_sp = per_cpu_symbol_search(args[optind])) &&
+ display_per_cpu_info(percpu_sp))
+ return;
+ else if (st->flags & LOAD_MODULE_SYMS)
do_load_module_filter = TRUE;
if (leader || do_load_module_filter)
If the symbol could not be found by symbol_search() in line 5669,
then how could it be found later by the second per_cpu_symbol_search()
added above?
And for for that matter, in the very few modules that have percpu sections
in my test machines/dumpfiles, I don't see any actual per-cpu symbols listed
by "sym -[mM]". How do you actually "see" the name of a module's percpu symbol?
> Anyway with previous approach,
> there is no possible solution in my idea at least,
> but I might get really right way from your proposal.
>
> Retake attached patches with per module space.
>
> I use module.percpu which guarantee straight-mapping in every module space.
> Also its size get from bfd_section_size() for legacy percpu implementation
> although latest kernel gives percpu_size.
> - I am sorry but I don't have i386 target environs now, not tested about i386.
> (I think that module.percpu can work well even if it is VMALLOC area.)
>
> But there was still more problem at gdb interface.
>
> When p &(module percpu) is not resolved by syment,
> gdb_pass_through() require pre-registration about module sections.
> The result gave wrong symbol value of module percpu.
>
> I append percpu section in gdb request from add_symbol_file_kallsyms()
> although percpu is not in kallsyms...
> Because multiple GNU_ADD_SYMBOL_FILE requests for one module did not
> work well.
> I am not very well around these implementations.
> Can I resolve it with other better way?
Not really. But I don't see a problem with the way that you did it -- which
seems to work just fine. The only thing that needs to be changed is where
add_symbol_file_percpu() calls RESIZEBUF() -- it needs to have "req" passed
as an argument instead of "req->buf", like this:
static long
add_symbol_file_percpu(struct load_module *lm, struct gnu_request *req, long buflen)
{
char pbuf[BUFSIZE];
int i;
char *secname;
long len;
len = strlen(req->buf);
for (i = 0; i < lm->mod_sections; i++) {
secname = lm->mod_section_data[i].name;
if ((lm->mod_section_data[i].flags & SEC_FOUND) &&
(STREQ(secname, ".data.percpu") ||
STREQ(secname, ".data..percpu"))) {
sprintf(pbuf, " -s %s 0x%lx", secname, lm->mod_percpu);
while ((len + strlen(pbuf)) >= buflen) {
RESIZEBUF(req->buf, buflen, buflen * 2);
buflen *= 2;
}
strcat(req->buf, pbuf);
len += strlen(pbuf);
}
}
return buflen;
}
The way you had it written, if RESIZEBUF() were to be called, it would never
be seen by gdb, because req->buf would still point to the old buffer.
Dave
15 years, 1 month
Re: [Crash-utility] [PATCH 0/4] To support module percpu symbol
by Dave Anderson
----- "Toshikazu Nakayama" <nakayama.ts(a)ncos.nec.co.jp> wrote:
> Hi Dave,
>
> I did some wrong changes and would understand your mentions.
>
> Updating __per_cpu_end itself makes original UI's corruption.
> And for sure, I want __per_cpu_end to be bumped with your guess.
>
> I fixed these bugs like attachments and tested again,
> of course checked st->__per_cpu_end value advised from you.
>
> One more update is to add is_per_cpu_range().
> Because there are some conditions about percpu range in symbols.c,
> I think it is better to use common function than inline.
> The function returns 0 (not percpu), 1 (kernels) and 2 (modules)
> although callers do not distinguish between 1 and 2 now.
Hello Toshi,
Unfortunately there are still problems with this patch design,
both with the "legacy" and "current" kernel types.
I modified your patch to display the new items added to the
structures in defs.h. For example, the new "st->percpu_mod_used"
value should be shown in dump_symbol_table(), along with the
effective (extended) end of percpu space that is used by your
is_per_cpu_range() function:
crash> help -s
...
__per_cpu_start: ffffffff80419000
__per_cpu_end: ffffffff8041f508
percpu_mod_used: 8508 (extended percpu end: ffffffff80427a10)
...
Anyway, as I mentioned in my review of your first patch, support
for the "legacy" module percpu symbols is not acceptable. In
the example above, note that the end of per-cpu space gets extended
from ffffffff8041f508 to ffffffff80427a10. But that value pushes
into legitimate symbol virtual address space above "__per_cpu_end":
crash> sym -l
...
ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
ffffffff8041f500 (d) per_cpu____icmp_socket
ffffffff8041f508 (A) __per_cpu_end <-- original value
ffffffff80420000 (d) .data_nosave
ffffffff80420000 (A) __nosave_begin
ffffffff80420000 (D) in_suspend
ffffffff80421000 (b) .bss
ffffffff80421000 (A) __bss_start
ffffffff80421000 (A) __nosave_end
ffffffff80421000 (B) empty_zero_page
ffffffff80422000 (B) boot_cpu_stack
ffffffff80426000 (B) boot_exception_stacks <-- bumped to ffffffff80427a10
ffffffff8042c000 (B) idt_table
ffffffff8042d000 (B) boot_delay
ffffffff8042d008 (B) printk_delay_msec
...
So, for example, take the "in_suspend" variable above, which is a global
integer variable at address ffffffff80420000:
crash> whatis in_suspend
int in_suspend;
crash>
Without your patch, it gets printed correctly like this:
crash> p in_suspend
in_suspend = $2 = 0
crash>
But with your patch, is_per_cpu_range() thinks that ffffffff80420000 is
a per-cpu symbol value because it is located within the extended range.
So the command incorrectly does this:
crash> p in_suspend
PER-CPU DATA TYPE:
int in_suspend;
PER-CPU ADDRESSES:
[0]: ffff81000157a000
[1]: ffff810001582580
[2]: ffff81000158ab00
[3]: ffff810001593080
[4]: ffff81000159b600
[5]: ffff8100015a3b80
[6]: ffff8100015ac100
[7]: ffff8100015b4680
crash>
I also have my doubts about the current kernels which have the single
"pcpu_reserved_chunk_limit" symbol, where its contents are simply added
to the base kernel's "__per_cpu_end" value. It seemingly would work OK
with x86_64, because the symbols are very low offset values, and cannot
be misconstrued for regular symbol address values:
crash> sym -l
0 (D) __per_cpu_start
0 (V) irq_stack_union
4000 (V) gdt_page
5000 (V) exception_stacks
b000 (V) tlb_vector_offset
...
15740 (V) call_single_queue
15780 (V) csd_data
157c0 (V) softnet_data
15900 (D) __per_cpu_end
ffffffff81000000 (T) _text
ffffffff81000000 (T) startup_64
ffffffff810000b7 (t) ident_complete
ffffffff81000100 (T) secondary_startup_64
...
But, taking a 32-bit x86 2.6.34-rc5 kernel as an example, the per-cpu symbols
are as shown below:
crash> sym -l
...
c0a90000 (D) __per_cpu_start <==
c0a90000 (V) gdt_page
c0a91000 (V) xen_vcpu
c0a91020 (V) xen_vcpu_info
c0a91060 (V) idt_desc
...
c0a995c0 (V) cfd_data
c0a99600 (V) call_single_queue
c0a99640 (V) csd_data
c0a99654 (D) __per_cpu_end <==
c0a9a000 (r) .smp_locks
c0a9a000 (D) __init_end
c0a9a000 (R) __smp_locks
c0aa1000 (b) .bss
c0aa1000 (B) __bss_start
c0aa1000 (R) __smp_locks_end
c0aa1000 (b) swapper_pg_pmd
c0aa2000 (b) swapper_pg_fixmap
c0aa3000 (B) empty_zero_page
c0aa4000 (b) level1_ident_pgt
...
So similar to the case of the "legacy" x86_64 example, if the
"pcpu_reserved_chunk_limit" value is simply added to the value
of the base kernel's "__per_cpu_end" value of c0a99654, it will
extend into (and overlap) legitimate base kernel virtual address
space.
And I haven't even bothered to look into how it would affect
the operation of all of the other architectures...
Perhaps the patch can be re-done so that the code can handle it on a
per-module basis without inadvertently affecting everything else?
In any case, there's no way I can accept the patch as it is
currently designed.
Thanks,
Dave
15 years, 1 month
Re: [Crash-utility] [PATCH 0/4] To support module percpu symbol
by Dave Anderson
----- "Toshikazu Nakayama" <nakayama.ts(a)ncos.nec.co.jp> wrote:
> Hi,
>
> The crash utility could not get module percpu symbols.
> Since percpu symbols are not included in kallsyms nor module_core area,
> they were not found out in module_init().
>
> However, symbols are existing in module object file
> .data..percpu (legacy kernel revision .data.percpu) section,
> load_module_symbols() can find and resolve them.
> (percpu symbols exist in mod_load_symtable, not in mod_ext_symtable)
>
> 'p' command is also updated to be made redundant legacy per_cpu__
> prefix.
>
> Note:
> I have not tested these paches enough over any arch or kernel revisions,
> also any distros.
> Just have been tested x86 (64bit) with linux-2.6.10 and linux-2.6.35.
>
> This is my first post to crash utility.
> I don't understand well the manner of this mailing list.
> (I'm sorry if there's anything wrong)
>
> Thanks,
> Toshi.
You've come to the right place... ;-)
And your patch submission is fine, although I do prefer that you send the
the patches as email attachments instead of inline. (Although that's my
problem because of the Zimbra email client that I use)
I tested your patch with two kernel types, one of them the "legacy" version
with "pcpu_num_used" and "pcpu_size" symbols (RHEL5), and the second one with
current kernels with the "pcpu_reserved_chunk_limit" symbol (RHEL6 and 2.6.37-rc1).
Here's what I see, first without, and then with your patch applied.
(1) Testing with a 2.6.18-based RHEL5 kernel:
Without your patch, the stored __per_cpu_start and __per_cpu_end
values are as shown here in the symbol_table_data structure:
crash> help -s
...
__per_cpu_start: ffffffff80419000
__per_cpu_end: ffffffff8041f508
...
which reflect the beginning and end of the __per_cpu kernel
symbol range:
crash> sym -l
...
ffffffff80419000 (A) __per_cpu_start
ffffffff80419000 (D) per_cpu__init_tss
ffffffff8041b080 (D) per_cpu__orig_ist
ffffffff8041b100 (d) per_cpu__idle_state
...
ffffffff8041f480 (d) per_cpu__flow_flush_tasklets
ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
ffffffff8041f500 (d) per_cpu____icmp_socket
ffffffff8041f508 (A) __per_cpu_end
With your patch applied, the __per_cpu_end gets incremented based
upon the "pcpu_num_used" and "pcpu_size" contents:
crash> help -s
...
__per_cpu_start: ffffffff80419000
__per_cpu_end: ffffffff80427a10
...
But that doesn't seem right, because the resultant value
pushes it into the bss symbol range:
crash> sym ffffffff80427a10
ffffffff80427a10 (B) boot_exception_stacks+6672
crash>
crash> sym -l
...
ffffffff8041f4c0 (d) per_cpu__rt_cache_stat
ffffffff8041f500 (d) per_cpu____icmp_socket
ffffffff8041f508 (A) __per_cpu_end <-- original value
ffffffff80420000 (d) .data_nosave
ffffffff80420000 (A) __nosave_begin
ffffffff80420000 (D) in_suspend
ffffffff80421000 (b) .bss
ffffffff80421000 (A) __bss_start
ffffffff80421000 (A) __nosave_end
ffffffff80421000 (B) empty_zero_page
ffffffff80422000 (B) boot_cpu_stack
ffffffff80426000 (B) boot_exception_stacks <-- bumped to ffffffff80427a10
ffffffff8042c000 (B) idt_table
ffffffff8042d000 (B) boot_delay
ffffffff8042d008 (B) printk_delay_msec
...
(2) Testing with a 2.6.32-based RHEL6 kernel:
Without your patch, the stored __per_cpu_start and __per_cpu_end
values are as shown here in the symbol_table_data structure:
crash> help -s
...
__per_cpu_start: 0
__per_cpu_end: 17358
...
which reflect the beginning and end of the kernel's __per_cpu
symbol range:
crash> sym -l
0 (D) __per_cpu_start
0 (V) per_cpu__irq_stack_union
4000 (V) per_cpu__gdt_page
5000 (V) per_cpu__exception_stacks
...
16940 (V) per_cpu__cpu_tlbstate
16980 (V) per_cpu__runqueues
17340 (V) per_cpu__sched_clock_data
17358 (D) __per_cpu_end
ffffffff81000000 (T) _text
...
With your patch applied, the st->__per_cpu_end value gets changed
like so:
crash> help -s
...
__per_cpu_start: 0
__per_cpu_end: ffffffff81bf36b8
...
But that doesn't seem right, where ffffffff81bf36b8 is the
"pcpu_reserved_chunk_limit" symbol value:
crash> sym ffffffff81bf36b8
ffffffff81bf36b8 (b) pcpu_reserved_chunk_limit
crash>
I'm guessing that you want it st->__per_cpu_end to be bumped from
0x17358 to 0x19358:
crash> px pcpu_reserved_chunk_limit
pcpu_reserved_chunk_limit = $4 = 0x19358
crash>
(3) Testing with a generic upstream 2.6.37-rc1 kernel, I see the same
behaviour as with the RHEL6 kernel.
In any case, there's no way I can change the st->__per_cpu_end values
as you propose -- even if it somehow works with your testing.
Comments?
Dave
> -----------------------------------------------------------------------------
> CHANGES: module percpu symbols are resolved while module object loading.
> cmd_p() can read percpu syombols without prefix in legacy environs.
>
> SRPM: crash-5.0.9-0.src.rpm
>
> TEST#1: Result on linux-2.6.10 x86(64) SMP(4 CPUs) * with test module
>
> [before]
>
> crash> sym -m test | grep abc
> crash> p abc
> p: gdb request failed: p abc
>
> [after]
>
> crash> sym -m test | grep abc
> ffffffff80871928 (d) per_cpu__abc
> crash> sym abc
> symbol not found: abc
> possible alternatives:
> ffffffff80871928 (d) per_cpu__abc
> crash> p abc
> PER-CPU DATA TYPE:
> int per_cpu__abc;
> PER-CPU ADDRESSES:
> [0]: 10008af0888
> [1]: 10008b00888
> [2]: 10008b10888
> [3]: 10008b20888
>
> TEST#2: Result on linux-2.6.35 x86(64) SMP (8 CPUs) * with KVM
> modules
>
> crash> mod | grep kvm
> ffffffffa0035c40 kvm_intel 39368
> /lib/modules/2.6.35/kernel/arch/x86/kvm/kvm-intel.ko
> ffffffffa008dba0 kvm 175160
> /lib/modules/2.6.35/kernel/arch/x86/kvm/kvm.ko
>
> [before]
>
> crash> sym -m kvm_intel | grep vcpus_on_cpu
> crash> p vcpus_on_cpu
> p: gdb request failed: p vcpus_on_cpu
>
> [after]
>
> crash> sym -m kvm_intel | grep vcpus_on_cpu
> 12a40 (d) vcpus_on_cpu
> crash> p vcpus_on_cpu
> PER-CPU DATA TYPE:
> struct list_head vcpus_on_cpu;
> PER-CPU ADDRESSES:
> [0]: ffff880001812a40
> [1]: ffff880001852a40
> [2]: ffff880001892a40
> [3]: ffff8800018d2a40
> [4]: ffff880001912a40
> [5]: ffff880001952a40
> [6]: ffff880001992a40
> [7]: ffff8800019d2a40
>
> Toshikazu Nakayama (4):
> Add percpu member.
> module's percpu basic procedure.
> expand percpu area.
> update cmd_p().
>
> defs.h | 2 ++
> kernel.c | 1 +
> symbols.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
> 3 files changed, 48 insertions(+), 5 deletions(-)
>
> --
> 1.7.3.2.161.g3089c
15 years, 1 month
[PATCH 0/4] To support module percpu symbol
by Toshikazu Nakayama
Hi,
The crash utility could not get module percpu symbols.
Since percpu symbols are not included in kallsyms nor module_core area,
they were not found out in module_init().
However, symbols are existing in module object file
.data..percpu (legacy kernel revision .data.percpu) section,
load_module_symbols() can find and resolve them.
(percpu symbols exist in mod_load_symtable, not in mod_ext_symtable)
'p' command is also updated to be made redundant legacy per_cpu__ prefix.
Note:
I have not tested these paches enough over any arch or kernel revisions,
also any distros.
Just have been tested x86 (64bit) with linux-2.6.10 and linux-2.6.35.
This is my first post to crash utility.
I don't understand well the manner of this mailing list.
(I'm sorry if there's anything wrong)
Thanks,
Toshi.
-----------------------------------------------------------------------------
CHANGES: module percpu symbols are resolved while module object loading.
cmd_p() can read percpu syombols without prefix in legacy environs.
SRPM: crash-5.0.9-0.src.rpm
TEST#1: Result on linux-2.6.10 x86(64) SMP(4 CPUs) * with test module
[before]
crash> sym -m test | grep abc
crash> p abc
p: gdb request failed: p abc
[after]
crash> sym -m test | grep abc
ffffffff80871928 (d) per_cpu__abc
crash> sym abc
symbol not found: abc
possible alternatives:
ffffffff80871928 (d) per_cpu__abc
crash> p abc
PER-CPU DATA TYPE:
int per_cpu__abc;
PER-CPU ADDRESSES:
[0]: 10008af0888
[1]: 10008b00888
[2]: 10008b10888
[3]: 10008b20888
TEST#2: Result on linux-2.6.35 x86(64) SMP (8 CPUs) * with KVM modules
crash> mod | grep kvm
ffffffffa0035c40 kvm_intel 39368 /lib/modules/2.6.35/kernel/arch/x86/kvm/kvm-intel.ko
ffffffffa008dba0 kvm 175160 /lib/modules/2.6.35/kernel/arch/x86/kvm/kvm.ko
[before]
crash> sym -m kvm_intel | grep vcpus_on_cpu
crash> p vcpus_on_cpu
p: gdb request failed: p vcpus_on_cpu
[after]
crash> sym -m kvm_intel | grep vcpus_on_cpu
12a40 (d) vcpus_on_cpu
crash> p vcpus_on_cpu
PER-CPU DATA TYPE:
struct list_head vcpus_on_cpu;
PER-CPU ADDRESSES:
[0]: ffff880001812a40
[1]: ffff880001852a40
[2]: ffff880001892a40
[3]: ffff8800018d2a40
[4]: ffff880001912a40
[5]: ffff880001952a40
[6]: ffff880001992a40
[7]: ffff8800019d2a40
Toshikazu Nakayama (4):
Add percpu member.
module's percpu basic procedure.
expand percpu area.
update cmd_p().
defs.h | 2 ++
kernel.c | 1 +
symbols.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 48 insertions(+), 5 deletions(-)
--
1.7.3.2.161.g3089c
15 years, 1 month