Re: [PATCH] mm/filemap.c: unconditionally call mark_page_accessed

[Xiaoning Ding <dingxn@cse.ohio-state.edu>]

Dave Kleikamp wrote:
> On Wed, 2007-03-14 at 22:33 +0100, Andreas Mohr wrote:
>> Hi,
>>
>> On Wed, Mar 14, 2007 at 03:55:41PM -0500, Dave Kleikamp wrote:
>>> On Wed, 2007-03-14 at 15:58 -0400, Ashif Harji wrote:
>>>> This patch unconditionally calls mark_page_accessed to prevent pages, 
>>>> especially for small files, from being evicted from the page cache despite 
>>>> frequent access.
>>> I guess the downside to this is if a reader is reading a large file, or
>>> several files, sequentially with a small read size (smaller than
>>> PAGE_SIZE), the pages will be marked active after just one read pass.
>>> My gut says the benefits of this patch outweigh the cost.  I would
>>> expect real-world backup apps, etc. to read at least PAGE_SIZE.
>> I also think that the patch is somewhat problematic, since the original
>> intention seems to have been a reduction of the number of (expensive?)
>> mark_page_accessed() calls,
> 
> mark_page_accessed() isn't expensive.  If called repeatedly, starting
> with the third call, it will check two page flags and return.  The only
> real expense is that the page appears busier than it may be and will be
> retained in memory longer than it should.
> 
If we allow mark_page_accessed() called multiple times for a single page,
a scan of large file with small-size reads would flush the buffer cache.
mark_page_accessed() also requests lru_lock when moving page from
inactive_list to active_list. It may also increase lock contention.

>> but this of course falls flat on its face in case
>> of permanent single-page accesses or accesses with progressing but very small
>> read size (single-byte reads or so), since the cached page content will expire
>> eventually due to lack of mark_page_accessed() updates; thus this patch
>> decided to call mark_page_accessed() unconditionally which may be a large
>> performance penalty for subsequent tiny-sized reads.
> 
> Any application doing many tiny-sized reads isn't exactly asking for
> great performance.
> 
>> I've been thinking hard how to avoid the mark_page_accessed() starvation in
>> case of a fixed, (almost) non-changing access state, but this seems hard since
>> it'd seem we need some kind of state management here to figure out good
>> intervals of when to call mark_page_accessed() *again* for this page. E.g.
>> despite non-changing access patterns you could still call mark_page_accessed()
>> every 32 calls or so to avoid expiry, but this would need extra helper
>> variables.
>>
>> A rather ugly way to do it may be to abuse ra.cache_hit or ra.mmap_hit content
>> with a
>> 	if ((prev_index != index) || (ra.cache_hit % 32 == 0))
>> 		mark_page_accessed(page);
>> This assumes that ra.cache_hit gets incremented for every access (haven't
>> checked whether this is the case).
>> That way (combined with an enhanced comment properly explaining the dilemma)
>> you would avoid most mark_page_accessed() invocations of subsequent same-page reads
>> but still do page status updates from time to time to avoid page deprecation.
>>
>> Does anyone think this would be acceptable? Any better idea?
> 
> I wouldn't go looking for anything more complicated than Ashif's patch,
> unless testing shows it to be harmful in some realistic workload.
> 
>> Andreas Mohr
>>
>> P.S.: since I'm not too familiar with this area I could be rather wrong after all...
> 
> I could be missing something as well.  :-)
> 
> Shaggy