[patch] agp: don't lock pages

[patch] agp: don't lock pages

[Nick Piggin]

Hi,

Does this patch solve the X problem? Does anyone see anything wrong
with it or know why agp was locking the pages?

--
AGP should not need to lock pages. They are not protecting any race
because there is no lock_page calls, only SetPageLocked.

This is causing hangs with d00806b183152af6d24f46f0c33f14162ca1262a.

Signed-off-by: Nick Piggin <npiggin@suse.de>

diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
index d535c40..3db4f40 100644
--- a/drivers/char/agp/generic.c
+++ b/drivers/char/agp/generic.c
@@ -1170,7 +1170,6 @@ void *agp_generic_alloc_page(struct agp_
 	map_page_into_agp(page);
 
 	get_page(page);
-	SetPageLocked(page);
 	atomic_inc(&agp_bridge->current_memory_agp);
 	return page_address(page);
 }
@@ -1187,7 +1186,6 @@ void agp_generic_destroy_page(void *addr
 	page = virt_to_page(addr);
 	unmap_page_from_agp(page);
 	put_page(page);
-	unlock_page(page);
 	free_page((unsigned long)addr);
 	atomic_dec(&agp_bridge->current_memory_agp);
 }
diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index a124060..2f319f4 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -213,7 +213,6 @@ static void *i8xx_alloc_pages(void)
 	}
 	global_flush_tlb();
 	get_page(page);
-	SetPageLocked(page);
 	atomic_inc(&agp_bridge->current_memory_agp);
 	return page_address(page);
 }
@@ -229,7 +228,6 @@ static void i8xx_destroy_pages(void *add
 	change_page_attr(page, 4, PAGE_KERNEL);
 	global_flush_tlb();
 	put_page(page);
-	unlock_page(page);
 	__free_pages(page, 2);
 	atomic_dec(&agp_bridge->current_memory_agp);
 }
diff --git a/drivers/char/agp/sgi-agp.c b/drivers/char/agp/sgi-agp.c
index cda608c..98cf8ab 100644
--- a/drivers/char/agp/sgi-agp.c
+++ b/drivers/char/agp/sgi-agp.c
@@ -51,7 +51,6 @@ static void *sgi_tioca_alloc_page(struct
 		return NULL;
 
 	get_page(page);
-	SetPageLocked(page);
 	atomic_inc(&agp_bridge->current_memory_agp);
 	return page_address(page);
 }

Re: [patch] agp: don't lock pages

[Bret Towe]

On 7/25/07, Nick Piggin <npiggin@suse.de> wrote:
> Hi,
>
> Does this patch solve the X problem? Does anyone see anything wrong
> with it or know why agp was locking the pages?

Yeap the mac mini is happy again
Thanks

> --
> AGP should not need to lock pages. They are not protecting any race
> because there is no lock_page calls, only SetPageLocked.
>
> This is causing hangs with d00806b183152af6d24f46f0c33f14162ca1262a.
>
> Signed-off-by: Nick Piggin <npiggin@suse.de>
>
> diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
> index d535c40..3db4f40 100644
> --- a/drivers/char/agp/generic.c
> +++ b/drivers/char/agp/generic.c
> @@ -1170,7 +1170,6 @@ void *agp_generic_alloc_page(struct agp_
>         map_page_into_agp(page);
>
>         get_page(page);
> -       SetPageLocked(page);
>         atomic_inc(&agp_bridge->current_memory_agp);
>         return page_address(page);
>  }
> @@ -1187,7 +1186,6 @@ void agp_generic_destroy_page(void *addr
>         page = virt_to_page(addr);
>         unmap_page_from_agp(page);
>         put_page(page);
> -       unlock_page(page);
>         free_page((unsigned long)addr);
>         atomic_dec(&agp_bridge->current_memory_agp);
>  }
> diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
> index a124060..2f319f4 100644
> --- a/drivers/char/agp/intel-agp.c
> +++ b/drivers/char/agp/intel-agp.c
> @@ -213,7 +213,6 @@ static void *i8xx_alloc_pages(void)
>         }
>         global_flush_tlb();
>         get_page(page);
> -       SetPageLocked(page);
>         atomic_inc(&agp_bridge->current_memory_agp);
>         return page_address(page);
>  }
> @@ -229,7 +228,6 @@ static void i8xx_destroy_pages(void *add
>         change_page_attr(page, 4, PAGE_KERNEL);
>         global_flush_tlb();
>         put_page(page);
> -       unlock_page(page);
>         __free_pages(page, 2);
>         atomic_dec(&agp_bridge->current_memory_agp);
>  }
> diff --git a/drivers/char/agp/sgi-agp.c b/drivers/char/agp/sgi-agp.c
> index cda608c..98cf8ab 100644
> --- a/drivers/char/agp/sgi-agp.c
> +++ b/drivers/char/agp/sgi-agp.c
> @@ -51,7 +51,6 @@ static void *sgi_tioca_alloc_page(struct
>                 return NULL;
>
>         get_page(page);
> -       SetPageLocked(page);
>         atomic_inc(&agp_bridge->current_memory_agp);
>         return page_address(page);
>  }
> -
> 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/
>

Re: [patch] agp: don't lock pages

[Benjamin Herrenschmidt]

On Wed, 2007-07-25 at 13:19 +0200, Nick Piggin wrote:
> Hi,
> 
> Does this patch solve the X problem? Does anyone see anything wrong
> with it or know why agp was locking the pages?

We need to do a little bit of auditing here, but I suspect it will turn
out all right. I think the reason it locked them in the first place was
to avoid AGP pages mapped into process space from being swapped out.

I think that should be taken care of by appropriate vma flags nowadays,
but we need to double check. It also might have been a way around dodgy
refcounting at one point but I think we got that right nowadays (I
remember fixing issues in that area when we removed PageReserved from
those pages back then).

Ben.

> --
> AGP should not need to lock pages. They are not protecting any race
> because there is no lock_page calls, only SetPageLocked.
> 
> This is causing hangs with d00806b183152af6d24f46f0c33f14162ca1262a.
> 
> Signed-off-by: Nick Piggin <npiggin@suse.de>
> 
> diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
> index d535c40..3db4f40 100644
> --- a/drivers/char/agp/generic.c
> +++ b/drivers/char/agp/generic.c
> @@ -1170,7 +1170,6 @@ void *agp_generic_alloc_page(struct agp_
>  	map_page_into_agp(page);
>  
>  	get_page(page);
> -	SetPageLocked(page);
>  	atomic_inc(&agp_bridge->current_memory_agp);
>  	return page_address(page);
>  }
> @@ -1187,7 +1186,6 @@ void agp_generic_destroy_page(void *addr
>  	page = virt_to_page(addr);
>  	unmap_page_from_agp(page);
>  	put_page(page);
> -	unlock_page(page);
>  	free_page((unsigned long)addr);
>  	atomic_dec(&agp_bridge->current_memory_agp);
>  }
> diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
> index a124060..2f319f4 100644
> --- a/drivers/char/agp/intel-agp.c
> +++ b/drivers/char/agp/intel-agp.c
> @@ -213,7 +213,6 @@ static void *i8xx_alloc_pages(void)
>  	}
>  	global_flush_tlb();
>  	get_page(page);
> -	SetPageLocked(page);
>  	atomic_inc(&agp_bridge->current_memory_agp);
>  	return page_address(page);
>  }
> @@ -229,7 +228,6 @@ static void i8xx_destroy_pages(void *add
>  	change_page_attr(page, 4, PAGE_KERNEL);
>  	global_flush_tlb();
>  	put_page(page);
> -	unlock_page(page);
>  	__free_pages(page, 2);
>  	atomic_dec(&agp_bridge->current_memory_agp);
>  }
> diff --git a/drivers/char/agp/sgi-agp.c b/drivers/char/agp/sgi-agp.c
> index cda608c..98cf8ab 100644
> --- a/drivers/char/agp/sgi-agp.c
> +++ b/drivers/char/agp/sgi-agp.c
> @@ -51,7 +51,6 @@ static void *sgi_tioca_alloc_page(struct
>  		return NULL;
>  
>  	get_page(page);
> -	SetPageLocked(page);
>  	atomic_inc(&agp_bridge->current_memory_agp);
>  	return page_address(page);
>  }

