[PATCH] makedumpfile: clean up readmem() by removing its recursive call

[Atsushi Kumagai]

Hello HATAYAMA-san,

On Thu, 21 Feb 2013 03:27:22 +0000
"Hatayama, Daisuke" <d.hatayama at jp.fujitsu.com> wrote:

> > From: kexec-bounces at lists.infradead.org
> > Sent: Wednesday, February 20, 2013 11:25 AM
> 
> > On Tue, 19 Feb 2013 05:05:39 +0000
> > "Hatayama, Daisuke" <d.hatayama at jp.fujitsu.com> wrote:
> > 
> > > Currently, readmem() calls itself recursively and its depth grows in
> > > proportin to the number of requested pages.
> > >
> > > For example, in __exclude_unnecessary_pages(), PGMM_CACHED is defined
> > > as 512 and page structure is of 56 bytes on x86_64, so size of page
> > > cache on x86_64 is now 56 * 512 = 28KiB, and this is 7 pages. This
> > > means that on the existing implementation readmem() is called
> > > recursively 7 times when reading mem_map array, but 6 of the 7 are
> > > in fact unnecessary.
> > >
> > > This patch cleans up readmem() by removing the recursive
> > > call. Instead, readmem() proceeds to the processing for next page by
> > > jump.
> > >
> > > Also, by this change, read operation is done in increasing order. This
> > > is more natural than the existing implementation in decreasing order.
> > >
> > > Signed-off-by: HATAYAMA Daisuke <d.hatayama at jp.fujitsu.com>
> > 
> > Thanks, I'll merge this patch into v1.5.4.
> 
> Sorry, I found I missed replacing size by size_orig at the last error path.
> 
> Could you use this new patch? There I also made some cleanup using MIN() and added performance topic a little in patch description.

Sure, I'll merge this one instead of old one.


Thanks
Atsushi Kumagai

> Thanks.
> HATAYAMA, Daisuke
> 
> From b5b603f465118f23d92f8c69e300281e6321ebe4 Mon Sep 17 00:00:00 2001
> From: HATAYAMA Daisuke <d.hatayama at jp.fujitsu.com>
> Date: Wed, 20 Feb 2013 20:13:07 +0900
> Subject: [PATCH] makedumpfile: clean up readmem() by removing its recursive
>  call
> 
> Currently, readmem() calls itself recursively and its depth grows in
> proportin to the number of requested pages.
> 
> For example, in __exclude_unnecessary_pages(), PGMM_CACHED is defined
> as 512 and page structure is of 56 bytes on x86_64, so size of page
> descriper cache on x86_64 is now 56 * 512 = 28KiB, and this is 7
> pages. This means that on the existing implementation, readmem() is
> called recursively 7 times when reading one entry of page descripter
> cache from mem_map array, but 6 of the 7 are in fact unnecessary.
> 
> This patch cleans up readmem() by removing the recursive
> call. Instead, readmem() proceeds to the processing for next page by
> jump.
> 
> By this change, read operation is done in increasing order. This is
> more natural than the existing implementation in decreasing order.
> 
> Also, there's visible performance gain. On 32GiB memory machine,
> repeating filtering 100-times on /proc/vmcore, average time for
> filtering on mem_map array was improved from 1.69 to 1.33 second:
> about 21.3 percent.
> 
> Signed-off-by: HATAYAMA Daisuke <d.hatayama at jp.fujitsu.com>
> ---
>  makedumpfile.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index acb1b21..14e8773 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -337,13 +337,12 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr)
>  int
>  readmem(int type_addr, unsigned long long addr, void *bufptr, size_t size)
>  {
> -	size_t read_size, next_size;
> -	unsigned long long next_addr;
> +	size_t read_size, size_orig = size;
>  	unsigned long long paddr, maddr = NOT_PADDR;
>  	unsigned long long pgaddr;
>  	void *pgbuf;
> -	char *next_ptr;
>  
> +next_page:
>  	switch (type_addr) {
>  	case VADDR:
>  		if ((paddr = vaddr_to_paddr(addr)) == NOT_PADDR) {
> @@ -386,21 +385,11 @@ readmem(int type_addr, unsigned long long addr, void *bufptr, size_t size)
>  		goto error;
>  	}
>  
> -	read_size = size;
> -
>  	/*
>  	 * Read each page, because pages are not necessarily continuous.
>  	 * Ex) pages in vmalloc area
>  	 */
> -	if (!is_in_same_page(addr, addr + size - 1)) {
> -		read_size = info->page_size - (addr % info->page_size);
> -		next_addr = roundup(addr + 1, info->page_size);
> -		next_size = size - read_size;
> -		next_ptr  = (char *)bufptr + read_size;
> -
> -		if (!readmem(type_addr, next_addr, next_ptr, next_size))
> -			goto error;
> -	}
> +	read_size = MIN(info->page_size - PAGEOFFSET(paddr), size);
>  
>  	pgaddr = PAGEBASE(paddr);
>  	pgbuf = cache_search(pgaddr);
> @@ -423,10 +412,18 @@ readmem(int type_addr, unsigned long long addr, void *bufptr, size_t size)
>  	}
>  
>  	memcpy(bufptr, pgbuf + PAGEOFFSET(paddr), read_size);
> -	return size;
> +
> +	addr += read_size;
> +	bufptr += read_size;
> +	size -= read_size;
> +
> +	if (size > 0)
> +		goto next_page;
> +
> +	return size_orig;
>  
>  error:
> -	ERRMSG("type_addr: %d, addr:%llx, size:%zd\n", type_addr, addr, size);
> +	ERRMSG("type_addr: %d, addr:%llx, size:%zd\n", type_addr, addr, size_orig);
>  	return FALSE;
>  }
>  
> -- 
> 1.8.1.2
>