Re: [Crash-utility] [PATCH 0/5] new options for memory debug commands

----- Original Message ----- > search: > -f struct-page.flags-mask > When searching kernel memory, crash walks through all identity > mapping space which includes all physical memory. Nowadays we > have machines that equipped with more than 256G memory, it takes > a long time to go through all pages, especially most of them > are used by user space programs (e.g. LRU pages). This option > allows us to skip pages with certain struct page flags. > -g > Allow us to skip whole compound pages instead of only skipping > head pages when using -f, because some flags are only set to > head pages which tails pages share same properties with their > head pages. > > kmem: > -k > Verify compound page order. Useful when debugging memory leak > caused by freeing compound pages with wrong order. > > -r > Only print used pages (i.e. the page count is not zero). Makes > the output smaller (and faster to grep/vim/emacs). > > Yu Zhao (5): > memory: better compound page support > memory: struct page.flags based filter > memory: skip compound pages when using search filter > memory: display and verify compound page order > memory: kmem option to skip free pages Hello Yu, OK, initially I have looked at these patches from an end-user perspective, and will wait on reviewing the technical implementation of your patchset until some basics below are taken care of. Also, can you separate this into two separate patchset postings, one for the kmem command, and the other for the search command? I understand they both use the compound page flag changes, but the common stuff can be put in one or the other, and it can be specified that one patch essentially depends upon the other being in place.

First, please fix these warnings: $ make warn ... [ cut ] ... cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 memory.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security memory.c: In function ‘PG_slab_flag_init’: memory.c:4909:2: warning: pointer targets in passing argument 2 of ‘enumerator_value’ differ in signedness [-Wpointer-sign] In file included from memory.c:19:0: defs.h:4645:5: note: expected ‘long int *’ but argument is of type ‘ulong *’ memory.c:4910:6: warning: pointer targets in passing argument 2 of ‘enumerator_value’ differ in signedness [-Wpointer-sign] In file included from memory.c:19:0: defs.h:4645:5: note: expected ‘long int *’ but argument is of type ‘ulong *’ memory.c:4917:2: warning: pointer targets in passing argument 2 of ‘enumerator_value’ differ in signedness [-Wpointer-sign] In file included from memory.c:19:0: defs.h:4645:5: note: expected ‘long int *’ but argument is of type ‘ulong *’ memory.c:4918:13: warning: pointer targets in passing argument 2 of ‘enumerator_value’ differ in signedness [-Wpointer-sign] In file included from memory.c:19:0: defs.h:4645:5: note: expected ‘long int *’ but argument is of type ‘ulong *’ memory.c: In function ‘__is_page_head’: memory.c:18332:9: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses] memory.c: In function ‘is_page_head’: memory.c:18345:9: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses] memory.c: In function ‘__is_page_tail’: memory.c:18354:9: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses] memory.c: In function ‘is_page_tail’: memory.c:18367:9: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses] ... Since the kmem -k and -r flags are only relevant when used with -p, then that should be specified clearly in the help page synopsis, i.e., something like: -"[-f|-F|-p|-c|-C|-i|-s|-S|-v|-V|-n|-z|-o|-h] [slab] [[-P] address]\n" +"[-f|-F|-p [-k][-r]|-c|-C|-i|-s|-S|-v|-V|-n|-z|-o|-h] [slab] [[-P] address]\n" And in cmd_kmem(), any attempt to use -k or -r outside of kmem -p should generate an error message and fail immediately. So in the case of a specified address, put a check for the kflag and rflag right after the for(...) line, and error(FATAL, ...) right there: 4607 for (i = 0; i < spec_addr; i++) { 4608 4609 if (Pflag) 4610 meminfo.memtype = PHYSADDR; 4611 else 4612 meminfo.memtype = IS_KVADDR(value[i]) ? 4613 KVADDR : PHYSADDR; 4614 4615 if (fflag) { 4616 meminfo.spec_addr = value[i]; 4617 meminfo.flags = ADDRESS_SPECIFIED; 4618 if (meminfo.calls++) 4619 fprintf(fp, "\n"); 4620 vt->dump_free_pages(&meminfo); 4621 fflag++; 4622 } 4623 4624 if (pflag) { 4625 meminfo.spec_addr = value[i]; 4626 meminfo.flags = ADDRESS_SPECIFIED; 4627 dump_mem_map(&meminfo); 4628 pflag++; 4629 } 4630 And here, you should check for the invalid kflag and rflag if the pflag is not set: 4735 if (pflag == 1) { 4736 if (kflag) 4737 meminfo.extra_flags |= VERIFY_COMPOUND_PAGES; 4738 if (rflag) 4739 meminfo.extra_flags |= SKIP_FREE_PAGES; 4740 dump_mem_map(&meminfo); 4741 } i.e., something like: if (pflag == 1) { (as above) } else if (kflag || rflag) error(FATAL, ...) Lastly, I'm not all that excited about the -k option. It sounds like you have had a specific bug where the "freed with the wrong order" bug occurred, and now you're addressing it from the crash utility after the fact. How often does that even happen? I don't know -- others may disagree. With respect to the search command, the new options need better documentation. First, I don't even see -g in the synopsis? It should be contained inside the [-f ...] block. Then, the "-f struct-page.flags-mask" option needs to be simplified in its name string, maybe "-f page-mask". And the "preceded by the letter n" business is something that's never been done by any other command, and it's just kind of strange. Maybe there could be both an "-f page-mask" and an "-F page-mask"? Also, fix this: SYNOPSIS search [-s start] [ -[kKV] | -u | -p | -t ] [-e end | -l length] [-m mask] [-f struct-page.flags-mask] [-x count] -[cwh] [value | (expression) | symbol | string] ... so that it does not exceed 80 columns. The explanation of each option should be on the same line: -g Skip whole compound page instead of only skipping head pages when using with -f. Only has effect with -f. Is there a good reason for restricting -f to virtual addresses only? And like cmd_kmem(), cmd_search() should have error checking, and fail if the -g option is used without -f (or -F). Thanks, Dave