Re: [patch] agp: don't lock pages

[Nick Piggin]

[forgot to cc Dave Jones...]


On Thu, Jul 26, 2007 at 07:26:53AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2007-07-25 at 13:19 +0200, Nick Piggin wrote:
> > Hi,
> > 
> > Does this patch solve the X problem? Does anyone see anything wrong
> > with it or know why agp was locking the pages?
> 
> We need to do a little bit of auditing here, but I suspect it will turn
> out all right. I think the reason it locked them in the first place was
> to avoid AGP pages mapped into process space from being swapped out.
> 
> I think that should be taken care of by appropriate vma flags nowadays,
> but we need to double check. It also might have been a way around dodgy
> refcounting at one point but I think we got that right nowadays (I
> remember fixing issues in that area when we removed PageReserved from
> those pages back then).

Yeah I had a bit of a look around, and it seems OK (but would
appreciate an ack from someone who knows the code).

These pages will never get seen by page reclaim, so we're OK
there. There is a get_page before the SetPageLocked and a put_page
right before the unlock_page, so refcounting should not be broken
if it wasn't already: note that the lock_page doesn't pin a
reference on a page in general -- we can use it as such for pagecache
(although it isn't very clean), because the lock pins the page in
pagecache and the pagecache holds a ref.

Anyway, if Dave or David can take a look, that would be appreciated.
We'll need this for 2.6.23.

Nick

> 
> Ben.
> 
> > --
> > AGP should not need to lock pages. They are not protecting any race
> > because there is no lock_page calls, only SetPageLocked.
> > 
> > This is causing hangs with d00806b183152af6d24f46f0c33f14162ca1262a.
> > 
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > 
> > diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
> > index d535c40..3db4f40 100644
> > --- a/drivers/char/agp/generic.c
> > +++ b/drivers/char/agp/generic.c
> > @@ -1170,7 +1170,6 @@ void *agp_generic_alloc_page(struct agp_
> >  	map_page_into_agp(page);
> >  
> >  	get_page(page);
> > -	SetPageLocked(page);
> >  	atomic_inc(&agp_bridge->current_memory_agp);
> >  	return page_address(page);
> >  }
> > @@ -1187,7 +1186,6 @@ void agp_generic_destroy_page(void *addr
> >  	page = virt_to_page(addr);
> >  	unmap_page_from_agp(page);
> >  	put_page(page);
> > -	unlock_page(page);
> >  	free_page((unsigned long)addr);
> >  	atomic_dec(&agp_bridge->current_memory_agp);
> >  }
> > diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
> > index a124060..2f319f4 100644
> > --- a/drivers/char/agp/intel-agp.c
> > +++ b/drivers/char/agp/intel-agp.c
> > @@ -213,7 +213,6 @@ static void *i8xx_alloc_pages(void)
> >  	}
> >  	global_flush_tlb();
> >  	get_page(page);
> > -	SetPageLocked(page);
> >  	atomic_inc(&agp_bridge->current_memory_agp);
> >  	return page_address(page);
> >  }
> > @@ -229,7 +228,6 @@ static void i8xx_destroy_pages(void *add
> >  	change_page_attr(page, 4, PAGE_KERNEL);
> >  	global_flush_tlb();
> >  	put_page(page);
> > -	unlock_page(page);
> >  	__free_pages(page, 2);
> >  	atomic_dec(&agp_bridge->current_memory_agp);
> >  }
> > diff --git a/drivers/char/agp/sgi-agp.c b/drivers/char/agp/sgi-agp.c
> > index cda608c..98cf8ab 100644
> > --- a/drivers/char/agp/sgi-agp.c
> > +++ b/drivers/char/agp/sgi-agp.c
> > @@ -51,7 +51,6 @@ static void *sgi_tioca_alloc_page(struct
> >  		return NULL;
> >  
> >  	get_page(page);
> > -	SetPageLocked(page);
> >  	atomic_inc(&agp_bridge->current_memory_agp);
> >  	return page_address(page);
> >  }

