Re: [patch 2] mm: speculative get_page

William Lee Irwin III
2005-06-27T14:12:20Z

On Mon, Jun 27, 2005 at 04:32:38PM +1000, Nick Piggin wrote:
> +static inline struct page *page_cache_get_speculative(struct page **pagep)
> +{
> + struct page *page;
> +
> + preempt_disable();
> + page = *pagep;
> + if (!page)
> + goto out_failed;
> +
> + if (unlikely(get_page_testone(page))) {
> + /* Picked up a freed page */
> + __put_page(page);
> + goto out_failed;
> + }

So you pick up 0->1 refcount transitions.

On Mon, Jun 27, 2005 at 04:32:38PM +1000, Nick Piggin wrote:
> + /*
> + * preempt can really be enabled here (only needs to be disabled
> + * because page allocation can spin on the elevated refcount, but
> + * we don't want to hold a reference on an unrelated page for too
> + * long, so keep preempt off until we know we have the right page
> + */
> +
> + if (unlikely(PageFreeing(page)) ||

SetPageFreeing is only done in shrink_list(), so other pages in the
buddy bitmaps and/or pagecache pages freed by other methods may not
be found by this. There's also likely trouble with higher-order pages.

On Mon, Jun 27, 2005 at 04:32:38PM +1000, Nick Piggin wrote:
> + unlikely(page != *pagep)) {
> + /* Picked up a page being freed, or one that's been reused */
> + put_page(page);
> + goto out_failed;
> + }
> + preempt_enable();
> +
> + return page;
> +
> +out_failed:
> + preempt_enable();
> + return NULL;
> +}

page != *pagep won't be reliably tripped unless the pagecache
modification has the appropriate memory barriers.

The lockless radix tree lookups are a harder problem than this, and
the implementation didn't look promising. I have other problems to deal
with so I'm not going to go very far into this.

While I agree that locklessness is the right direction for the
pagecache to go, this RFC seems to have too far to go to use it to
conclude anything about the subject.

-- wli