From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 2/3] drm/ttm/dma: Fix accounting error when calling ttm_mem_global_free_page. Date: Wed, 21 Dec 2011 10:30:18 -0500 Message-ID: <20111221153018.GA26757@phenom.dumpdata.com> References: <1323720569-15435-1-git-send-email-konrad.wilk@oracle.com> <1323720569-15435-3-git-send-email-konrad.wilk@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Jerome Glisse Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, bskeggs@redhat.com, thellstrom@vmware.com List-Id: dri-devel@lists.freedesktop.org On Wed, Dec 21, 2011 at 10:17:36AM -0500, Jerome Glisse wrote: > On Mon, Dec 12, 2011 at 3:09 PM, Konrad Rzeszutek Wilk > wrote: > > The code to figure out how many pages to shrink the pool > > ends up capping the 'count' at _manager->options.max_size - which i= s OK. > > Except that the 'count' is also used when accounting for how many p= ages > > are recycled - which we end up with the invalid values. This fixes > > it by using a different value for the amount of pages to shrink. > > > > Signed-off-by: Konrad Rzeszutek Wilk > > --- > > =A0drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | =A0 10 ++++++---- > > =A01 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu= /drm/ttm/ttm_page_alloc_dma.c > > index 6c06d0b..e57aa24 100644 > > --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > > @@ -949,7 +949,7 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_= dma, struct device *dev) > > =A0 =A0 =A0 =A0struct dma_page *d_page, *next; > > =A0 =A0 =A0 =A0enum pool_type type; > > =A0 =A0 =A0 =A0bool is_cached =3D false; > > - =A0 =A0 =A0 unsigned count =3D 0, i; > > + =A0 =A0 =A0 unsigned count =3D 0, i, npages; > > =A0 =A0 =A0 =A0unsigned long irq_flags; > > > > =A0 =A0 =A0 =A0type =3D ttm_to_type(ttm->page_flags, ttm->caching_s= tate); > > @@ -971,11 +971,13 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *tt= m_dma, struct device *dev) > > =A0 =A0 =A0 =A0pool->npages_in_use -=3D count; > > =A0 =A0 =A0 =A0if (is_cached) { > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pool->nfrees +=3D count; > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 npages =3D count; >=20 > This is wrong, i thought we didn't wanted to shrink cached page, your > 3/3 patch remove this line. Thought i am kind of wondering why we > don't shrink the cached pool, i can't remember the reason. We delete the cached ones: 988 if (is_cached) { 989 list_for_each_entry_safe(d_page, next, &ttm_dma->p= ages_list, page_list) { 990 ttm_mem_global_free_page(ttm->glob->mem_gl= ob, 991 d_page->p); 992 ttm_dma_page_put(pool, d_page); 993 } And then we end up calling in ttm_dma_page_pool_free for (count) pages that we had already deleted. So it ends up being a bit redundant. The 3/3 patch fixes that (by removing the call to ttm_dma_page_pool_fre= e for is_cached. This patch (2/3) tries to preserve the logic if it is ei= ther is_cached or !is_cached. >=20 > > =A0 =A0 =A0 =A0} else { > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pool->npages_free +=3D count; > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0list_splice(&ttm_dma->pages_list, &p= ool->free_list); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 npages =3D count; > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (pool->npages_free > _manager->op= tions.max_size) { > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 count =3D pool->npage= s_free - _manager->options.max_size; > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 npages =3D pool->npag= es_free - _manager->options.max_size; > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > > =A0 =A0 =A0 =A0} > > =A0 =A0 =A0 =A0spin_unlock_irqrestore(&pool->lock, irq_flags); > > @@ -1000,8 +1002,8 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *tt= m_dma, struct device *dev) > > =A0 =A0 =A0 =A0} > > > > =A0 =A0 =A0 =A0/* shrink pool if necessary */ > > - =A0 =A0 =A0 if (count) > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ttm_dma_page_pool_free(pool, count); > > + =A0 =A0 =A0 if (npages) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ttm_dma_page_pool_free(pool, npages); > > =A0 =A0 =A0 =A0ttm->state =3D tt_unpopulated; > > =A0} > > =A0EXPORT_SYMBOL_GPL(ttm_dma_unpopulate); > > -- > > 1.7.7.3 > > >=20 > Otherwise Reviewed-by:Jerome Glisse Thanks!