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

[Dave Anderson]

----- 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