Re: [RFC][PATCH 1/3] radix priority search tree - objrmap complexity fix

Andrea Arcangeli
2004-04-03T17:40:44Z

JFYI, swap suspend now works fine with rc3-aa3 that includes the below
and shutdown the harmless warnings generated by the first
gfp-no-compound patch. This also means the -mm writeback fixes doing
the additional page_cache_get to keep the remove_exclusive_swap_cache
away, worked fine too and you should merge that fix in -mm.

I believe Christoph's oops on ppc, is either the missign tlb flushing
support (fixed by Hugh in the last patch I posted to the list) or some
other ppc issue and it's the last pending known bug (xfs also has a bug
in truncate exposed by a bugcheck in objrmap, but it's not a vm issue
and its fix should be almost ready too).

I'm very convinced that the alloc_pages API should be the same for all
archs w/o or w/ MMU, and I'm fine if we want to make the non-compound
retval the default (and change __GFP_NO_COMP to __GFP_COMP) in the long
run (to optimize all callers but hugetlbfs). For the short term
__GFP_NO_COMP and compound being the default is the safest (for all
archs).

On Fri, Apr 02, 2004 at 06:46:34PM +0200, Andrea Arcangeli wrote:
> On Fri, Apr 02, 2004 at 10:43:34AM +0100, Christoph Hellwig wrote:
> > I got lots of the following OOPSEs with 2.6.5-rc3aa2 on a powerpc running
> > the xfs testsuite (with the truncate fix applied):
> >
> > Apr 2 13:27:21 bird kernel: Bad page state at destroy_compound_page (in process 'swapper', page c08d9920)
> > Apr 2 13:27:21 bird kernel: flags:0x00000008 mapping:00000000 mapped:0 count:0
> > Apr 2 13:27:21 bird kernel: Backtrace:
> > Apr 2 13:27:21 bird kernel: Call trace:
> > Apr 2 13:27:21 bird kernel: [c000b5c8] dump_stack+0x18/0x28
> > Apr 2 13:27:21 bird kernel: [c0048b60] bad_page+0x70/0xb0
> > Apr 2 13:27:21 bird kernel: [c0048c70] destroy_compound_page+0x80/0xb8
>
> it's not clear why this triggered, bad_page only shows the "master"
> compound page and not the contents of the slave page that triggered the
> bad_page. Can you try again with this incremental patch applied?
> Thanks!
>
> --- x/mm/page_alloc.c.~1~ 2004-04-02 05:24:50.000000000 +0200
> +++ x/mm/page_alloc.c 2004-04-02 18:32:53.189244408 +0200
> @@ -73,9 +73,9 @@ static void bad_page(const char *functio
> {
> printk(KERN_EMERG "Bad page state at %s (in process '%s', page %p)\n",
> function, current->comm, page);
> - printk(KERN_EMERG "flags:0x%08lx mapping:%p mapped:%d count:%d\n",
> + printk(KERN_EMERG "flags:0x%08lx mapping:%p mapped:%d count:%d private:0x%08lx\n",
> (unsigned long)page->flags, page->mapping,
> - page_mapped(page), page_count(page));
> + page_mapped(page), page_count(page), page->private);
> printk(KERN_EMERG "Backtrace:\n");
> dump_stack();
> printk(KERN_EMERG "Trying to fix it up, but a reboot is needed\n");
> @@ -137,9 +137,9 @@ static void destroy_compound_page(struct
> struct page *p = page + i;
>
> if (!PageCompound(p))
> - bad_page(__FUNCTION__, page);
> + bad_page(__FUNCTION__, p);
> if (p->private != (unsigned long)page)
> - bad_page(__FUNCTION__, page);
> + bad_page(__FUNCTION__, p);
> ClearPageCompound(p);
> }
> }
> @@ -272,8 +272,12 @@ void __free_pages_ok(struct page *page,
> int i;
>
> mod_page_state(pgfree, 1 << order);
> - for (i = 0 ; i < (1 << order) ; ++i)
> - free_pages_check(__FUNCTION__, page + i);
> + for (i = 0 ; i < (1 << order) ; ++i) {
> + struct page * _page = page + i;
> + if (unlikely(i))
> + __put_page(_page);
> + free_pages_check(__FUNCTION__, _page);
> + }
> list_add(&page->lru, &list);
> kernel_map_pages(page, 1<<order, 0);
> free_pages_bulk(page_zone(page), 1, &list, order);
> @@ -316,19 +320,21 @@ static void prep_new_page(struct page *
> (page->flags & (
> 1 << PG_private |
> 1 << PG_locked |
> - 1 << PG_lru |
> + 1 << PG_lru |
> 1 << PG_active |
> 1 << PG_dirty |
> 1 << PG_reclaim |
> 1 << PG_anon |
> 1 << PG_maplock |
> 1 << PG_swapcache |
> - 1 << PG_writeback )))
> + 1 << PG_writeback |
> + 1 << PG_compound )))
> bad_page(__FUNCTION__, page);
>
> page->flags &= ~(1 << PG_uptodate | 1 << PG_error |
> 1 << PG_referenced | 1 << PG_arch_1 |
> - 1 << PG_checked | 1 << PG_mappedtodisk);
> + 1 << PG_checked | 1 << PG_mappedtodisk |
> + 1 << PG_compound);
> page->private = 0;
> set_page_count(page, 1);
> }
>
>
> this incrmental bit made some harmless warning go away from swap resume,
> but it didn't fix swap resume completely yet OTOH I'm not sure anymore
> if there's any further VM issue or if it's a swap suspend issue. the
> PageCompound bugcheck would already trap any compound page in
> rw_swap_page_sync, so I'm sure nobody tried to swap compound pages in
> swap resume, and I'm also sure that the page->count is now correct, or
> free_pages_check would trigger. I cannot trigger any further bugcheck
> here (and the above patch only shutdown some false positive that
> couldn't hurt functionality, plus it adds further bugchecks).