> Date: Fri, 19 Jun 2015 15:40:22 -0400 > From: anderson@xxxxxxxxxx > To: crash-utility@xxxxxxxxxx > Subject: Re: [Crash-utility] [PATCH v2] files: support dump file pages from its address space > > > 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> I fixed line up problem in v3 patch, please check v3 patch in my another mail. The major problem here is ADDR-SPACE and PAGE-COUNT width is greater than long size on 32 bit kernel. > 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> I can't reproduce this issue on my 32bit kernel. Could you verify whether you can get the same number of pages by tree command? First step, get the page_tree address, address_space.page_tree e6c6a754 Second step, using tree command dump pages, tree -t radix -N <page_tree address> > > 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 I removed page_tree address in patch v3. > > 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: In my v3 patch, I files -a accept inode as parameter, instead of address space address. The benefit of this change is, files -a is more separate with files -m. We decoupled two commands here. In many cases, when we get a file from kernel core, we have the requirements that dump its memory pages by using files -a with this file inode. > > 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: Yes. I used -m and -a now. But how about using -p instead of -a for page dump? > 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. Give an update in v3. I didn't add examples at this time point. Will add it after CLI got confirmed. Anyway, please review all related changes in v3 patch. Thanks. |
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility