[Crash-utility] [PATCH 0/5] new options for memory debug commands
Yu Zhao
yuzhao at google.com
Fri Feb 13 01:31:15 UTC 2015
On Thu, Feb 12, 2015 at 1:28 PM, Dave Anderson <anderson at redhat.com> wrote:
>
>
> ----- 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.
>
>
Thanks for reviewing, Dave.
I'll fix all things you mentioned here and post the updated patches.
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20150212/02e6af29/attachment.htm>
More information about the Crash-utility
mailing list