Re: [Crash-utility] [PATCH v2] Add read diagnostics to crash

----- Original Message ----- Hi Dave, Your patch has merit, but I'm going to pare it down a bit. I use CRASHDEBUG(8) fairly frequently, but with your changes as-is, it's far too verbose. It's also a bit redundant -- here's an example of a CRASHDEBUG(8) kdump read currently: crash> rd ffff88003c1a5cc0 <addr: ffff88003c1a5cc0 count: 1 flag: 490 (KVADDR)> <readmem: ffff88003c1a5cc0, KVADDR, "64-bit KVADDR", 8, (FOE), 7fff0096b5a8> addr: ffff88003c1a5cc0 paddr: 3c1a5cc0 cnt: 8 ffff88003c1a5cc0: 0000000000000000 ........ text hit rate: 65% (3505 of 5378) crash> And then with your patch: crash> rd ffff88003c1a5cc0 <addr: ffff88003c1a5cc0 count: 1 flag: 490 (KVADDR)> <readmem: ffff88003c1a5cc0, KVADDR, "64-bit KVADDR", 8, (FOE), 7fff1c97b6e8> addr: ffff88003c1a5cc0 paddr: 3c1a5cc0 cnt: 8 rd: read_kdump for: virt=ffff88003c1a5cc0 phys=3c1a5cc0 rd: read_kdump trying read_netdump for: virt=ffff88003c1a5cc0 phys=3c1a5cc0 rd: read_netdump(-1, 7fff1c97b6e8, 8, ffff88003c1a5cc0, 3c1a5cc0) rd: read KDUMP or NETDUMP_ELF64 for multi-segment: 3c195fb8 rd: seek and read offset: 3c195fb8 ffff88003c1a5cc0: 0000000000000000 ........ text hit rate: 65% (3505 of 5378) crash> The "<readmem: ...>" and subsequent "addr: ..." lines come from readmem(). I don't understand the need for the next 3 lines, as they are completely redundant: rd: read_kdump for: virt=ffff88003c1a5cc0 phys=3c1a5cc0 rd: read_kdump trying read_netdump for: virt=ffff88003c1a5cc0 phys=3c1a5cc0 rd: read_netdump(-1, 7fff1c97b6e8, 8, ffff88003c1a5cc0, 3c1a5cc0) I don't argue with the displays of the resultant file offsets, but I may move them to CRASHDEBUG(9): rd: read KDUMP or NETDUMP_ELF64 for multi-segment: 3c195fb8 rd: seek and read offset: 3c195fb8 And the same goes for compressed kdumps: crash> rd ffff81005286e0c0 <addr: ffff81005286e0c0 count: 1 flag: 490 (KVADDR)> <readmem: ffff81005286e0c0, KVADDR, "64-bit KVADDR", 8, (FOE), 7fff11d16958> addr: ffff81005286e0c0 paddr: 5286e0c0 cnt: 8 ffff81005286e0c0: 0000000000000000 ........ text hit rate: 66% (239 of 357) crash> With the patch, aside from the "pfn" displays, it's redundant information: crash> rd ffff81005286e0c0 <addr: ffff81005286e0c0 count: 1 flag: 490 (KVADDR)> <readmem: ffff81005286e0c0, KVADDR, "64-bit KVADDR", 8, (FOE), 7fffd3ca4478> addr: ffff81005286e0c0 paddr: 5286e0c0 cnt: 8 rd: read_diskdump(-1, 7fffd3ca4478, 8, ffff81005286e0c0, 5286e0c0), pfn=5286e rd: Buffering page with pfn=5286e and current physaddr=5286e000 ffff81005286e0c0: 0000000000000000 ........ text hit rate: 66% (239 of 357) crash> For starters, I think the patch can be simplified by modifying the "addr: ..." line that is displayed by readmem() just prior to issuing it to the configured pc->readmem function so that it shows the receiving function. So that line, which may be displayed several times if the read crosses a page boundary, could be reworked to show something like this: crash> rd ffff81005286e0c0 <addr: ffff81005286e0c0 count: 1 flag: 490 (KVADDR)> <readmem: ffff81005286e0c0, KVADDR, "64-bit KVADDR", 8, (FOE), 7fffd3ca4478> <read_netdump: addr: ffff81005286e0c0 paddr: 5286e0c0 cnt: 8 ... and then a line or two from the relevant read functions would show the file offset that was resolved from the physical address. Also, the new function that translates the pc->readmem function address to a string for display could also be used in a single location in main.c instead of having to put debug statements in each location where where pc->readmem is initialized. So let me play around with this a bit... Thanks, Dave