3 p.m.
And a couple other things I forgot to mention...
The vast majority of the "files -M" output consists of either the file
descriptors for CHR, FIFO, SOCK, etc., files, or REG file descriptors
that don't have any pages mapped into the task's address space.
(1) Wouldn't it make more sense to show *only* the file descriptors
that have mapped pages? And if the task has no mapped pages,
just display "(no mapped pages)".
(2) Is there any good reason to show the ROOT and CWD directories?
Dave
----- Original Message -----
Hi Oliver,
A few more comments and suggestions regarding your patch.
A couple things I noted when testing on a 32-bit x86.
First, the columns don't line up correctly:
crash> files -M 3804
PID: 3804 TASK: f466a5e0 CPU: 0 COMMAND: "crash"
ROOT: / CWD: /root/crash-5.1.8
FD ADDR-SPACE PAGE-COUNT INODE TYPE PATH
0 e6d6f63c 0 e6d6f570 CHR /dev/pts/0
1 e6d6f63c 0 e6d6f570 CHR /dev/pts/0
2 e6d6f63c 0 e6d6f570 CHR /dev/pts/0
3 f53ec874 0 f53ec7a8 CHR /dev/null
4 f02944d4 0 f0294408 CHR /dev/crash
5 e6c6a294 0 e6c6a1c8 REG /tmp/tmpfvd9PjN
6 e6ca9e54 0 e6ca9d88 FIFO
7 e6ca9e54 0 e6ca9d88 FIFO
8 e6c6a754 147034 e6c6a688 REG
/root/crash-5.1.8/snapshot-2.6.40.4-5.fc15.i686.PAE
9 e6caa3f4 0 e6caa328 FIFO
crash>
And secondly, taking the address_space e6c6a754 from the task above,
again, shouldn't the page count above be reflected in the number of
shown by the address_space tree dump, where the page dump seems to
be missing about 20000 pages?:
crash> files -m e6c6a754 | wc -l
128825
crash>
For the address_space page tree dump, I don't see any point in displaying the
page_tree member address:
crash> files -m ffff810218e31220
Address Space ffff810218e31220, page tree ffff810218e31228, 12 pages
PAGE PHYSICAL MAPPING INDEX CNT FLAGS
ffff81010774d1f8 221609000 ffff810218e31220 0 1 22010000001006c
ffff81010485b378 14ac59000 ffff810218e31220 1 1 14810000001006c
ffff810107993a78 22bc79000 ffff810218e31220 2 1 228100000010028
ffff8101049d3660 1517d4000 ffff810218e31220 3 1 150100000010028
ffff810103b29670 10e742000 ffff810218e31220 4 1 108100000010028
ffff810106d51ba0 1f3bec000 ffff810218e31220 5 1 1f0100000010028
ffff810103ac95f0 10cbd2000 ffff810218e31220 6 1 108100000010028
ffff810106c8cc18 1f03a5000 ffff810218e31220 7 1 1f0100000010028
ffff8101077b1028 223293000 ffff810218e31220 8 1 220100000010028
ffff810106cc03b0 1f125a000 ffff810218e31220 9 1 1f0100000010028
ffff810107a04cd8 22dccd000 ffff810218e31220 a 1 228100000010028
ffff8101078741b8 226a51000 ffff810218e31220 b 1 220100000010028
crash>
For that matter, displaying the address_space address is redundant
since (1) it has to be entered as the command argument, and (2) it gets
shown in every page line "MAPPING". On the other hand, perhaps the inode
that contains the address_space structure would be helpful, say, like
this:
crash> files -m ffff810218e31220
ADDRESS_SPACE INODE PAGES
ffff810218e31220 ffff810218e310e0 12
PAGE PHYSICAL MAPPING INDEX CNT FLAGS
ffff81010774d1f8 221609000 ffff810218e31220 0 1 22010000001006c
ffff81010485b378 14ac59000 ffff810218e31220 1 1 14810000001006c
ffff810107993a78 22bc79000 ffff810218e31220 2 1 228100000010028
ffff8101049d3660 1517d4000 ffff810218e31220 3 1 150100000010028
ffff810103b29670 10e742000 ffff810218e31220 4 1 108100000010028
ffff810106d51ba0 1f3bec000 ffff810218e31220 5 1 1f0100000010028
ffff810103ac95f0 10cbd2000 ffff810218e31220 6 1 108100000010028
ffff810106c8cc18 1f03a5000 ffff810218e31220 7 1 1f0100000010028
ffff8101077b1028 223293000 ffff810218e31220 8 1 220100000010028
ffff810106cc03b0 1f125a000 ffff810218e31220 9 1 1f0100000010028
ffff810107a04cd8 22dccd000 ffff810218e31220 a 1 228100000010028
ffff8101078741b8 226a51000 ffff810218e31220 b 1 220100000010028
crash>
Also, I usually try to avoid using upper-case capital letters as arguments
unless there aren't any more logical lower-case letters left to use.
How about changing your "files -M" and "files -m" to something like:
crash> files -m [pid|task] (for your current files -M)
and
crash> files -a <address_space> (for your current "files -m");
And don't forget to update the help_files[] array with your new
functionality.
Thanks,
Dave
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility