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

[Ashif Harji]

On Wed, 14 Mar 2007, Xiaoning Ding wrote:

> 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.

The problem with the existing logic is that it is too coarse.  In trying 
to deal with one usage pattern it is negatively impacting performance for 
other reasonable access patterns.

Further, consider the extreme case of scanning a file 1 byte at a time. 
In this case, you are going to access a page over 4000 times, but that 
page is not going to be marked as active and hence that page is likely to 
be evicted from the cache.  Clearly, there are cases when scanning a file 
that you would like the pages to be kept in the cache.

Finally, the existing code is problematic as there is no reasonable way to 
circumvent the negative impact for small files.

Hence, I think a change is necessary.  The question is whether the 
intent of conditionally calling mark_page_accessed() is still reasonable 
and whether the amount of bookkeeping required to detect that usage 
pattern but not create a problem for other usage patterns is reasonable.

I would tend to agree with David that:  "Any application doing many 
tiny-sized reads isn't exactly asking for great performance."  As well, 
applications concerned with performance and caching problems can read in a 
file in PAGE_SIZE chunks.  I still think the simple fix of removing the 
condition is the best approach, but I'm certainly open to alternatives.

ashif.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/