linux-kernel - Re: [PATCH 05/11] readahead: replace ra->mmap_miss with ra->ra

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 8 Feb 2010 19:19:18 +1100
From:	Nick Piggin <npiggin@...e.de>
To:	Wu Fengguang <fengguang.wu@...el.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Jens Axboe <jens.axboe@...cle.com>,
	Andi Kleen <andi@...stfloor.org>,
	Steven Whitehouse <swhiteho@...hat.com>,
	Chris Mason <chris.mason@...cle.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Clemens Ladisch <clemens@...isch.de>,
	Olivier Galibert <galibert@...ox.com>,
	Linux Memory Management List <linux-mm@...ck.org>,
	linux-fsdevel@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 05/11] readahead: replace ra->mmap_miss with
 ra->ra_flags

On Sun, Feb 07, 2010 at 12:10:18PM +0800, Wu Fengguang wrote:
> Introduce a readahead flags field and embed the existing mmap_miss in it
> (to save space).

Is that the only reason? 
 
> It will be possible to lose the flags in race conditions, however the
> impact should be limited.

Is this really a good tradeoff? Randomly readahead behaviour can
change.

I never liked this mmap_miss counter, though. It doesn't seem like
it can adapt properly for changing mmap access patterns.

Is there any reason why the normal readahead algorithms can't
detect this kind of behaviour (in much fewer than 100 misses) and
also adapt much faster if the access changes?

> 
> CC: Nick Piggin <npiggin@...e.de>
> CC: Andi Kleen <andi@...stfloor.org>
> CC: Steven Whitehouse <swhiteho@...hat.com>
> Signed-off-by: Wu Fengguang <fengguang.wu@...el.com>
> ---
>  include/linux/fs.h |   30 +++++++++++++++++++++++++++++-
>  mm/filemap.c       |    7 ++-----
>  2 files changed, 31 insertions(+), 6 deletions(-)
> 
> --- linux.orig/include/linux/fs.h	2010-02-07 11:46:35.000000000 +0800
> +++ linux/include/linux/fs.h	2010-02-07 11:46:37.000000000 +0800
> @@ -892,10 +892,38 @@ struct file_ra_state {
>  					   there are only # of pages ahead */
>  
>  	unsigned int ra_pages;		/* Maximum readahead window */
> -	unsigned int mmap_miss;		/* Cache miss stat for mmap accesses */
> +	unsigned int ra_flags;
>  	loff_t prev_pos;		/* Cache last read() position */
>  };
>  
> +/* ra_flags bits */
> +#define	READAHEAD_MMAP_MISS	0x0000ffff /* cache misses for mmap access */
> +
> +/*
> + * Don't do ra_flags++ directly to avoid possible overflow:
> + * the ra fields can be accessed concurrently in a racy way.
> + */
> +static inline unsigned int ra_mmap_miss_inc(struct file_ra_state *ra)
> +{
> +	unsigned int miss = ra->ra_flags & READAHEAD_MMAP_MISS;
> +
> +	if (miss < READAHEAD_MMAP_MISS) {
> +		miss++;
> +		ra->ra_flags = miss | (ra->ra_flags &~ READAHEAD_MMAP_MISS);
> +	}
> +	return miss;
> +}
> +
> +static inline void ra_mmap_miss_dec(struct file_ra_state *ra)
> +{
> +	unsigned int miss = ra->ra_flags & READAHEAD_MMAP_MISS;
> +
> +	if (miss) {
> +		miss--;
> +		ra->ra_flags = miss | (ra->ra_flags &~ READAHEAD_MMAP_MISS);
> +	}
> +}
> +
>  /*
>   * Check if @index falls in the readahead windows.
>   */
> --- linux.orig/mm/filemap.c	2010-02-07 11:46:35.000000000 +0800
> +++ linux/mm/filemap.c	2010-02-07 11:46:37.000000000 +0800
> @@ -1418,14 +1418,12 @@ static void do_sync_mmap_readahead(struc
>  		return;
>  	}
>  
> -	if (ra->mmap_miss < INT_MAX)
> -		ra->mmap_miss++;
>  
>  	/*
>  	 * Do we miss much more than hit in this file? If so,
>  	 * stop bothering with read-ahead. It will only hurt.
>  	 */
> -	if (ra->mmap_miss > MMAP_LOTSAMISS)
> +	if (ra_mmap_miss_inc(ra) > MMAP_LOTSAMISS)
>  		return;
>  
>  	/*
> @@ -1455,8 +1453,7 @@ static void do_async_mmap_readahead(stru
>  	/* If we don't want any read-ahead, don't bother */
>  	if (VM_RandomReadHint(vma))
>  		return;
> -	if (ra->mmap_miss > 0)
> -		ra->mmap_miss--;
> +	ra_mmap_miss_dec(ra);
>  	if (PageReadahead(page))
>  		page_cache_async_readahead(mapping, ra, file,
>  					   page, offset, ra->ra_pages);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/