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

----- Original Message ----- > For those who don't care about the removed cpu, data of offline cpu is > bothering. This patchset is trying to hide data of offline cpu. Please > check each patch to see what is hiden. > > The last version is here: > https://www.redhat.com/archives/crash-utility/2014-September/msg00000.html > > Changelog: > v1->v2: > 1. patch 1: add environment variable and its argument to hide/show > data of offline cpu. > 2. add description of environment variable to crash.8 > 3. remove the restrict of x86_64 > 4. add "<OFFLINE>" to indicate those hiden data. > 5. patch 22/23: addd "<OFFLINE>" to indicate idle task on offline > cpu > > Qiao Nuohan (25): > add a flag to display/hide data of offline cpus > add an API to check whether to hide a cpus' data > x86_64: modify help -m/-M to hide offline cpus' data > x86_64: modify bt -E to hide offline cpus' data > x86_64: modify mach to hide offline cpus' data > x86_64: modify mach -c to hide offline cpus' data > modify help -r to hide offline cpus' registers > modify bt -c to hide offline cpus' data > modify display_sys_stats() to hide cpus' number > modify help -k to hide offline cpus' number > modify set -c to hide offline cpu > modify irq -s to hide offline cpus' data > modify irq -a to hide offline cpus' data > modify timer -r to hide offline cpus' data > modify timer to hide offline cpus' data > modify ptov offset:cpuspec to hide offline cpus' data > fix max_cpudata_limit() when offlined cpu exists > modify kmem -o to hide offline cpus' data > modify kmem -S(SLUB) to hide offline cpus' data > modify struct/union/* [:cpuspec] to hide offline cpus' data > modify command p to hide offline cpus' data > modify ps to indicate idle task on offline cpu > modify print_task_header() to indicate idle task on offline cpu > modify ps -l/-m -C cpu to hide offline cpus' data > modify runq to hide offline cpus' data > > crash.8 | 4 +++ > defs.h | 3 ++ > diskdump.c | 6 +++- > help.c | 6 ++++ > kernel.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++---- > main.c | 15 +++++++++- > memory.c | 30 +++++++++++++++---- > netdump.c | 25 ++++++++++++++-- > symbols.c | 15 +++++++++- > task.c | 97 > ++++++++++++++++++++++++++++++++++++++++++++++++++++---------- > tools.c | 15 ++++++++++ > x86_64.c | 74 +++++++++++++++++++++++++++++++++++++++-------- > 12 files changed, 331 insertions(+), 42 deletions(-) > > -- > 1.8.5.3 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 -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility .