[PATCH] makedumpfile: reduce unnecessary memory copy

[Atsushi Kumagai]

Hello Jingbai,

>> This patch intends to reduce unnecessary memory copy in compressing memory
>> blocks.
>>
>> old logic like this:
>>     compress(buf_out, &size_out, buf, size);
>>     ...
>>     memcpy(buf, buf_out, pd.size);
>>     ...
>>     write_cache(cd_page, buf, pd.size)
>>
>> new logic:
>>     compress(buf_out, &size_out, buf, size);
>>     ...
>>     if (compressed?)
>>         write_cache(cd_page, buf_out, pd.size)
>>     else
>>         write_cache(cd_page, buf, pd.size)
>>
>> This occurs for each single page, so by the new logic it can reduce a lot of
>> unnecessary memcpy() on machines with huge memory.
>> This patch wasn't expected to improve the kdump performance dramatically due to
>> the memory copy operations on modern processors are pretty fast.
>>
>
>Now /proc/vmcore has zero copy mechanism with mmap(), but now
>makedumpfile still uses buffers some places in makedumpfile to keep
>changes as less as possible for ease of maintainance. I think ease of
>maintainance is important, so I don't think we should soon totally
>remove copy in makedumpfile. We should first benchmark where can be
>improved very much by just removing copy and then should decide where
>to optimize.
>
>OTOH, this patch also seems to be a kind of clean-up patch. Even if
>amount of improvement is a little, I don't object this patch; of
>course, then patch description should say it's rather cleanup patch
>than optimization.

I have no additional comments, so please send the v2 patch.


Thanks
Atsushi Kumagai

>>
>> Signed-off-by: Jingbai Ma <jingbai.ma at hp.com>
>> ---
>>  makedumpfile.c |   24 ++++++++++++++----------
>>  1 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/makedumpfile.c b/makedumpfile.c
>> index 23251a1..80f76cf 100644
>> --- a/makedumpfile.c
>> +++ b/makedumpfile.c
>> @@ -6245,7 +6245,6 @@ write_kdump_pages(struct cache_data *cd_header, struct cache_data *cd_page)
>>  		    && (size_out < info->page_size)) {
>>  			pd.flags = DUMP_DH_COMPRESSED_ZLIB;
>>  			pd.size  = size_out;
>> -			memcpy(buf, buf_out, pd.size);
>>  #ifdef USELZO
>>  		} else if (info->flag_lzo_support
>>  			   && (info->flag_compress & DUMP_DH_COMPRESSED_LZO)
>> @@ -6255,7 +6254,6 @@ write_kdump_pages(struct cache_data *cd_header, struct cache_data *cd_page)
>>  			   && (size_out < info->page_size)) {
>>  			pd.flags = DUMP_DH_COMPRESSED_LZO;
>>  			pd.size  = size_out;
>> -			memcpy(buf, buf_out, pd.size);
>>  #endif
>>  #ifdef USESNAPPY
>>  		} else if ((info->flag_compress & DUMP_DH_COMPRESSED_SNAPPY)
>> @@ -6267,7 +6265,6 @@ write_kdump_pages(struct cache_data *cd_header, struct cache_data *cd_page)
>>  			   && (size_out < info->page_size)) {
>>  			pd.flags = DUMP_DH_COMPRESSED_SNAPPY;
>>  			pd.size  = size_out;
>> -			memcpy(buf, buf_out, pd.size);
>>  #endif
>>  		} else {
>>  			pd.flags = 0;
>> @@ -6286,8 +6283,13 @@ write_kdump_pages(struct cache_data *cd_header, struct cache_data *cd_page)
>>  		/*
>>  		 * Write the page data.
>>  		 */
>> -		if (!write_cache(cd_page, buf, pd.size))
>> -			goto out;
>> +		if (pd.flags == 0) {
>> +			if (!write_cache(cd_page, buf, pd.size))
>> +				goto out;
>> +		} else {
>> +			if (!write_cache(cd_page, buf_out, pd.size))
>> +				goto out;
>> +		}
>>  	}
>
>if (!write_cache(cd_page, pd.flags ? buf_out : buf, pd.size))
>    goto out;
>
>>
>>  	/*
>> @@ -6424,7 +6426,6 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
>>  		    && (size_out < info->page_size)) {
>>  			pd.flags = DUMP_DH_COMPRESSED_ZLIB;
>>  			pd.size  = size_out;
>> -			memcpy(buf, buf_out, pd.size);
>>  #ifdef USELZO
>>  		} else if (info->flag_lzo_support
>>  			   && (info->flag_compress & DUMP_DH_COMPRESSED_LZO)
>> @@ -6434,7 +6435,6 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
>>  			   && (size_out < info->page_size)) {
>>  			pd.flags = DUMP_DH_COMPRESSED_LZO;
>>  			pd.size  = size_out;
>> -			memcpy(buf, buf_out, pd.size);
>>  #endif
>>  #ifdef USESNAPPY
>>  		} else if ((info->flag_compress & DUMP_DH_COMPRESSED_SNAPPY)
>> @@ -6446,7 +6446,6 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
>>  			   && (size_out < info->page_size)) {
>>  			pd.flags = DUMP_DH_COMPRESSED_SNAPPY;
>>  			pd.size  = size_out;
>> -			memcpy(buf, buf_out, pd.size);
>>  #endif
>>  		} else {
>>  			pd.flags = 0;
>> @@ -6465,8 +6464,13 @@ write_kdump_pages_cyclic(struct cache_data *cd_header, struct cache_data *cd_pag
>>                  /*
>>                   * Write the page data.
>>                   */
>> -                if (!write_cache(cd_page, buf, pd.size))
>> -                        goto out;
>> +		if (pd.flags == 0) {
>> +			if (!write_cache(cd_page, buf, pd.size))
>> +				goto out;
>> +		} else {
>> +			if (!write_cache(cd_page, buf_out, pd.size))
>> +				goto out;
>> +		}
>>          }
>
>Similaly.
>
>>
>>  	ret = TRUE;
>>
>
>Thanks.
>HATAYAMA, Daisuke