possible bug in find_get_pages

It seems that page->_count == 0 at some point on some overnight runs with locks the system into a tight loop from the repeat: and a goto repeat in find_get_pages.

Quoted 3 lines · mark gross

It seems that page->_count == 0 at some point on some overnight runs with locks the system into a tight loop from the repeat: and a goto repeat in find_get_pages.

A page with ref count zero should not be in any mapping. If the page is in a mapping then the page is used. Therefore the refcount should be > 0.

If there is a page with zero refcount and its in a mapping then something erroneously decreased the refcount.

Nick wrote the code so I CCed him.

Still form a static read of the code that goto repeat raises eyebrows as why would anyone expect to get anything different from radix_page_deref_slot calling it again with the same arguments?

On Fri, 6 Mar 2009 11:26:25 -0800
mark gross <[email protected]> wrote:

I'm looking at a system hang (note: new hardware going under stress tests using a ubuntu 2.6.27-11-generic)

It seems that page->_count == 0 at some point on some overnight runs with locks the system into a tight loop from the repeat: and a goto repeat in find_get_pages.

Code inserted for convenience:

unsigned find_get_pages(struct address_space *mapping, pgoff_t start,
unsigned int nr_pages, struct page **pages)

{
	unsigned int i;
	unsigned int ret;
	unsigned int nr_found;

	rcu_read_lock();
restart:
	nr_found = radix_tree_gang_lookup_slot(&mapping->page_tree,
				(void ***)pages, start, nr_pages);
	ret = 0;
	for (i = 0; i < nr_found; i++) {
		struct page *page;
repeat:
		page = radix_tree_deref_slot((void **)pages[i]);
		if (unlikely(!page))
			continue;
		/*
		 * this can only trigger if nr_found == 1, making
		 * livelock
		 * a non issue.
		 */
		if (unlikely(page == RADIX_TREE_RETRY))
			goto restart;

		if (!page_cache_get_speculative(page))
			goto repeat; <---------_always_hits_ 

		/* Has the page moved? */
		if (unlikely(page != *((void **)pages[i]))) {
			page_cache_release(page);
			goto repeat;
		}

		pages[ret] = page;
		ret++;
	}
	rcu_read_unlock();
	return ret;
}

My question is that as I look at this code I don't see any way out of it once I get a page with zero _count from radix_tree_deref_slot, then I will get the same page forever. The input to radix_tree_deref_slot never changes so I assume the output should be the same crappy page with zero _count that drops me on the goto repeat line.

Is this a bug?

Also, is having a page->_count == 0 an unexpected or invalid state?

Thanks!

--mgross

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Nick already found and solved this problem .
It can help you.

http://patchwork.kernel.org/patch/860/

Quoted 73 lines

On Fri, 6 Mar 2009 11:26:25 -0800
mark gross <[email protected]> wrote:

I'm looking at a system hang (note: new hardware going under stress tests using a ubuntu 2.6.27-11-generic)

It seems that page->_count == 0 at some point on some overnight runs with locks the system into a tight loop from the repeat: and a goto repeat in find_get_pages.

Code inserted for convenience:

unsigned find_get_pages(struct address_space *mapping, pgoff_t start,
unsigned int nr_pages, struct page **pages)

{
	unsigned int i;
	unsigned int ret;
	unsigned int nr_found;

	rcu_read_lock();
restart:
	nr_found = radix_tree_gang_lookup_slot(&mapping->page_tree,
				(void ***)pages, start, nr_pages);
	ret = 0;
	for (i = 0; i < nr_found; i++) {
		struct page *page;
repeat:
		page = radix_tree_deref_slot((void **)pages[i]);
		if (unlikely(!page))
			continue;
		/*
		 * this can only trigger if nr_found == 1, making
		 * livelock
		 * a non issue.
		 */
		if (unlikely(page == RADIX_TREE_RETRY))
			goto restart;

		if (!page_cache_get_speculative(page))
			goto repeat; <---------_always_hits_ 

		/* Has the page moved? */
		if (unlikely(page != *((void **)pages[i]))) {
			page_cache_release(page);
			goto repeat;
		}

		pages[ret] = page;
		ret++;
	}
	rcu_read_unlock();
	return ret;
}

My question is that as I look at this code I don't see any way out of it once I get a page with zero _count from radix_tree_deref_slot, then I will get the same page forever. The input to radix_tree_deref_slot never changes so I assume the output should be the same crappy page with zero _count that drops me on the goto repeat line.

Is this a bug?

Also, is having a page->_count == 0 an unexpected or invalid state?

Thanks!

--mgross

Quoted 5 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 2 lines

Kinds Regards
Minchan Kim

Quoted 5 lines · Minchan Kim

Nick already found and solved this problem .
It can help you.

http://patchwork.kernel.org/patch/860/

Wow, this reads just like the problem we are seeing. I'll try the patch and let the test run for a few days!

We've even see it come out of the live lock once in a while as well. I was thinking cache coherency HW issue until this :)

I'll send an update after running the test.

Quoted 3 lines · mark gross

It seems that page->_count == 0 at some point on some overnight runs with locks the system into a tight loop from the repeat: and a goto repeat in find_get_pages.

A page with ref count zero should not be in any mapping. If the page is in a mapping then the page is used. Therefore the refcount should be > 0.

If there is a page with zero refcount and its in a mapping then something erroneously decreased the refcount.

Quoted 13 lines · mark gross

Quoted 5 lines · Minchan Kim

Nick already found and solved this problem .
It can help you.

http://patchwork.kernel.org/patch/860/

Wow, this reads just like the problem we are seeing. I'll try the patch and let the test run for a few days!

We've even see it come out of the live lock once in a while as well. I was thinking cache coherency HW issue until this :)

Quoted 1 line

I'll send an update after running the test.

Note that after some discussion, the accepted fix looks a bit different (and might potentially fix another problem if the compiler gets very smart, although gcc doesn't seem to).

Git commit e8c82c2e23e3527e0c9dc195e432c16784d270fa