Re: [PATCH 3/3][RFC] swsusp: shrink file cache first

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

Hi

I have some comment.

> File cache pages are saved to disk either through normal writeback by
> reclaim or by including them in the suspend image written to a
> swapfile.
> 
> Writing them either way should take the same amount of time but doing
> normal writeback and unmap changes the fault behaviour on resume from
> prefault to on-demand paging, smoothening out resume and giving
> previously cached pages the chance to stay out of memory completely if
> they are not used anymore.
> 
> Another reason for preferring file page eviction is that the locality
> principle is visible in fault patterns and swap might perform really
> bad with subsequent faulting of contiguously mapped pages.
> 
> Since anon and file pages now live on different lists, selectively
> scanning one type only is straight-forward.

I don't understand your point.
Which do you want to improve suspend performance or resume performance?

if we think suspend performance, we should consider swap device and file-backed device
are different block device.
the interleave of file-backed page out and swap out can improve total write out performce.

if we think resume performance, we shold how think the on-disk contenious of the swap consist
process's virtual address contenious.
it cause to reduce unnecessary seek.
but your patch doesn't this.

Could you explain this patch benefit?
and, I think you should mesure performence result.


<snip>


> @@ -2134,17 +2144,17 @@ unsigned long shrink_all_memory(unsigned
>  
>  	/*
>  	 * We try to shrink LRUs in 5 passes:
> -	 * 0 = Reclaim from inactive_list only
> -	 * 1 = Reclaim from active list but don't reclaim mapped
> -	 * 2 = 2nd pass of type 1
> -	 * 3 = Reclaim mapped (normal reclaim)
> -	 * 4 = 2nd pass of type 3
> +	 * 0 = Reclaim unmapped inactive file pages
> +	 * 1 = Reclaim unmapped file pages

I think your patch reclaim mapped file at priority 0 and 1 too.


> +	 * 2 = Reclaim file and inactive anon pages
> +	 * 3 = Reclaim file and anon pages
> +	 * 4 = Second pass 3
>  	 */
>  	for (pass = 0; pass < 5; pass++) {
>  		int prio;
>  
> -		/* Force reclaiming mapped pages in the passes #3 and #4 */
> -		if (pass > 2)
> +		/* Reclaim mapped pages in higher passes */
> +		if (pass > 1)
>  			sc.may_swap = 1;

Why need this line?
If you reclaim only file backed lru, may_swap isn't effective.
So, Can't we just remove this line and always set may_swap=1 ?