Re: [PATCH] add -s option to vm to display one vma at a time

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



At 2012-3-15 3:06, Dave Anderson wrote:


----- Original Message -----
At 2012-3-14 4:45, Dave Anderson wrote:


----- Original Message -----
At 2012-3-13 17:56, qiaonuohan wrote:

Hello Dave,

When I am using "vm -p" command, I feel it is chaotic when too
much data is printed. I think it is clear to display one vma
each time.

In the patch, I compare all vmas with the argument of -s option.
If an equal vma is found, it will be printed like below.

crash>   vm -ps ffff88028ff7d3a0
        VMA           START       END     FLAGS FILE
ffff88028ff7d3a0 7fff29b71000 7fff29b87000 100173
VIRTUAL     PHYSICAL
7fff29b71000  (not mapped)
7fff29b72000  (not mapped)
7fff29b73000  (not mapped)
7fff29b74000  (not mapped)
7fff29b75000  (not mapped)
7fff29b76000  (not mapped)
7fff29b77000  27faad000
7fff29b78000  2807aa000
7fff29b79000  280781000
7fff29b7a000  280787000
7fff29b7b000  280776000
7fff29b7c000  280786000
7fff29b7d000  27f2df000
7fff29b7e000  27f2e0000
7fff29b7f000  27f2e1000
7fff29b80000  27f2d7000
7fff29b81000  28b1ac000
7fff29b82000  28ecc1000
7fff29b83000  28c5c2000
7fff29b84000  28aaf4000
7fff29b85000  28aaf9000
7fff29b86000  289566000
crash>

Seems like a reasonable request.

However, I don't like your mixing it with the "-R reference" usage,
because it confuses things like this simple example:

    crash>   vm -p -s ffff810ec9f57818
          VMA           START       END     FLAGS FILE
    ffff810ec9f57818   4010d000   40110000 101877 /usr/local/java/jdk1.5.0_19/bin/java
    VIRTUAL     PHYSICAL
    4010d000    ec89b1000
    4010e000    FILE: /usr/local/java/jdk1.5.0_19/bin/java  OFFSET: e000
    4010f000    e99803000
    crash>

If the command above were to be qualified with a "-R 4010e000" reference
check, it results in a nonsensical error message:

    crash>   vm -p -s ffff810ec9f57818 -R 4010e000
    vm: only one -R option allowed
    Usage:
      vm [-p | -v | -m | [-R reference] | [-f vm_flags]] [pid |
      taskp] ...
    Enter "help vm" for details.
    crash>

A few suggestions:

(1) Don't even bother to tie it in with the "vm -v" option, because if
      you already know the vm_area_struct address, then just print it with
      "vm_area_struct<address>".

(2) Then, since it *only* applies with the -p functionality, make it
      a new "-P<vmaddr>" option, where the help page explains that the
      <vmaddr>   argument must be a vm_area_struct of the current context:

      Usage:
        vm [-p | -P vmaddr | -v | -m | [-R reference] | [-f
        vm_flags]] [pid | taskp] ...

(3) Make -P mutually exclusive with all of the other options.

(4) Do not use the reference structure for this feature.  Just use your new
      flag, and pass the vma address in the 3rd "vaddr" argument to vm_area_dump(),
      since it's not even being used by cmd_vm().

I have modified the patch, please check.

OK, these are my suggestions to finalize this patch.  First, fix this:

   # make warn
   ...
   cc -c -g -DX86_64  -DGDB_7_3_1  memory.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector
   memory.c: In function "vm_area_dump":
   memory.c:3503:7: warning: "single_vma" may be used uninitialized in this function [-Wuninitialized]
   ...

Secondly, this command is no different from the other context-sensitive
"vm" command options, so please always print the task header like they
do.  Just remove the three restrictions in cmd_vm() that do this:

   -               if (!ref)
   +               if (!(ref || (flag&  PRINT_SINGLE_VMA)))
                         print_task_header(fp, CURRENT_CONTEXT(), 0);

Third, the option is either going to work OK or fail to find the VMA.
Your patch does this if an invalid vm_area_struct address is entered:

   crash>  vm -P ffff880617e7af44
         VMA           START       END     FLAGS FILE
   crash>

Regardless of success or failure, the print_task_header() call in cmd_vm()
should always show the task information.  Then, if you actually find the VMA,
print the "VMA  START ..." header along with its data:

   crash>  vm -P ffff880284cf87e0
   PID: 13318  TASK: ffff8806294f2690  CPU: 11  COMMAND: "crash"
         VMA           START       END     FLAGS FILE
   ffff880284cf87e0     400000     9fa000 8001875 /root/crash-6.0.4/crash
   VIRTUAL     PHYSICAL
   400000      28c7fa000
   401000      29470a000
   402000      29470b000
   403000      29470c000
   404000      29470d000
   405000      29470e000
   406000      29470f000
   ...

