----- Original Message -----
> At 2012-5-25 3:12, Dave Anderson wrote:
> > But again, the red-black tree dump should be similar to the radix tree
> > dump, and both of them should be similar to the "list" command.
> Fixed. I misunderstood the address printed by list command. I thought I
> need to print the address of tree node.
You have fixed the red-black tree issue, but now it seems that the
radix tree dump no longer works.
As a verification of a red-black tree list, let's again take the list
of vm_area_structs that in a red-black list rooted at each mm_struct:
crash> vm | head -3
PID: 16041 TASK: ffff8100100187a0 CPU: 1 COMMAND: "crash"
MM PGD RSS TOTAL_VM
ffff810039735ac0 ffff810001d8a000 111400k 190424k
crash>
So the mm_struct address is ffff810039735ac0, its rb_root is
at offset "-r mm_struct.mm_rb", and each rb_node in the list
can be found at the offset "-o vm_area_struct.vm_rb". To
verify, the "-s vm_area_struct.vm_mm" of each entry should
point back to the mm_struct:
But in fixing red-black tree list, it seems that you have
broken radix tree lists, which worked OK in the previous patch.
The address_space.page_tree example that you use in the help page,
and that I used in my last email, now fails. For example, take an
address_space structure address of ffff81000d1edb88:
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
05-29-2012, 06:57 PM
Dave Anderson
Adding a new command rbtree
----- Original Message -----
>
>
> ----- Original Message -----
> > At 2012-5-25 3:12, Dave Anderson wrote:
> > > But again, the red-black tree dump should be similar to the radix tree
> > > dump, and both of them should be similar to the "list" command.
> > Fixed. I misunderstood the address printed by list command. I thought I
> > need to print the address of tree node.
>
> You have fixed the red-black tree issue, but now it seems that the
> radix tree dump no longer works.
As it turns out, the radix tree problem I reported seems to be an issue
associated with the kernel version.
With your latest patch, radix tree dumps do work OK on RHEL6 (and later)
kernels. For example:
In line 3779, you calculate "height" using: MEMBER_OFFSET("radix_tree_node", "height")
which returns -1, because RHEL5's radix_tree_node does not have a "height" member:
crash> radix_tree_node
struct radix_tree_node {
unsigned int count;
void *slots[64];
long unsigned int tags[2][1];
}
SIZE: 536
crash>
In RHEL6, a radix_tree_node structure does have a "height" member:
crash> radix_tree_node
struct radix_tree_node {
unsigned int height;
unsigned int count;
struct rcu_head rcu_head;
void *slots[64];
long unsigned int tags[3][1];
}
SIZE: 560
crash>
In any case, I now see that your patch uses MEMBER_OFFSET(...) in 4 places:
which is not acceptable, because it hides these kinds of programming errors.
For each of the 4 structure members above, add them to the offset_table[] array,
initialize them with MEMBER_OFFSET_INIT(), and use OFFSET(...) instead of
MEMBER_OFFSET(). Then if your code is incorrect as above, it will fail
immediately and show a meaningful error message.
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
05-30-2012, 03:48 AM
qiaonuohan
Adding a new command rbtree
Modified. I remain the "-N" option. But for radix tree on RHEL5 or
below, the height does not exist. So "-N" is not supported for radix
tree on these version.
At 2012-5-30 3:56, Dave Anderson wrote:
----- Original Message -----
----- Original Message -----
As it turns out, the radix tree problem I reported seems to be an issue
associated with the kernel version.
But when I try the same thing on a RHEL5 kernel, it always fails like
this:
In line 3779, you calculate "height" using: MEMBER_OFFSET("radix_tree_node", "height")
which returns -1, because RHEL5's radix_tree_node does not have a "height" member:
crash> radix_tree_node
struct radix_tree_node {
unsigned int count;
void *slots[64];
long unsigned int tags[2][1];
}
SIZE: 536
crash>
In RHEL6, a radix_tree_node structure does have a "height" member:
crash> radix_tree_node
struct radix_tree_node {
unsigned int height;
unsigned int count;
struct rcu_head rcu_head;
void *slots[64];
long unsigned int tags[3][1];
}
SIZE: 560
crash>
In any case, I now see that your patch uses MEMBER_OFFSET(...) in 4 places:
which is not acceptable, because it hides these kinds of programming errors.
For each of the 4 structure members above, add them to the offset_table[] array,
initialize them with MEMBER_OFFSET_INIT(), and use OFFSET(...) instead of
MEMBER_OFFSET(). Then if your code is incorrect as above, it will fail
immediately and show a meaningful error message.
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
--
--
Regards
Qiao Nuohan
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
05-30-2012, 02:53 PM
Dave Anderson
Adding a new command rbtree
----- Original Message -----
> Modified. I remain the "-N" option. But for radix tree on RHEL5 or
> below, the height does not exist. So "-N" is not supported for radix
> tree on these version.
This is getting tiresome...
With this patch, both red-black or radix tree dumps fail
when dumped from their roots, on RHEL5, RHEL6 and Fedora.
Here are the results using the simple examples shown in your help page.
RHEL5 red-black tree:
crash> vm | head -3
PID: 15684 TASK: ffff81001b4c1820 CPU: 6 COMMAND: "crash"
MM PGD RSS TOTAL_VM
ffff81003b5beb00 ffff810014b43000 112352k 195584k
crash> mm_struct.mm_rb ffff81003b5beb00
mm_rb = {
rb_node = 0xffff8100110a1e78
}
crash> tree -t rb -r mm_struct.mm_rb -o vm_area_struct.vm_rb ffff81003b5beb00
tree: "-r offset" is useless when specified "-N"ffff81003b5bead0
ffff810033522738
7fff36916fd0
tree: invalid kernel virtual address: 7fff36917010 type: "rb_node rb_left"
crash>
tree: "-r offset" is useless when specified "-N"tree: invalid structure member offset: radix_tree_node_height
FILE: tools.c LINE: 3756 FUNCTION: do_rdtree()
crash>
RHEL6 red-black tree:
crash> vm | head -3
PID: 861 TASK: ffff880037309500 CPU: 0 COMMAND: "crash"
MM PGD RSS TOTAL_VM
ffff880037b4d280 ffff880015877000 154492k 291112k
crash> mm_struct.mm_rb ffff880037b4d280
mm_rb = {
rb_node = 0xffff880004d0ee40
}
crash> tree -t rb -r mm_struct.mm_rb -o vm_area_struct.vm_rb ffff880037b4d280
tree: "-r offset" is useless when specified "-N"ffff880037b4d248
ffff88003a0dd750
7fff2fb5cfc8
tree: invalid kernel virtual address: 7fff2fb5d010 type: "rb_node rb_left"
crash>
RHEL6 radix tree:
crash> vtop 400000 | tail -2
PAGE PHYSICAL MAPPING INDEX CNT FLAGS
ffffea000070bde0 20364000 ffff880024da3de0 0 3 2000000000086c
crash> address_space.page_tree ffff880024da3de0
page_tree = {
height = 3,
gfp_mask = 32,
rnode = 0xffff88001da1c911
}
crash> tree -t radix -r address_space.page_tree ffff880024da3de0
tree: "-r offset" is useless when specified "-N"tree: "-N" option is not supported or applicable for radix tree on this architecture or kernel
crash>
Fedora 3.3.1-3.fc16 red-black tree:
crash> vm | head -3
PID: 1175 TASK: ffff88003acb9730 CPU: 2 COMMAND: "crash"
MM PGD RSS TOTAL_VM
ffff880038495c00 ffff88003ac5a000 181580k 335436k
crash> mm_struct.mm_rb ffff880038495c00
mm_rb = {
rb_node = 0xffff880036ae4668
}
crash> tree -t rb -r mm_struct.mm_rb -o vm_area_struct.vm_rb ffff880038495c00
tree: "-r offset" is useless when specified "-N"ffff880038495bc8
ffff880036ae4968
4703fc8
tree: invalid kernel virtual address: 4704010 type: "rb_node rb_left"
crash>
Fedora 3.3.1-3.fc16 radix tree:
crash> vtop 400000 | tail -2
PAGE PHYSICAL MAPPING INDEX CNT FLAGS
ffffea0000ad2500 2b494000 ffff88003a209320 0 2 2000000002006c
crash> address_space.page_tree ffff88003a209320
page_tree = {
height = 2,
gfp_mask = 32,
rnode = 0xffff88003dff6e99
}
crash> tree -t radix -r address_space.page_tree ffff88003a209320
tree: "-r offset" is useless when specified "-N"tree: "-N" option is not supported or applicable for radix tree on this architecture or kernel
crash>
Do you even test these patches before posting them?
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
05-31-2012, 01:59 AM
qiaonuohan
Adding a new command rbtree
At 2012-5-30 22:53, Dave Anderson wrote:
Do you even test these patches before posting them?
Sorry. I made a patch, and after it was tested, I rework it and forget
to test it again. It's all my fault that made so much trouble to you. I
apologize for it.
--
--
Regards
Qiao Nuohan
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
05-31-2012, 06:34 PM
Dave Anderson
Adding a new command rbtree
----- Original Message -----
> At 2012-5-30 22:53, Dave Anderson wrote:
> > Do you even test these patches before posting them?
>
> Sorry. I made a patch, and after it was tested, I rework it and forget
> to test it again. It's all my fault that made so much trouble to you. I
> apologize for it.
No problem...
OK, so here is my current test matrix and results:
(1) tree -t rbtree -r mm_struct.mm_rb -o vm_area_struct.vm_rb <root>
RHEL5: OK
RHEL6: OK
Fedora: OK
(2) tree -t rbtree -o vm_area_struct.vm_rb -N <node>
RHEL5: OK
RHEL6: OK
Fedora: OK
(3) tree -t radix -r address_space.page_tree <root>
RHEL5: OK
RHEL6: OK
Fedora: OK
(4) tree -t radix -N <node>
RHEL5: OK (not supported)
RHEL6: OK (note caveat below)
Fedora: OK (note caveat below)
Caveat: for newer kernels that support -N for radix trees, a user might
run into a situation where the radix_tree_node pointer has the "1-bit"
set, and the node address is just cut-and-pasted. For example:
I forget what the usage of the low-bit means w/respect to
radix_tree_node pointers (?), but you've already got this in
place in your do_rdtree() function:
if (node_p & 1)
node_p &= ~1;
and I remember having to do the same thing in the currently
existing radix_tree_lookup() function.
So I believe that the 1-bit should be stripped off when a user
inadvertently enters such an address as a -N radix_tree_node address.
With this change to cmd_tree():
if (hexadecimal_only(args[optind], 0)) {
value = htol(args[optind], FAULT_ON_ERROR, NULL);
if (IS_KVADDR(value)) {
td->start = value;
+ if ((td->start & 1) &&
+ (td->flags & TREE_NODE_POINTER) &&
+ (type_flag == RADIXTREE_REQUEST))
+ td->start &= ~1;
goto next_arg;
}
}
I haven't tinkered with the -p option, although I don't think that it's
a particularly important option. I may add a couple more error-handling
additions, and perhaps some slight user interface changes, and the help
page needs to be cleaned-up/clarified. But aside from that, this is
looking pretty good for inclusion in crash-6.0.8.
No further patch posting is required unless you find a new bug or other
problem -- nice job!
Thanks,
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
05-31-2012, 07:51 PM
Dave Anderson
Adding a new command rbtree
----- Original Message -----
> I haven't tinkered with the -p option, although I don't think that it's
> a particularly important option. I may add a couple more error-handling
> additions, and perhaps some slight user interface changes, and the help
> page needs to be cleaned-up/clarified. But aside from that, this is
> looking pretty good for inclusion in crash-6.0.8.
>
> No further patch posting is required unless you find a new bug or other
> problem -- nice job!
And just in case you notice these, no further patch is required -- I've got
them fixed:
1. I removed the TREE_START_ENTERED #define since it's unused.
2. This error message uses the wrong offset value, it should use the local
"root_offset" variable:
case 'r':
if (td->flags & TREE_ROOT_OFFSET_ENTERED)
error(FATAL,
"root offset value %d (0x%lx) already entered
",
td->member_offset, td->member_offset);
3. The rdtree_interation() function did not do an hq_enter() of each slot entry
to check for duplicates.
4. FYI -- whenever you add new entries to the offset_table[] array, please put
them at the end of the array so that extension modules that were built with
an older "defs.h" will still see the same offsets. The new entries can be still
be displayed next to similar members as you've done in dump_offset_table().
(and thanks for remembering to do that -- most people forget or are unaware of
the function).
Thanks,
Dave
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
Adding a new command rbtree
