Re: [Crash-utility] [PATCH v4 4/6] Introduce maple tree vma iteration to memory.c

Wednesday, 4 January 2023

Several small things about indent and style..

On 2023/01/03 21:05, Tao Liu wrote:
...
 Since memory.c:vm_area_dump will iterate all vma, this patch mainly
 introduces maple tree vma iteration to it.

 We extract the code which handles each vma into a function. If
 mm_struct_mmap exist, aka the linked list of vma iteration available,
 we goto the original way; if not and mm_struct_mm_mt exist, aka
 maple tree is available, then we goto the maple tree vma iteration.

 Signed-off-by: Tao Liu <ltao(a)redhat.com&gt;
 ---
   Makefile |   2 +-
   memory.c | 327 +++++++++++++++++++++++++++++++++----------------------
   2 files changed, 199 insertions(+), 130 deletions(-)

 diff --git a/Makefile b/Makefile
 index 102597f..a94a243 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -355,7 +355,7 @@ filesys.o: ${GENERIC_HFILES} filesys.c
   help.o: ${GENERIC_HFILES} help.c
   	${CC} -c ${CRASH_CFLAGS} help.c ${WARNING_OPTIONS} ${WARNING_ERROR}

 -memory.o: ${GENERIC_HFILES} memory.c
 +memory.o: ${GENERIC_HFILES} ${MAPLE_TREE_HFILES} memory.c
   	${CC} -c ${CRASH_CFLAGS} memory.c ${WARNING_OPTIONS} ${WARNING_ERROR}

   test.o: ${GENERIC_HFILES} test.c
 diff --git a/memory.c b/memory.c
 index 625a94b..534f82f 100644
 --- a/memory.c
 +++ b/memory.c
 @@ -21,6 +21,7 @@
   #include <ctype.h>
   #include <netinet/in.h>
   #include <byteswap.h>
 +#include "maple_tree.h"

   struct meminfo {           /* general purpose memory information structure */
           ulong cache;       /* used by the various memory searching/dumping */
 @@ -136,6 +137,27 @@ struct searchinfo {
   	char buf[BUFSIZE];
   };

 +struct handle_each_vm_area_args {
 +	ulong task;
 +	ulong flag;
 +	ulong vaddr;
 +	struct reference *ref;
 +	char *vma_header;
 +	char *buf1;
 +	char *buf2;
 +	char *buf3;
 +	char *buf4;
 +	char *buf5;
 +	ulong vma;
 +	char **vma_buf;
 +	struct task_mem_usage *tm;
 +	int *found;
 +	int *single_vma_found;
 +	unsigned int radix;
 +	struct task_context *tc;
 +	ulong *single_vma;
 +};
 +
   static char *memtype_string(int, int);
   static char *error_handle_string(ulong);
   static void collect_page_member_data(char *, struct meminfo *);
 @@ -298,6 +320,7 @@ static void dump_page_flags(ulonglong);
   static ulong kmem_cache_nodelists(ulong);
   static void dump_hstates(void);
   static ulong freelist_ptr(struct meminfo *, ulong, ulong);
 +static ulong handle_each_vm_area(struct handle_each_vm_area_args *);

   /*
    *  Memory display modes specific to this file.
 @@ -362,6 +385,10 @@ vm_init(void)

           MEMBER_OFFSET_INIT(task_struct_mm, "task_struct", "mm");
           MEMBER_OFFSET_INIT(mm_struct_mmap, "mm_struct", "mmap");
 +        MEMBER_OFFSET_INIT(mm_struct_mm_mt, "mm_struct", "mm_mt");
 +	if (VALID_MEMBER(mm_struct_mm_mt)) {
 +		maple_init();
 +	}
           MEMBER_OFFSET_INIT(mm_struct_pgd, "mm_struct", "pgd");
   	MEMBER_OFFSET_INIT(mm_struct_rss, "mm_struct", "rss");
   	if (!VALID_MEMBER(mm_struct_rss))
 @@ -3868,7 +3895,7 @@ bailout:
    *                        for references -- and only then does a display
    */

 -#define PRINT_VM_DATA()                                                  \
 +#define PRINT_VM_DATA(buf4, buf5, tm)                                    \
                   {                                                        \
                   fprintf(fp, "%s  %s  ",                                  \
                       mkstring(buf4, VADDR_PRLEN, CENTER|LJUST, "MM"),     \
 @@ -3890,9 +3917,9 @@ bailout:
                       mkstring(buf5, 8, CENTER|LJUST, NULL));              \
   	        }

 -#define PRINT_VMA_DATA()                                                       \
 +#define PRINT_VMA_DATA(buf1, buf2, buf3, buf4, vma)                            \
   	fprintf(fp, "%s%s%s%s%s %6llx%s%s\n",                                  \
 -                mkstring(buf4, VADDR_PRLEN, CENTER|LJUST|LONG_HEX, MKSTR(vma)),      
\
 +                mkstring(buf4, VADDR_PRLEN, CENTER|LJUST|LONG_HEX, MKSTR(vma)),\
   	        space(MINSPACE),                                               \
                   mkstring(buf2, UVADDR_PRLEN, RJUST|LONG_HEX, MKSTR(vm_start)), \
                   space(MINSPACE),                                               \
 @@ -3919,18 +3946,143 @@ bailout:
      (DO_REF_SEARCH(X) && (string_exists(S)) &&
FILENAME_COMPONENT((S),(X)->str))
   #define VM_REF_FOUND(X)    ((X) && ((X)->cmdflags & VM_REF_HEADER))

 -ulong
 -vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
 +static ulong handle_each_vm_area(struct handle_each_vm_area_args *args)
   {
 -        struct task_context *tc;
 -	ulong vma;
 +	char *dentry_buf, *file_buf;
   	ulong vm_start;
   	ulong vm_end;
 -	ulong vm_next, vm_mm;
 -	char *dentry_buf, *vma_buf, *file_buf;
 +	ulong vm_mm;
   	ulonglong vm_flags;
   	ulong vm_file, inode;
   	ulong dentry, vfsmnt;
 +
 +	if ((args->flag & PHYSADDR) && !DO_REF_SEARCH(args->ref))
 +		fprintf(fp, "%s", args->vma_header);
 +
 +	inode = 0;
 +	BZERO(args->buf1, BUFSIZE);
 +	*(args->vma_buf) = fill_vma_cache(args->vma);
 +
 +	vm_mm = ULONG(*(args->vma_buf) + OFFSET(vm_area_struct_vm_mm));
 +	vm_end = ULONG(*(args->vma_buf) + OFFSET(vm_area_struct_vm_end));
 +	vm_start = ULONG(*(args->vma_buf) + OFFSET(vm_area_struct_vm_start));
 +	vm_flags = get_vm_flags(*(args->vma_buf));
 +	vm_file = ULONG(*(args->vma_buf) + OFFSET(vm_area_struct_vm_file));
 +
 +	if (args->flag & PRINT_SINGLE_VMA) {
 +		if (args->vma != *(args->single_vma))
 +			return 0;
 +		fprintf(fp, "%s", args->vma_header);
 +		*(args->single_vma_found) = TRUE;
 +	}
 +
 +	if (args->flag & PRINT_VMA_STRUCTS) {
 +		dump_struct("vm_area_struct", args->vma, args->radix);
 +		return 0;
 +	}
 +
 +	if (vm_file && !(args->flag & VERIFY_ADDR)) {
 +		file_buf = fill_file_cache(vm_file);
 +		dentry = ULONG(file_buf + OFFSET(file_f_dentry));
 +		dentry_buf = NULL;
 +		if (dentry) {
 +			dentry_buf = fill_dentry_cache(dentry);
 +			if (VALID_MEMBER(file_f_vfsmnt)) {

...
 +				vfsmnt = ULONG(file_buf +
 +					OFFSET(file_f_vfsmnt));
 +				get_pathname(dentry, args->buf1, BUFSIZE,
 +					1, vfsmnt);
 +			} else {
 +				get_pathname(dentry, args->buf1, BUFSIZE,
 +					1, 0);
 +			}
 +		}
 +		if ((args->flag & PRINT_INODES) && dentry) {
 +			inode = ULONG(dentry_buf +
 +				OFFSET(dentry_d_inode));
 +		}

Let's join these wrapped lines and remove unnecessary {}s at this 
opportunity like this?

	if (VALID_MEMBER(file_f_vfsmnt)) {
		vfsmnt = ULONG(file_buf + OFFSET(file_f_vfsmnt));
		get_pathname(dentry, args->buf1, BUFSIZE, 1, vfsmnt);
	} else
		get_pathname(dentry, args->buf1, BUFSIZE, 1, 0);
...

...
 +	}
 +
 +	if (!(args->flag & UVADDR) || ((args->flag & UVADDR) &&
 +		((args->vaddr >= vm_start) && (args->vaddr < vm_end)))) {
          ^^^^^^^^
Please replace this tab with 4 spaces to distinguish this line and the 
following lines.

...
 +		*(args->found) = TRUE;
 +
 +		if (args->flag & VERIFY_ADDR)
 +			return args->vma;
 +
 +		if (DO_REF_SEARCH(args->ref)) {
 +			if (VM_REF_CHECK_HEXVAL(args->ref, args->vma) ||
 +				VM_REF_CHECK_HEXVAL(args->ref, (ulong)vm_flags) ||
 +				VM_REF_CHECK_STRING(args->ref, args->buf1)) {
                          ^^^^^^^^
Same as above.

...
 +				if (!(args->ref->cmdflags & VM_REF_HEADER)) {
 +					print_task_header(fp, args->tc, 0);
 +					PRINT_VM_DATA(args->buf4, args->buf5, args->tm);
 +					args->ref->cmdflags |= VM_REF_HEADER;
 +				}
 +				if (!(args->ref->cmdflags & VM_REF_VMA) ||
 +					(args->ref->cmdflags & VM_REF_PAGE)) {
                                  ^^^^^^^^
Same as above.

...
 +					fprintf(fp, "%s", args->vma_header);
 +					args->ref->cmdflags |= VM_REF_VMA;
 +					args->ref->cmdflags &= ~VM_REF_PAGE;
 +					args->ref->ref1 = args->vma;
 +				}
 +				PRINT_VMA_DATA(args->buf1, args->buf2,
 +						args->buf3, args->buf4, args->vma);
 +			}
 +
 +			if (vm_area_page_dump(args->vma, args->task,
 +				vm_start, vm_end, vm_mm, args->ref)) {
                          ^^^^^^^^
Please replace this tab with 3 tabs here (I prefer to put args close), 
or 4 spaces same as above.

...
 +				if (!(args->ref->cmdflags & VM_REF_HEADER)) {
 +					print_task_header(fp, args->tc, 0);
 +					PRINT_VM_DATA(args->buf4, args->buf5, args->tm);
 +					args->ref->cmdflags |= VM_REF_HEADER;
 +				}
 +				if (!(args->ref->cmdflags & VM_REF_VMA) ||
 +					(args->ref->ref1 != args->vma)) {
                                  ^^^^^^^^
Please replace this tab with 4 spaces.

...
 +					fprintf(fp, "%s", args->vma_header);
 +					PRINT_VMA_DATA(args->buf1, args->buf2,
 +							args->buf3, args->buf4, args->vma);
 +					args->ref->cmdflags |= VM_REF_VMA;
 +					args->ref->ref1 = args->vma;
 +				}
 +
 +				args->ref->cmdflags |= VM_REF_DISPLAY;
 +				vm_area_page_dump(args->vma, args->task,
 +					vm_start, vm_end, vm_mm, args->ref);
 +				args->ref->cmdflags &= ~VM_REF_DISPLAY;
 +			}
 +
 +			return 0;
 +		}
 +
 +		if (inode) {
 +			fprintf(fp, "%lx%s%s%s%s%s%6llx%s%lx %s\n",
 +				args->vma, space(MINSPACE),
 +				mkstring(args->buf2, UVADDR_PRLEN, RJUST|LONG_HEX,
 +				MKSTR(vm_start)), space(MINSPACE),
 +				mkstring(args->buf3, UVADDR_PRLEN, RJUST|LONG_HEX,
 +				MKSTR(vm_end)), space(MINSPACE),
 +				vm_flags, space(MINSPACE), inode, args->buf1);
 +		} else {
 +			PRINT_VMA_DATA(args->buf1, args->buf2,
 +					args->buf3, args->buf4, args->vma);
 +
 +			if (args->flag & (PHYSADDR|PRINT_SINGLE_VMA))
 +				vm_area_page_dump(args->vma, args->task,
 +					vm_start, vm_end, vm_mm, args->ref);
 +		}
 +
 +		if (args->flag & UVADDR)
 +			return args->vma;
 +	}
 +	return 0;
 +}
 +
 +ulong
 +vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference *ref)
 +{
 +	struct task_context *tc;
 +	ulong vma;
   	ulong single_vma;
   	unsigned int radix;
   	int single_vma_found;
 @@ -3942,6 +4094,10 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference
*ref)
   	char buf4[BUFSIZE];
   	char buf5[BUFSIZE];
   	char vma_header[BUFSIZE];
 +	char *vma_buf;
 +	int i;
 +	ulong mm_mt, entry_num;
 +	struct list_pair *entry_list;

           tc = task_to_context(task);
   	tm = &task_mem_usage;
 @@ -3975,14 +4131,14 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct
reference *ref)
           if (VM_REF_CHECK_HEXVAL(ref, tm->mm_struct_addr) ||
               VM_REF_CHECK_HEXVAL(ref, tm->pgd_addr)) {
           	print_task_header(fp, tc, 0);
 -		PRINT_VM_DATA();
 +		PRINT_VM_DATA(buf4, buf5, tm);
   		fprintf(fp, "\n");
                   return (ulong)NULL;
           }

           if (!(flag & (UVADDR|PRINT_MM_STRUCT|PRINT_VMA_STRUCTS|PRINT_SINGLE_VMA))
&&
   	    !DO_REF_SEARCH(ref))
 -		PRINT_VM_DATA();
 +		PRINT_VM_DATA(buf4, buf5, tm);

           if (!tm->mm_struct_addr) {
   		if (pc->curcmd_flags & MM_STRUCT_FORCE) {
 @@ -4006,9 +4162,6 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct reference
*ref)
                   return (ulong)NULL;
   	}

 -	readmem(tm->mm_struct_addr + OFFSET(mm_struct_mmap), KVADDR,
 -		&vma, sizeof(void *), "mm_struct mmap", FAULT_ON_ERROR);
 -
          	sprintf(vma_header, "%s%s%s%s%s  FLAGS%sFILE\n",
                   mkstring(buf1, VADDR_PRLEN, CENTER|LJUST, "VMA"),
                   space(MINSPACE),
 @@ -4021,125 +4174,41 @@ vm_area_dump(ulong task, ulong flag, ulong vaddr, struct
reference *ref)
   	    !DO_REF_SEARCH(ref))
   		fprintf(fp, "%s", vma_header);

 -	for (found = FALSE; vma; vma = vm_next) {
 -
 -		if ((flag & PHYSADDR) && !DO_REF_SEARCH(ref))
 -			fprintf(fp, "%s", vma_header);
 -
 -		inode = 0;
 -		BZERO(buf1, BUFSIZE);
 -		vma_buf = fill_vma_cache(vma);
 -
 -		vm_mm = ULONG(vma_buf + OFFSET(vm_area_struct_vm_mm));
 -		vm_end = ULONG(vma_buf + OFFSET(vm_area_struct_vm_end));
 -		vm_next = ULONG(vma_buf + OFFSET(vm_area_struct_vm_next));
 -		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, radix);
 -			continue;
 -		}
 +	found = FALSE;

 -		if (vm_file && !(flag & VERIFY_ADDR)) {
 -			file_buf = fill_file_cache(vm_file);
 -			dentry = ULONG(file_buf + OFFSET(file_f_dentry));
 -			dentry_buf = NULL;
 -			if (dentry) {
 -				dentry_buf = fill_dentry_cache(dentry);
 -				if (VALID_MEMBER(file_f_vfsmnt)) {
 -					vfsmnt = ULONG(file_buf +
 -                                		OFFSET(file_f_vfsmnt));
 -					get_pathname(dentry, buf1, BUFSIZE,
 -						1, vfsmnt);
 -				} else {
 -					get_pathname(dentry, buf1, BUFSIZE,
 -						1, 0);
 -				}
 -			}
 -			if ((flag & PRINT_INODES) && dentry) {
 -				inode = ULONG(dentry_buf +
 -					OFFSET(dentry_d_inode));
 +	struct handle_each_vm_area_args args = {
 +		.task = task,		.flag = flag,	.vaddr = vaddr,
 +		.ref = ref,		.tc = tc,	.radix = radix,
 +		.tm = tm,		.buf1 = buf1,	.buf2 = buf2,
 +		.buf3 = buf3,		.buf4 = buf4,	.buf5 = buf5,
 +		.vma_header = vma_header,		.single_vma = &single_vma,
 +		.single_vma_found = &single_vma_found,	.found = &found,
 +		.vma_buf = &vma_buf,
 +	};
 +
 +	if (INVALID_MEMBER(mm_struct_mmap) && VALID_MEMBER(mm_struct_mm_mt)) {
 +		mm_mt = tm->mm_struct_addr + OFFSET(mm_struct_mm_mt);
 +		entry_num = do_maple_tree(mm_mt, MAPLE_TREE_COUNT, NULL);
 +		entry_list = (struct list_pair *)GETBUF(entry_num * sizeof(struct list_pair));
 +		do_maple_tree(mm_mt, MAPLE_TREE_GATHER, entry_list);
 +
 +		for (i = 0; i < entry_num; i++) {
 +			if (!!(args.vma = (ulong)entry_list[i].value) &&
 +			    handle_each_vm_area(&args)) {

This is good :)

Thanks,
Kazu

> +				FREEBUF(entry_list);
> +				return args.vma;
>   			}
>   		}
> -
> -		if (!(flag & UVADDR) || ((flag & UVADDR) &&
> -		    ((vaddr >= vm_start) && (vaddr < vm_end)))) {
> -			found = TRUE;
> -
> -			if (flag & VERIFY_ADDR)
> -				return vma;
> -
> -			if (DO_REF_SEARCH(ref)) {
> -				if (VM_REF_CHECK_HEXVAL(ref, vma) ||
> -				    VM_REF_CHECK_HEXVAL(ref, (ulong)vm_flags) ||
> -				    VM_REF_CHECK_STRING(ref, buf1)) {
> -					if (!(ref->cmdflags & VM_REF_HEADER)) {
> -						print_task_header(fp, tc, 0);
> -                				PRINT_VM_DATA();
> -						ref->cmdflags |= VM_REF_HEADER;
> -					}
> -					if (!(ref->cmdflags & VM_REF_VMA) ||
> -					    (ref->cmdflags & VM_REF_PAGE)) {
> -						fprintf(fp, "%s", vma_header);
> -						ref->cmdflags |= VM_REF_VMA;
> -						ref->cmdflags &= ~VM_REF_PAGE;
> -						ref->ref1 = vma;
> -					}
> -					PRINT_VMA_DATA();
> -				}
> -
> -				if (vm_area_page_dump(vma, task,
> -				    vm_start, vm_end, vm_mm, ref)) {
> -					if (!(ref->cmdflags & VM_REF_HEADER)) {
> -					        print_task_header(fp, tc, 0);
> -                			        PRINT_VM_DATA();
> -						ref->cmdflags |= VM_REF_HEADER;
> -					}
> -                                        if (!(ref->cmdflags & VM_REF_VMA)
||
> -                                            (ref->ref1 != vma)) {
> -                                                fprintf(fp, "%s",
vma_header);
> -						PRINT_VMA_DATA();
> -                                                ref->cmdflags |= VM_REF_VMA;
> -                                                ref->ref1 = vma;
> -					}
> -
> -					ref->cmdflags |= VM_REF_DISPLAY;
> -					vm_area_page_dump(vma, task,
> -						vm_start, vm_end, vm_mm, ref);
> -					ref->cmdflags &= ~VM_REF_DISPLAY;
> -				}
> -
> -				continue;
> -			}
> -
> -			if (inode) {
> -                                fprintf(fp, "%lx%s%s%s%s%s%6llx%s%lx
%s\n",
> -                                    vma, space(MINSPACE),
> -                                    mkstring(buf2, UVADDR_PRLEN, RJUST|LONG_HEX,
> -                                        MKSTR(vm_start)), space(MINSPACE),
> -                                    mkstring(buf3, UVADDR_PRLEN, RJUST|LONG_HEX,
> -                                        MKSTR(vm_end)), space(MINSPACE),
> -				    vm_flags, space(MINSPACE), inode, buf1);
> -			} else {
> -				PRINT_VMA_DATA();
> -				
> -				if (flag & (PHYSADDR|PRINT_SINGLE_VMA))
> -					vm_area_page_dump(vma, task,
> -						vm_start, vm_end, vm_mm, ref);
> -			}
> -
> -			if (flag & UVADDR)
> +		FREEBUF(entry_list);
> +	} else {
> +		readmem(tm->mm_struct_addr + OFFSET(mm_struct_mmap), KVADDR,
> +			&vma, sizeof(void *), "mm_struct mmap", FAULT_ON_ERROR);
> +		while (vma) {
> +			args.vma = vma;
> +			if (handle_each_vm_area(&args))
>   				return vma;
> -		}
> +			vma = ULONG(vma_buf + OFFSET(vm_area_struct_vm_next));
> +		}
>   	}
>   
>   	if (flag & VERIFY_ADDR)

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

Re: [Crash-utility] [PATCH v4 4/6] Introduce maple tree vma iteration to memory.c