Re: [patch] agp: don't lock pages

[Nick Piggin]

[one more try]

On Thu, Jul 26, 2007 at 02:41:14AM +0200, Nick Piggin wrote:
> [forgot to cc Dave Jones...]
> 
> 
> On Thu, Jul 26, 2007 at 07:26:53AM +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2007-07-25 at 13:19 +0200, Nick Piggin wrote:
> > > Hi,
> > > 
> > > Does this patch solve the X problem? Does anyone see anything wrong
> > > with it or know why agp was locking the pages?
> > 
> > We need to do a little bit of auditing here, but I suspect it will turn
> > out all right. I think the reason it locked them in the first place was
> > to avoid AGP pages mapped into process space from being swapped out.
> > 
> > I think that should be taken care of by appropriate vma flags nowadays,
> > but we need to double check. It also might have been a way around dodgy
> > refcounting at one point but I think we got that right nowadays (I
> > remember fixing issues in that area when we removed PageReserved from
> > those pages back then).
> 
> Yeah I had a bit of a look around, and it seems OK (but would
> appreciate an ack from someone who knows the code).
> 
> These pages will never get seen by page reclaim, so we're OK
> there. There is a get_page before the SetPageLocked and a put_page
> right before the unlock_page, so refcounting should not be broken
> if it wasn't already: note that the lock_page doesn't pin a
> reference on a page in general -- we can use it as such for pagecache
> (although it isn't very clean), because the lock pins the page in
> pagecache and the pagecache holds a ref.
> 
> Anyway, if Dave or David can take a look, that would be appreciated.
> We'll need this for 2.6.23.
> 
> Nick
> 
> > 
> > Ben.
> > 
> > > --
> > > AGP should not need to lock pages. They are not protecting any race
> > > because there is no lock_page calls, only SetPageLocked.
> > > 
> > > This is causing hangs with d00806b183152af6d24f46f0c33f14162ca1262a.
> > > 
> > > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > > 
> > > diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
> > > index d535c40..3db4f40 100644
> > > --- a/drivers/char/agp/generic.c
> > > +++ b/drivers/char/agp/generic.c
> > > @@ -1170,7 +1170,6 @@ void *agp_generic_alloc_page(struct agp_
> > >  	map_page_into_agp(page);
> > >  
> > >  	get_page(page);
> > > -	SetPageLocked(page);
> > >  	atomic_inc(&agp_bridge->current_memory_agp);
> > >  	return page_address(page);
> > >  }
> > > @@ -1187,7 +1186,6 @@ void agp_generic_destroy_page(void *addr
> > >  	page = virt_to_page(addr);
> > >  	unmap_page_from_agp(page);
> > >  	put_page(page);
> > > -	unlock_page(page);
> > >  	free_page((unsigned long)addr);
> > >  	atomic_dec(&agp_bridge->current_memory_agp);
> > >  }
> > > diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
> > > index a124060..2f319f4 100644
> > > --- a/drivers/char/agp/intel-agp.c
> > > +++ b/drivers/char/agp/intel-agp.c
> > > @@ -213,7 +213,6 @@ static void *i8xx_alloc_pages(void)
> > >  	}
> > >  	global_flush_tlb();
> > >  	get_page(page);
> > > -	SetPageLocked(page);
> > >  	atomic_inc(&agp_bridge->current_memory_agp);
> > >  	return page_address(page);
> > >  }
> > > @@ -229,7 +228,6 @@ static void i8xx_destroy_pages(void *add
> > >  	change_page_attr(page, 4, PAGE_KERNEL);
> > >  	global_flush_tlb();
> > >  	put_page(page);
> > > -	unlock_page(page);
> > >  	__free_pages(page, 2);
> > >  	atomic_dec(&agp_bridge->current_memory_agp);
> > >  }
> > > diff --git a/drivers/char/agp/sgi-agp.c b/drivers/char/agp/sgi-agp.c
> > > index cda608c..98cf8ab 100644
> > > --- a/drivers/char/agp/sgi-agp.c
> > > +++ b/drivers/char/agp/sgi-agp.c
> > > @@ -51,7 +51,6 @@ static void *sgi_tioca_alloc_page(struct
> > >  		return NULL;
> > >  
> > >  	get_page(page);
> > > -	SetPageLocked(page);
> > >  	atomic_inc(&agp_bridge->current_memory_agp);
> > >  	return page_address(page);
> > >  }

