Messages in this thread |  | | From | "Christian Ehrhardt" <> | Date | Sat, 31 Aug 2002 18:14:48 +0200 | Subject | Re: [RFC] [PATCH] Include LRU in page count |
| |
On Sat, Aug 31, 2002 at 01:03:07AM +0200, Daniel Phillips wrote: > This rare race happened to become not so rare in 2.5 recently, and was > analyzed by Christian Ehrhardt, who also proposed a solution based on a new > approach to locking, essentially put_page_testone. We went on to check 2.4
Just a little correction: The key function implemented in my solution is an atomic GET_page_testone which is called if the page might have a zero refcount. The idea I had in mind is to distinguish heavy and weak references to pages. Your solution is probably the better way to go.
> I proposed an alternate solution using the traditional put_page_testzero > primitive, which relies on assigning a page count of one for membership on > the lru list. A slightly racy heuristic is used for efficient lru list > removal. The resulting incarnation of lru_cache_release is: > > static inline void page_cache_release(struct page *page) > { > if (page_count(page) == 2 && spin_trylock(&pagemap_lru_lock)) { > if (PageLRU(page) && page_count(page) == 2) > __lru_cache_del(page); > spin_unlock(&pagemap_lru_lock); > } > put_page(page); > }
Just saw that this can still race e.g. with lru_cache_add (not hard to fix though):
| void lru_cache_add(struct page * page) | { | if (!TestSetPageLRU(page)) {
Window is here: Once we set the PageLRU bit page_cache_release assumes that there is a reference held by the lru cache.
| spin_lock(&pagemap_lru_lock); | add_page_to_inactive_list(page); |#if LRU_PLUS_CACHE==2 | get_page(page); |#endif
But only starting at this point the reference actually exists.
| spin_unlock(&pagemap_lru_lock); | } |}
Solution: Change the PageLRU bit inside the lock. Looks like lru_cache_add is the only place that doesn't hold the lru lock to change the LRU flag and it's probably not a good idea even without the patch.
Two more comments: I don't think it is a good idea to use put_page_nofree in __lru_cache_del. This is probably safe now but it adds an additional rule that lru_cache_del can't be called without holding a second reference to the page. Also there may be lru only pages on the active list, i.e. refill inactive should have this hunk as well:
> +#if LRU_PLUS_CACHE==2 > + BUG_ON(!page_count(page)); > + if (unlikely(page_count(page) == 1)) { > + mmstat(vmscan_free_page); > + BUG_ON(!TestClearPageLRU(page)); // side effect abuse!! > + put_page(page); > + continue; > + } > +#endif
regards Christian Ehrhardt
-- 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/
|  |