Re: X-freeze after clflush changes [Was: 2.6.23-rc6-mm1]

["Dave Airlie" <airlied@gmail.com>]

On 9/20/07, Andi Kleen <andi@firstfloor.org> wrote:
> On Wed, Sep 19, 2007 at 12:10:17PM -0700, Andrew Morton wrote:
> > On Wed, 19 Sep 2007 16:59:04 +0200 Jiri Slaby <jirislaby@gmail.com> wrote:
> >
> > > ---------8<---------8<---------8<---------8<---------8<---------8<----
> > > That means
> > > void agp_generic_destroy_page(void *addr)
> > > {
> > >         struct page *page;
> > >
> > >         if (addr == NULL)
> > >                 return;
> > >
> > >         page = virt_to_page(addr);
> > > (1)     unmap_page_from_agp(page);
> > >         put_page(page);
> > > (2)     free_page((unsigned long)addr);
> > >         atomic_dec(&agp_bridge->current_memory_agp);
> > > }
> > >
> > > (1) unmap_page_from_agp -> change_page_attr -> change_page_attr_addr ->
> > > __change_page_attr -> save_page -> list_add(&fpage->lru, &deferred_pages);
> > > (2) free_page -> free_pages -> __free_pages -> free_hot_page ->
> > > free_hot_cold_page -> list_add(&page->lru, &pcp->list);
> >
> > that'll hurt.
> >
> > > any ideas how to fix this?
> >
> > We should hold a single reference on the page for its membership in
> > deferred_pages.
>
> The code is broken anyways. If you free pages without flushing
> them first some other innocent user allocating them will end up
> with possible uncached pages for some time.
>
> Does this simple patch help?
>
> -Andi
>
>
> Flush uncached AGP pages before freeing

In theory this should be handled by the caller, so as to avoid the
overhead of continuous flushing however I can see a potential race
condition here if the pages are put back into the kernel before the
caller flushes the mappings..

Do we need some sort of two step approach here? as flushing after each
page would be a major overhead for dynamic agp stuff in the new memory
manager..

Dave.

>
> Signed-off-by: Andi Kleen <ak@suse.de>
>
> Index: linux/drivers/char/agp/generic.c
> ===================================================================
> --- linux.orig/drivers/char/agp/generic.c
> +++ linux/drivers/char/agp/generic.c
> @@ -1185,6 +1185,7 @@ void agp_generic_destroy_page(void *addr
>
>         page = virt_to_page(addr);
>         unmap_page_from_agp(page);
> +       flush_agp_mappings();
>         put_page(page);
>         free_page((unsigned long)addr);
>         atomic_dec(&agp_bridge->current_memory_agp);
> -
> 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/
>