Re: MM patches against 2.5.31

Christian Ehrhardt
2002-08-28T13:14:45Z

On Tue, Aug 27, 2002 at 06:48:50PM +0200, Daniel Phillips wrote:
> On Monday 26 August 2002 22:58, Christian Ehrhardt wrote:
> > > Nope, still don't see it. Whoever hits put_page_testzero frees the page,
> > > secure in the knowlege that there are no other references to it.
> >
> > Well yes, but we cannot remove the page from the lru atomatically
> > at page_cache_release time if we follow your proposal. If you think we can,
> > show me your implementation of page_cache_release and I'll show
> > you where the races are (unless you do everything under the lru_lock
> > of course).
>
> void page_cache_release(struct page *page)
> {
> spin_lock(&pagemap_lru_lock);
> if (PageLRU(page) && page_count(page) == 2) {
> __lru_cache_del(page);
> atomic_dec(&page->count);
> }
> spin_unlock(&pagemap_lru_lock);
> if (put_page_testzero(page))
> __free_pages_ok(page, 0);
> }
>
> This allows the following benign race, with initial page count = 3:
> [ ...]
> Neither holder of a page reference sees the count at 2, and so the page
> is left on the lru with count = 1. This won't happen often and such
> pages will be recovered from the cold end of the list in due course.

Ok, agreed. I think this will work but taking the lru lock each time
is probably not a good idea.

> We could also do this:
>
> void page_cache_release(struct page *page)
> {
> if (page_count(page) == 2) {
> spin_lock(&pagemap_lru_lock);
> if (PageLRU(page) && page_count(page) == 2) {
> __lru_cache_del(page);
> atomic_dec(&page->count);
> }
> spin_unlock(&pagemap_lru_lock);
> }
> if (put_page_testzero(page))
> __free_pages_ok(page, 0);
> }
>
> Which avoids taking the lru lock sometimes in exchange for widening the
> hole through which pages can end up with count = 1 on the lru list.

This sounds like something that is worth trying. I missed that one.

Side note: The BUG in __pagevec_lru_del seems strange. refill_inactive
or shrink_cache could have removed the page from the lru before
__pagevec_lru_del acquired the lru lock.

regards Christian

--
THAT'S ALL FOLKS!