Re: [patch] agp: don't lock pages

[Dave Airlie]

>
> On Thu, Jul 26, 2007 at 07:26:53AM +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2007-07-25 at 13:19 +0200, Nick Piggin wrote:
> > > Hi,
> > >
> > > Does this patch solve the X problem? Does anyone see anything wrong
> > > with it or know why agp was locking the pages?
> >
> > We need to do a little bit of auditing here, but I suspect it will turn
> > out all right. I think the reason it locked them in the first place was
> > to avoid AGP pages mapped into process space from being swapped out.
> >
> > I think that should be taken care of by appropriate vma flags nowadays,
> > but we need to double check. It also might have been a way around dodgy
> > refcounting at one point but I think we got that right nowadays (I
> > remember fixing issues in that area when we removed PageReserved from
> > those pages back then).
>
> Yeah I had a bit of a look around, and it seems OK (but would
> appreciate an ack from someone who knows the code).
>
> These pages will never get seen by page reclaim, so we're OK
> there. There is a get_page before the SetPageLocked and a put_page
> right before the unlock_page, so refcounting should not be broken
> if it wasn't already: note that the lock_page doesn't pin a
> reference on a page in general -- we can use it as such for pagecache
> (although it isn't very clean), because the lock pins the page in
> pagecache and the pagecache holds a ref.
>
> Anyway, if Dave or David can take a look, that would be appreciated.
> We'll need this for 2.6.23.

I talked with Ben earlier and I can't see anything inherently wrong
with removing the lock_page, I assume it was put there to stop things
getting swapped but if the get/put does that then I'd be happy to
remove it.

I'm just a bit confused how this didn't get picked up in -mm at all.

Dave.

Re: [patch] agp: don't lock pages

[Nick Piggin]

On Thu, Jul 26, 2007 at 11:44:22AM +1000, Dave Airlie wrote:
> >
> >Yeah I had a bit of a look around, and it seems OK (but would
> >appreciate an ack from someone who knows the code).
> >
> >These pages will never get seen by page reclaim, so we're OK
> >there. There is a get_page before the SetPageLocked and a put_page
> >right before the unlock_page, so refcounting should not be broken
> >if it wasn't already: note that the lock_page doesn't pin a
> >reference on a page in general -- we can use it as such for pagecache
> >(although it isn't very clean), because the lock pins the page in
> >pagecache and the pagecache holds a ref.
> >
> >Anyway, if Dave or David can take a look, that would be appreciated.
> >We'll need this for 2.6.23.
> 
> I talked with Ben earlier and I can't see anything inherently wrong
> with removing the lock_page, I assume it was put there to stop things
> getting swapped but if the get/put does that then I'd be happy to
> remove it.

Well it is prevented from being swapped out because it never gets
put on swapout lists, but the get/put certainly doesn't hurt :)
 

> I'm just a bit confused how this didn't get picked up in -mm at all.

Beats me. It was in there for nearly 5 months. Mustn't have been
tested or reported.