Messages in this thread |  | | From | "Christian Ehrhardt" <> | Date | Wed, 28 Aug 2002 15:14:45 +0200 | Subject | Re: MM patches against 2.5.31 |
| |
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! - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |