Re: [patch 2] mm: speculative get_page

Nick Piggin
2005-06-28T00:03:25Z

William Lee Irwin III wrote:
> 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.
>

Yep ie. a page that's freed or being freed.

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

It is also done by remove_exclusive_swap_page, although that hunk
leaked into a later patch (#5), sorry.

Other methods (eg truncate) don't seem to have an atomicity guarantee
anyway - ie. it is valid to pick up a reference on a page that is
just about to get truncated. PageFreeing is only used when some code
is making an assumption about the number of users of the page.

> be found by this. There's also likely trouble with higher-order pages.
>

There isn't because higher order pages aren't used for pagecache.

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

There are appropriate memory barriers: the radix tree is
modified uner the rwlock/spinlock, and this function has
a memory barrier before testing page != *pagep.

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

What's wrong with the lockless radix tree lookups?

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

You don't seem to have looked enough to conclude anything about it.

--
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com