But if the command fails to find the VMA, then indicate that under
the task header:

   crash>  vm -P ffff880284cf87e8
   PID: 13318  TASK: ffff8806294f2690  CPU: 11  COMMAND: "crash"
   (not found)
   crash>

Lastly, please don't use subterfuge to accomplish the task at
hand.  The setting of PHYSADDR in the vm_area_dump() function
makes no sense at all:

   +       single_vma_header = 0;
   +       if (flag&  PRINT_SINGLE_VMA) {
   +               flag |= PHYSADDR;
   +               single_vma = vaddr;
   +               vaddr = 0;
   +       }

It may "work" by doing the above, but I'm sure you can accomplish
it just as easily using your PRINT_SINGLE_VMA flag and/or your new
local variables, making it both easier to understand and more
maintainable.

The patch is modified. I think I need to pay more attention to maintainable and thanks for your patience.


Thanks,
   Dave

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility




--
--
Regards
Qiao Nuohan
diff --git a/defs.h b/defs.h
index a942dbb..f3ca47b 100755
--- a/defs.h
+++ b/defs.h
@@ -2241,6 +2241,7 @@ struct load_module {
 #define PRINT_INODES      (0x10)   /* KVADDR, UVADDR, and PHYSADDR */
 #define PRINT_MM_STRUCT   (0x20)
 #define PRINT_VMA_STRUCTS (0x40)
+#define PRINT_SINGLE_VMA  (0x80)
 
 #define MIN_PAGE_SIZE  (4096)
 
diff --git a/help.c b/help.c
index f752283..e294908 100755
--- a/help.c
+++ b/help.c
@@ -3108,7 +3108,7 @@ NULL
 char *help_vm[] = { 
 "vm",
 "virtual memory",
-"[-p | -v | -m | [-R reference] | [-f vm_flags]] [pid | taskp] ... ",
+"[-p | -P vmaddr | -v | -m | [-R reference] | [-f vm_flags]] [pid | taskp] ... ",
 "  This command displays basic virtual memory information of a context,",
 "  consisting of a pointer to its mm_struct and page dirctory, its RSS and ",
 "  total virtual memory size; and a list of pointers to each vm_area_struct,",
@@ -3124,6 +3124,8 @@ char *help_vm[] = {
 "            -p  translate each virtual page to its physical address, or if", 
 "                the page is not mapped, its swap device and offset, or",
 "                filename and offset.",
+"     -P vmaddr  translate each virtual page of the specified VM area to its",
+"                physical address, the output is like -p option.",
 "  -R reference  search for references to this number or filename.",
 "            -m  dump the mm_struct assocated with the task.",
 "            -v  dump all of the vm_area_structs associated with the task.",
@@ -3302,6 +3304,20 @@ char *help_vm[] = {
 "  Translate a FLAGS value:\n",
 "    %s> vm -f 3875",
 "    3875: (READ|EXEC|MAYREAD|MAYWRITE|MAYEXEC|DENYWRITE|EXECUTABLE|LOCKED)",
+" ",
+"  Display the page translations of VM area ffff880037638b70:\n",
+"    %s> vm -P ffff880037638b70",
+"    PID: 24406  TASK: ffff88003baa6b40  CPU: 1   COMMAND: \"crash\"",
+"          VMA           START       END     FLAGS FILE",
+"    ffff880037638b70 7f089c697000 7f089c69e000 80000d1 /usr/lib64/gconv/gconv-modules.cache",
+"    VIRTUAL     PHYSICAL",
+"    7f089c697000  36c3e000",
+"    7f089c698000  36c3d000",
+"    7f089c699000  36c3c000",
+"    7f089c69a000  FILE: /usr/lib64/gconv/gconv-modules.cache  OFFSET: 3000",
+"    7f089c69b000  36c3a000",
+"    7f089c69c000  FILE: /usr/lib64/gconv/gconv-modules.cache  OFFSET: 5000",
+"    7f089c69d000  36c38000",
 NULL               
 };
 
diff --git a/memory.c b/memory.c
index 55a184b..7397c7b 100755
--- a/memory.c
+++ b/memory.c
@@ -3036,16 +3036,18 @@ cmd_vm(void)
 	int c;
 	ulong flag;
 	ulong value;
+	ulong single_vma;
 	ulonglong llvalue;
 	struct task_context *tc;
 	struct reference reference, *ref;
 	int subsequent;
 
 	flag = 0;
+	single_vma = 0;
 	ref = NULL;
 	BZERO(&reference, sizeof(struct reference));
 
-        while ((c = getopt(argcnt, args, "f:pmvR:")) != EOF) {
+        while ((c = getopt(argcnt, args, "f:pmvR:P:")) != EOF) {
                 switch(c)
 		{
 		case 'f':
@@ -3090,6 +3092,15 @@ cmd_vm(void)
 			}
 			break;
 
+		case 'P':
+			if (flag) {
+				argerrs++;
+			} else {
+				flag |= PRINT_SINGLE_VMA;
+				single_vma = htol(optarg, FAULT_ON_ERROR, NULL);
+			}
+			break;
+
 		default:
 			argerrs++;
 			break;
@@ -3102,7 +3113,7 @@ cmd_vm(void)
 	if (!args[optind]) {
 		if (!ref)
 			print_task_header(fp, CURRENT_CONTEXT(), 0);
-		vm_area_dump(CURRENT_TASK(), flag, 0, ref);
+		vm_area_dump(CURRENT_TASK(), flag, single_vma, ref);
 		return;
 	}
 
@@ -3115,14 +3126,14 @@ cmd_vm(void)
 			for (tc = pid_to_context(value); tc; tc = tc->tc_next) {
                                 if (!ref)
                                         print_task_header(fp, tc, subsequent++);
-                                vm_area_dump(tc->task, flag, 0, ref);
+                                vm_area_dump(tc->task, flag, single_vma, ref);
                         }
 			break;
 
 		case STR_TASK:
 			if (!ref)
                                 print_task_header(fp, tc, subsequent++);
-                        vm_area_dump(tc->task, flag, 0, ref);
+                        vm_area_dump(tc->task, flag, single_vma, ref);
 			break;
 
 		case STR_INVALID:
@@ -3388,6 +3399,8 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
 	ulonglong vm_flags;
 	ulong vm_file, inode;
 	ulong dentry, vfsmnt;
+	ulong single_vma;
+	int single_vma_found;
 	int found;
 	struct task_mem_usage task_mem_usage, *tm;
 	char buf1[BUFSIZE];
@@ -3401,6 +3414,13 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
 	tm = &task_mem_usage;
 	get_task_mem_usage(task, tm);
 
+	single_vma = 0;
+	single_vma_found = FALSE;
+	if (flag & PRINT_SINGLE_VMA) {
+		single_vma = vaddr;
+		vaddr = 0;
+	}
+
 	if (ref) {
 		ref->cmdflags = VM_REF_SEARCH;
 		if (IS_A_NUMBER(ref->str)) {
@@ -3420,7 +3440,7 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
                 return (ulong)NULL;
         }
 
-        if (!(flag & (UVADDR|PRINT_MM_STRUCT|PRINT_VMA_STRUCTS)) &&
+        if (!(flag & (UVADDR|PRINT_MM_STRUCT|PRINT_VMA_STRUCTS|PRINT_SINGLE_VMA)) &&
 	    !DO_REF_SEARCH(ref)) 
 		PRINT_VM_DATA();
 
@@ -3443,7 +3463,7 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
                 mkstring(buf3, UVADDR_PRLEN, CENTER|RJUST, "END"),
 		space(MINSPACE));
 
-	if (!(flag & (PHYSADDR|VERIFY_ADDR|PRINT_VMA_STRUCTS)) && 
+	if (!(flag & (PHYSADDR|VERIFY_ADDR|PRINT_VMA_STRUCTS|PRINT_SINGLE_VMA)) && 
 	    !DO_REF_SEARCH(ref)) 
 		fprintf(fp, vma_header);
 
@@ -3462,6 +3482,13 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
 		vm_start = ULONG(vma_buf + OFFSET(vm_area_struct_vm_start));
 		vm_flags = get_vm_flags(vma_buf);
 		vm_file = ULONG(vma_buf + OFFSET(vm_area_struct_vm_file));
+		
+		if (flag & PRINT_SINGLE_VMA) {
+			if (vma != single_vma)
+				continue;
+			fprintf(fp, "%s", vma_header);
+			single_vma_found = TRUE;
+		}
 
 		if (flag & PRINT_VMA_STRUCTS) {
 			dump_struct("vm_area_struct", vma, 0);
@@ -3551,7 +3578,7 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
 			} else {
 				PRINT_VMA_DATA();
 				     
-				if (flag & PHYSADDR) 
+				if (flag & (PHYSADDR|PRINT_SINGLE_VMA)) 
 					vm_area_page_dump(vma, task,
 						vm_start, vm_end, vm_mm, ref);
 			}
@@ -3564,6 +3591,9 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
 	if (flag & VERIFY_ADDR)
 		return (ulong)NULL;
 
+	if ((flag & PRINT_SINGLE_VMA) && !single_vma_found)
+		fprintf(fp, "(not found)\n");
+
 	if ((flag & UVADDR) && !found) 
 		fprintf(fp, "(not found)\n");
 
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility

[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux