Re: [Crash-utility] [PATCH v2 00/25] support hiding data of offline cpu

----- Original Message ----- Hello Qiao, In the future, can you please post the patches as attachments? Unfortunately my Zimbra email web interface replace tabs with spaces, so I have to manually cut-and-paste each patch from the crash-utility mailing list archives. I have a problem with a number of the patches, some of which I have NAK'd completely, and others I would suggest changing the output slightly. As I recall, one of your arguments for doing this is to avoid verbose display of offline cpu data. But in a few of your patches, it simply replaces a one-line per-cpu piece of data with the <OFFLINE> string. To me it makes more sense to continue to display the data with an "[OFFLINE]" tag appended to the end of the output line. Also, I had asked that there be *no* behavioral changes unless the "offline" environment variable was set to "hide". But there are a few patches that change things regardless of the setting. So for the most part, if any of the individual patches do *not* use your hide_offline_cpu() function, then it's highly likely that it has been NAK'd. Lastly, can you make your output show "[OFFLINE]" instead of "<OFFLINE>"? I'd prefer to keep the usage of "[ brackets ]" as "informational" in nature, whereas "< braces >" are typically used to surround symbol names. Here are the patches that I have NAK'd or would like changed: [PATCH v2 03/25] x86_64: modify help -m/-M to hide offline cpus' data NAK. This patch makes no sense to me. It's a debug command that shows the contents of the crash-internal machdep and machine_specific data structures. What possible benefit would there be to hide the legitimate addresses/contents? Please leave them as they are. [PATCH v2 05/25] x86_64: modify mach to hide offline cpus' data Please continue to display the stack addresses, but *followed by* "[OFFLINE]". [PATCH v2 09/25] modify display_sys_stats() to hide cpus' number Here you are changing crash behavior regardless of the "offline" setting. While it is true that you are now mimicking the PPC64 behaviour, that is because I typically allow IBM to do their own thing with respect to the PPC64 (and S390/S390X) architectures. What I would suggest you do is this: for all architectures except PPC64, continue to show kt->cpus, but append the number of offlined cpus after it, i.e., something like this: KERNEL: ../kdump/vmlinux DUMPFILE: ../kdump/vmcore [PARTIAL DUMP] CPUS: 4 [1 OFFLINE] DATE: Tue Sep 23 14:54:26 2014 UPTIME: 00:02:45 [PATCH v2 10/25] modify help -k to hide offline cpus' number NAK. "help -k" is a debug option whose purpose is to dump the contents of the crash-internal kernel_table; accordingly, the "cpus" output should show the contents of the kt->cpus member regardless whether there are offline cpus. [PATCH v2 13/25] modify irq -a to hide offline cpus' data NAK. This command essentially shows the contents of a cpu mask in a kernel data structure -- regardless whether the cpu is offline. [PATCH v2 16/25] modify ptov offset:cpuspec to hide offline cpus' data There is no reason to *not* show the per-cpu VIRTUAL address, and you're not saving any space. Instead please show the virtual address *followed by* "[OFFLINE]". [PATCH v2 18/25] modify kmem -o to hide offline cpus' data There is no reason to *not* show the per-cpu OFFSET values, and you're not saving any space. Instead please show the offset value *followed by* "[OFFLINE]". [PATCH v2 22/25] modify ps to indicate idle task on offline cpu NAK. This patch changes behavior regardless of the "offline" setting. The swapper task *does* exist, and it actually is the active task on that (offline) cpu. Plus you're doing it only for the active task on the offline cpu -- but there would still be several other kernel tasks shown for that cpu even when it is offline. [PATCH v2 23/25] modify print_task_header() to indicate idle task on offline cpu NAK. This patches changes behavior regardless of the "offline" setting. The print_task_header() function is called by 53 different functions, and I'm not going to even bother checking each one for relevancy. Thanks, Dave