11:43 a.m.
Hi all,
a few people complained to me that the crash utility lacks a
user-friendly interface for percpu variables, especially if they are
dynamically allocated. I implemented a new command which should make
these people happy.
Tested on kernel 3.11, but I didn't really add anything
version-dependent (instead I used the existing infrastructure).
Comments welcome!
Petr Tesarik
11:45 a.m.
New subject: [PATCH 1/3] Implement basic percpu functionality
This piece allows to dump a percpu struct or union at a given address
on a given CPU.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
defs.h | 2
global_data.c | 1
help.c | 65 ++++++++++++++++++++++++
symbols.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 224 insertions(+)
11:46 a.m.
New subject: [PATCH 2/3] Extend percpu to recognize static percpu variables
This patch extends the "percpu" command so that it can be used on static
variable directly (a bit like the "p" command).
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
help.c | 4 ++--
symbols.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 52 insertions(+), 9 deletions(-)
11:48 a.m.
New subject: [PATCH 3/3] Allow hex/dec override for "percpu"
Add standard options to override the default decimal/hexadecimal output
format.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
help.c | 5 ++++-
symbols.c | 22 +++++++++++++++++++++-
2 files changed, 25 insertions(+), 2 deletions(-)
2:54 p.m.
----- Original Message -----
Hi all,
a few people complained to me that the crash utility lacks a
user-friendly interface for percpu variables, especially if they are
dynamically allocated. I implemented a new command which should make
these people happy.
Tested on kernel 3.11, but I didn't really add anything
version-dependent (instead I used the existing infrastructure).
Comments welcome!
Petr Tesarik
Hi Petr,
First let me preface this by saying that I like this concept a lot.
A couple comments...
First, a minor nit -- "make warn" shows this:
$ make warn
...
cc -c -g -DX86_64 -DGDB_7_6 symbols.c -I./gdb-7.6/bfd -I./gdb-7.6/include -Wall -O2
-Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security
symbols.c: In function 'cmd_percpu':
symbols.c:6071:2: warning: implicit declaration of function 'isdigit'
[-Wimplicit-function-declaration]
symbols.c:6143:6: warning: suggest parentheses around comparison in operand of
'&' [-Wparentheses]
...
As a quick test, I created a command input file that looks like this:
$ cat input
percpu -a irq_stack_union
percpu -a gdt_page
percpu -a exception_stacks
percpu -a cpu_llc_shared_map
percpu -a cpu_core_map
percpu -a cpu_sibling_map
percpu -a cpu_llc_id
percpu -a cpu_number
percpu -a x86_bios_cpu_apicid
percpu -a x86_cpu_to_apicid
percpu -a cpu_loops_per_jiffy
percpu -a xen_vcpu_info
percpu -a xen_vcpu
percpu -a idt_desc
percpu -a shadow_tls_desc
...
where I displayed all of the per-cpu variables that are seen in
the kernel's per-cpu symbol list, and it worked nicely.
Your help page shows this:
crash> help percpu
NAME
percpu - percpu variables
SYNOPSIS
percpu [-dx][-a] [-c cpu] [cpu]... [struct|union|*] [struct_name] [address|symbol]
So my test used the "[-a]" and "[symbol]" arguments.
But it's not clear to me when you would use the "[struct|union|*]",
"[struct_name]" and/or the "[address]" arguments? The remainder of
the help page give no explanation of what they are, nor does it
contain any examples of how they would be used. I'm presuming that
it has something to do with dynamically-allocated per-cpu variables?
Now, as to the functionality of the command, currently the "p" command
does this when presented with a percpu symbol:
crash> p idle_threads
PER-CPU DATA TYPE:
struct task_struct *idle_threads;
PER-CPU ADDRESSES:
[0]: ffff88021e20e360
[1]: ffff88021e24e360
[2]: ffff88021e28e360
[3]: ffff88021e2ce360
crash>
So then you can cast the address as whatever the type is, and display it.
Your command pulls out the calculated per-cpu address and prints
it exactly as the "p" command does, and then follows it with the
symbolic display:
crash> percpu -a idle_threads
[0]: ffff88021e20e360
(struct task_struct *) 0xffffffff81c13440 <init_task>
[1]: ffff88021e24e360
(struct task_struct *) 0xffff88021282d330
[2]: ffff88021e28e360
(struct task_struct *) 0xffff88021282dac0
[3]: ffff88021e2ce360
(struct task_struct *) 0xffff88021282e250
crash>
Since this patch essentially adds a command that does the
same thing as the "p" command does with regular symbols, I
wonder if it would be better suited to be merged with the
"p" command? Did you try doing something like that? (As you
are probably aware, getting me to add a new command is like
like pulling teeth...)
Nice job, though, this is really going to be a nice addition.
Thanks,
Dave
2:41 p.m.
On Fri, 11 Oct 2013 15:54:54 -0400 (EDT)
Dave Anderson <anderson(a)redhat.com> wrote:
----- Original Message -----
> Hi all,
>
> a few people complained to me that the crash utility lacks a
> user-friendly interface for percpu variables, especially if they are
> dynamically allocated. I implemented a new command which should make
> these people happy.
>
> Tested on kernel 3.11, but I didn't really add anything
> version-dependent (instead I used the existing infrastructure).
>
> Comments welcome!
>
> Petr Tesarik
Hi Petr,
Hi Dave,
First let me preface this by saying that I like this concept a lot.
Glad you like it. I'm pretty sure we'll find a way to implement this
feature in a way that is likable by everybody.
A couple comments...
First, a minor nit -- "make warn" shows this:
$ make warn
...
cc -c -g -DX86_64 -DGDB_7_6 symbols.c -I./gdb-7.6/bfd -I./gdb-7.6/include -Wall -O2
-Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security
symbols.c: In function 'cmd_percpu':
symbols.c:6071:2: warning: implicit declaration of function 'isdigit'
[-Wimplicit-function-declaration]
symbols.c:6143:6: warning: suggest parentheses around comparison in operand of
'&' [-Wparentheses]
...
Ah, yes, I forgot about "make warn" when updating the patch set.
Fixing these minor issues should be easy enough.
As a quick test, I created a command input file that looks like
this:
$ cat input
percpu -a irq_stack_union
percpu -a gdt_page
percpu -a exception_stacks
percpu -a cpu_llc_shared_map
percpu -a cpu_core_map
percpu -a cpu_sibling_map
percpu -a cpu_llc_id
percpu -a cpu_number
percpu -a x86_bios_cpu_apicid
percpu -a x86_cpu_to_apicid
percpu -a cpu_loops_per_jiffy
percpu -a xen_vcpu_info
percpu -a xen_vcpu
percpu -a idt_desc
percpu -a shadow_tls_desc
...
where I displayed all of the per-cpu variables that are seen in
the kernel's per-cpu symbol list, and it worked nicely.
Your help page shows this:
crash> help percpu
NAME
percpu - percpu variables
SYNOPSIS
percpu [-dx][-a] [-c cpu] [cpu]... [struct|union|*] [struct_name] [address|symbol]
So my test used the "[-a]" and "[symbol]" arguments.
But it's not clear to me when you would use the "[struct|union|*]",
"[struct_name]" and/or the "[address]" arguments? The remainder of
the help page give no explanation of what they are, nor does it
contain any examples of how they would be used. I'm presuming that
it has something to do with dynamically-allocated per-cpu variables?
There is a second example in the help:
Show a per-cpu variable on all processors:
crash> percpu -a
[0]: ffff88011e21a468
struct disk_stats {
sectors = {11197190, 7550896},
ios = {360193, 159113},
merges = {11723, 22075},
ticks = {6180943, 11137498},
io_ticks = 1781827,
time_in_queue = 16785949
}
[and so on...]
The address (0x1a468) was taken from the dkstats field of a struct
hd_struct on my system. The "struct", "union" and "*"
variants are
needed only to disambiguate from a symbol name (if needed) and they are
similar to the "struct", "union" and "*" commands.
Now, as to the functionality of the command, currently the
"p" command
does this when presented with a percpu symbol:
crash> p idle_threads
PER-CPU DATA TYPE:
struct task_struct *idle_threads;
PER-CPU ADDRESSES:
[0]: ffff88021e20e360
[1]: ffff88021e24e360
[2]: ffff88021e28e360
[3]: ffff88021e2ce360
Yes, I know. The crash utility is in fact not so bad when it comes to
global percpu variables.
[...]
Your command pulls out the calculated per-cpu address and prints
it exactly as the "p" command does, and then follows it with the
symbolic display:
crash> percpu -a idle_threads
[0]: ffff88021e20e360
(struct task_struct *) 0xffffffff81c13440 <init_task>
[1]: ffff88021e24e360
(struct task_struct *) 0xffff88021282d330
[2]: ffff88021e28e360
(struct task_struct *) 0xffff88021282dac0
[3]: ffff88021e2ce360
(struct task_struct *) 0xffff88021282e250
crash>
Since this patch essentially adds a command that does the
same thing as the "p" command does with regular symbols, I
wonder if it would be better suited to be merged with the
"p" command? Did you try doing something like that?
Yes, I thought about extending one of the existing commands.
These are the issues I encountered:
1. The "percpu" combines both "*" and "p". The "*"
functionality
(which you didn't test) is in fact primary, and I wanted to get it
with as little typing as possible. If you can propose an acceptably
short variant of "percpu 0 disk_stats 0x1a468" using the existing
commands, I will in fact prefer it to a standalone command.
2. Many options to the cmd_datatype_common were incompatible with the
"percpu" display (e.g. "-f"), some where meaningless (e.g.
"-u").
3. I'd have to implement percpu twice (once for "p" and once for
cmd_datatype_common.
4. Percpu without any argument uses the current CPU. Try something like
"set -c 0", "percpu current_task", then
"set -c 1", "percpu current_task"
This may not be needed so often, so I'm fine with making it an
option.
In general, I think it's a matter of taste, and if you dislike new
commands, it all boils down to finding a suitable syntax to extend the
existing commands. Unfortunately, "-c" (as CPU) is already taken for
count, and "-p" (as processor) is already taken for pointer
dereference. :-(
I can think of:
A. "-C" (but it requires a shift key, and two options that only
differ in case may be confusing)
B. any other random letter ("-a" is free).
If you find an option letter (let's mark it "-?" here), it could be
used like this:
p -? current_task # global var on the current CPU
p -?1-3 current_task # global var on selected CPUs
p -?a current_task # global var on all CPUs
struct -? disk_stats 0x1a468 # dynamic var
*disk_stats -?a 0x1a468 # dynamic var on all CPUs
Petr T
11:22 a.m.
On Sat, 12 Oct 2013 21:41:49 +0200
Petr Tesarik <ptesarik(a)suse.cz> wrote:
[...]
In general, I think it's a matter of taste, and if you dislike new
commands, it all boils down to finding a suitable syntax to extend the
existing commands. Unfortunately, "-c" (as CPU) is already taken for
count, and "-p" (as processor) is already taken for pointer
dereference. :-(
I can think of:
A. "-C" (but it requires a shift key, and two options that only
differ in case may be confusing)
B. any other random letter ("-a" is free).
Hm, it seems this is the only blocking piece. If nobody has an idea
until tomorrow, I'll just pick '-C', even though I hate both the shift
key and the fact that another command ('irq') uses a different option
('-c') for the same concept.
Petr T
1:20 p.m.
----- Original Message -----
On Sat, 12 Oct 2013 21:41:49 +0200
Petr Tesarik <ptesarik(a)suse.cz> wrote:
>[...]
> In general, I think it's a matter of taste, and if you dislike new
> commands, it all boils down to finding a suitable syntax to extend the
> existing commands. Unfortunately, "-c" (as CPU) is already taken for
> count, and "-p" (as processor) is already taken for pointer
> dereference. :-(
>
> I can think of:
>
> A. "-C" (but it requires a shift key, and two options that only
> differ in case may be confusing)
> B. any other random letter ("-a" is free).
Hm, it seems this is the only blocking piece. If nobody has an idea
until tomorrow, I'll just pick '-C', even though I hate both the shift
key and the fact that another command ('irq') uses a different option
('-c') for the same concept.
I agree with you on both counts.
I haven't had the chance to reply to your second response (I took the holiday
off yesterday), but I think a suitable compromise would be to change the
"struct/union/* -c count" option to perhaps a "-n number" option?
Another other option that crossed my mind would be to have a unique manner
for presenting per-cpu addresses and cpu(s) as a command argument. They are
offset values, but they could be confused for zero-based kernel virtual address
arches (s390/s390x), so there would need to be a option to differentiate
zero-based kernel virtual addresses, and then a second option of the cpu
specifier.
But anyway, maybe per-cpu addresses could be made to be "self-identified"
if they were presented as "<offset>:<cpu-identifer>", where the
colon would
define the argument as a per-cpu item, and the <cpu-identifier> part could be
one of the following:
offset: nothing following the colon means the current cpu
offset:a all cpus
offset:cpu-specifier same as your patch or the irq command
That way the struct/union/* commands would seemingly only require one
of these new hybrid option formats as an "address" argument, and no
additional -<opt> would be required, right?
For the "p <per-cpu symbol>" command, only the additional cpu identifier
would be required, using either "-c <cpu-specifier">, or maybe it could
also allow a "<per-cpu symbol>:<cpu-specifier>" option, where if the
colon
was not applied, the current behavior would be remain unchanged (because
there still should be a way to just display what it does now). And if
the :<cpu-specifier>" were appended to the symbol name, then it would
go ahead an print the contents of the item(s).
Maybe I'm overthinking it...
But since per-cpu stuff has become so prevalent over the years, maybe it's
worth having a new specifier format. What do you think?
Dave
8:43 a.m.
On Tue, 15 Oct 2013 14:20:08 -0400 (EDT)
Dave Anderson <anderson(a)redhat.com> wrote:
----- Original Message -----
> On Sat, 12 Oct 2013 21:41:49 +0200
> Petr Tesarik <ptesarik(a)suse.cz> wrote:
>
> >[...]
> > In general, I think it's a matter of taste, and if you dislike new
> > commands, it all boils down to finding a suitable syntax to extend the
> > existing commands. Unfortunately, "-c" (as CPU) is already taken for
> > count, and "-p" (as processor) is already taken for pointer
> > dereference. :-(
> >
> > I can think of:
> >
> > A. "-C" (but it requires a shift key, and two options that only
> > differ in case may be confusing)
> > B. any other random letter ("-a" is free).
>
> Hm, it seems this is the only blocking piece. If nobody has an idea
> until tomorrow, I'll just pick '-C', even though I hate both the shift
> key and the fact that another command ('irq') uses a different option
> ('-c') for the same concept.
I agree with you on both counts.
I haven't had the chance to reply to your second response (I took the holiday
off yesterday), but I think a suitable compromise would be to change the
"struct/union/* -c count" option to perhaps a "-n number" option?
Another other option that crossed my mind would be to have a unique manner
for presenting per-cpu addresses and cpu(s) as a command argument. They are
offset values, but they could be confused for zero-based kernel virtual address
arches (s390/s390x), so there would need to be a option to differentiate
zero-based kernel virtual addresses, and then a second option of the cpu
specifier.
FYI I went through the same exercise...
But anyway, maybe per-cpu addresses could be made to be
"self-identified"
if they were presented as "<offset>:<cpu-identifer>", where the
colon would
define the argument as a per-cpu item, and the <cpu-identifier> part could be
one of the following:
offset: nothing following the colon means the current cpu
offset:a all cpus
offset:cpu-specifier same as your patch or the irq command
This is genius! I really love this syntax and I'm working on the patch
already.
Petr T
8:55 a.m.
----- Original Message -----
On Tue, 15 Oct 2013 14:20:08 -0400 (EDT)
Dave Anderson <anderson(a)redhat.com> wrote:
> ----- Original Message -----
> > On Sat, 12 Oct 2013 21:41:49 +0200
> > Petr Tesarik <ptesarik(a)suse.cz> wrote:
> >
> > >[...]
> > > In general, I think it's a matter of taste, and if you dislike new
> > > commands, it all boils down to finding a suitable syntax to extend the
> > > existing commands. Unfortunately, "-c" (as CPU) is already taken
for
> > > count, and "-p" (as processor) is already taken for pointer
> > > dereference. :-(
> > >
> > > I can think of:
> > >
> > > A. "-C" (but it requires a shift key, and two options that only
> > > differ in case may be confusing)
> > > B. any other random letter ("-a" is free).
> >
> > Hm, it seems this is the only blocking piece. If nobody has an idea
> > until tomorrow, I'll just pick '-C', even though I hate both the
shift
> > key and the fact that another command ('irq') uses a different option
> > ('-c') for the same concept.
>
> I agree with you on both counts.
>
> I haven't had the chance to reply to your second response (I took the
> holiday
> off yesterday), but I think a suitable compromise would be to change the
> "struct/union/* -c count" option to perhaps a "-n number"
option?
>
> Another other option that crossed my mind would be to have a unique manner
> for presenting per-cpu addresses and cpu(s) as a command argument. They
> are
> offset values, but they could be confused for zero-based kernel virtual
> address
> arches (s390/s390x), so there would need to be a option to differentiate
> zero-based kernel virtual addresses, and then a second option of the cpu
> specifier.
FYI I went through the same exercise...
> But anyway, maybe per-cpu addresses could be made to be "self-identified"
> if they were presented as "<offset>:<cpu-identifer>", where
the colon would
> define the argument as a per-cpu item, and the <cpu-identifier> part could
> be
> one of the following:
>
> offset: nothing following the colon means the current cpu
> offset:a all cpus
> offset:cpu-specifier same as your patch or the irq command
This is genius! I really love this syntax and I'm working on the patch
already.
How about the somewhat similar usage for "p", where the self-identified object
could also contain a per-cpu symbol:
<per-cpu symbol>:<cpu-identifier>
Then we could do something like:
crash> p <per-cpu symbol>
which would continue to output the type and addresses as it does now,
but then alternatively you could:
crash> p <per-cpu symbol>:<cpu_identifier>
which would just print the datatype contents for the cpus specified?
Dave
4059
days inactive
4064
days old
devel@lists.crash-utility.osci.io
9 comments
2 participants
participants (2)
-
Dave Anderson -
Petr Tesarik