[PATCH 2/3] drm/ttm: fix object deallocation to properly fill in the page pool.

[Michel Dänzer]

On 08.07.2015 02:41, Jerome Glisse wrote:
> On Tue, Jul 07, 2015 at 03:39:29PM +0900, Michel Dänzer wrote:
>> On 07.07.2015 01:10, Jerome Glisse wrote:
>>>
>>> In meantime i think we need to apply this patch as it is really a fix to
>>> make the code actually do what the comment and design pretends it does.
>>
>> I agree.
>>
>> BTW, maybe it should be split up into the actual fix (removing the
>> npages assignment) and the NUM_PAGES_TO_ALLOC related simplification?
> 
> This would make 2 really small patch, patch is small as it is :) But why
> not.

It's not about size but about having one commit for each logical change.
It took me a while to realize that the NUM_PAGES_TO_ALLOC changes aren't
directly related to the fix, and I've seen the same thing happen to others.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

[PATCH 2/3] drm/ttm: fix object deallocation to properly fill in the page pool.

[Michel Dänzer]

On 07.07.2015 01:10, Jerome Glisse wrote:
> On Mon, Jul 06, 2015 at 06:11:29PM +0900, Michel Dänzer wrote:
>>
>> Hi Jérôme,
>>
>> On 13.08.2014 12:52, Jérôme Glisse wrote:
>>> From: Jérôme Glisse 
>>>
>>> Current code never allowed the page pool to actualy fill in anyway. This fix
>>> it and also allow it to grow over its limit until it grow beyond the batch
>>> size for allocation and deallocation.
>>>
>>> Signed-off-by: Jérôme Glisse 
>>> Reviewed-by: Mario Kleiner 
>>> Tested-by: Michel Dänzer 
>>> Cc: Thomas Hellstrom 
>>> Cc: Konrad Rzeszutek Wilk 
>>> ---
>>>  drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 9 ++---
>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
>>> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>>> index c96db43..a076ff3 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>>> @@ -953,14 +953,9 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, 
>>> struct device *dev)
>>> } else {
>>> pool->npages_free += count;
>>> list_splice(_dma->pages_list, >free_list);
>>> -   npages = count;
>>> -   if (pool->npages_free > _manager->options.max_size) {
>>> +   if (pool->npages_free >= (_manager->options.max_size +
>>> + NUM_PAGES_TO_ALLOC))
>>> npages = pool->npages_free - _manager->options.max_size;
>>> -   /* free at least NUM_PAGES_TO_ALLOC number of pages
>>> -* to reduce calls to set_memory_wb */
>>> -   if (npages < NUM_PAGES_TO_ALLOC)
>>> -   npages = NUM_PAGES_TO_ALLOC;
>>> -   }
>>> }
>>> spin_unlock_irqrestore(>lock, irq_flags);
>>>  
>>>
>>
>> Colleagues of mine have measured significant performance gains for some
>> workloads with this patch. Without it, a lot of CPU cycles are spent
>> changing the caching attributes of pages on allocation.
>>
>> Note that the performance effect seems to mostly disappear when applying
>> patch 1 as well, so apparently 64MB is too small for the max pool size.
>>
>> Is there any chance this patch could be applied without the
>> controversial patch 3? If not, do you have ideas for addressing the
>> concerns raised against patch 3?
> 
> Wahou, now i need to find the keys to the DeLorean to travel back in time.
> 
> This patch is a fix and should be applied without 1 or 3. Because today
> basicly the pool is always emptied and never fill up. But as Thomas pointed
> out there is already bit too much pool accross the stack. Proper solution
> would be to work something inside the mm level or the architecture (i assume
> AMD is mostly interested in x86 on that front).
> 
> Here the whole issue is really about allocating page with WC/UC cache
> properties. Changing cache properties on page is really costly on several
> level, like the kernel needs to break the huge linear mapping and populate
> lower level to remap the page with proper cache attribute inside the kernel
> mapping.
> 
> As far as i remember the kernel never goes back to huge page mapping when
> restoring page cache attribute, which meaning that little by litte with
> uptime you loose the whole huge page mapping benefit for everything and you
> waste more memory.

That sounds pretty bad, but this patch might actually help a little for
that by reducing the amount of huge page mappings that need to be broken up?


> In meantime i think we need to apply this patch as it is really a fix to
> make the code actually do what the comment and design pretends it does.

I agree.

BTW, maybe it should be split up into the actual fix (removing the
npages assignment) and the NUM_PAGES_TO_ALLOC related simplification?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

[PATCH 2/3] drm/ttm: fix object deallocation to properly fill in the page pool.

[Jerome Glisse]

On Tue, Jul 07, 2015 at 03:39:29PM +0900, Michel Dänzer wrote:
> On 07.07.2015 01:10, Jerome Glisse wrote:
> > On Mon, Jul 06, 2015 at 06:11:29PM +0900, Michel Dänzer wrote:
> >>
> >> Hi Jérôme,
> >>
> >> On 13.08.2014 12:52, Jérôme Glisse wrote:
> >>> From: Jérôme Glisse 
> >>>
> >>> Current code never allowed the page pool to actualy fill in anyway. This 
> >>> fix
> >>> it and also allow it to grow over its limit until it grow beyond the batch
> >>> size for allocation and deallocation.
> >>>
> >>> Signed-off-by: Jérôme Glisse 
> >>> Reviewed-by: Mario Kleiner 
> >>> Tested-by: Michel Dänzer 
> >>> Cc: Thomas Hellstrom 
> >>> Cc: Konrad Rzeszutek Wilk 
> >>> ---
> >>>  drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 9 ++---
> >>>  1 file changed, 2 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
> >>> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> >>> index c96db43..a076ff3 100644
> >>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> >>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> >>> @@ -953,14 +953,9 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, 
> >>> struct device *dev)
> >>>   } else {
> >>>   pool->npages_free += count;
> >>>   list_splice(_dma->pages_list, >free_list);
> >>> - npages = count;
> >>> - if (pool->npages_free > _manager->options.max_size) {
> >>> + if (pool->npages_free >= (_manager->options.max_size +
> >>> +   NUM_PAGES_TO_ALLOC))
> >>>   npages = pool->npages_free - _manager->options.max_size;
> >>> - /* free at least NUM_PAGES_TO_ALLOC number of pages
> >>> -  * to reduce calls to set_memory_wb */
> >>> - if (npages < NUM_PAGES_TO_ALLOC)
> >>> - npages = NUM_PAGES_TO_ALLOC;
> >>> - }
> >>>   }
> >>>   spin_unlock_irqrestore(>lock, irq_flags);
> >>>  
> >>>
> >>
> >> Colleagues of mine have measured significant performance gains for some
> >> workloads with this patch. Without it, a lot of CPU cycles are spent
> >> changing the caching attributes of pages on allocation.
> >>
> >> Note that the performance effect seems to mostly disappear when applying
> >> patch 1 as well, so apparently 64MB is too small for the max pool size.
> >>
> >> Is there any chance this patch could be applied without the
> >> controversial patch 3? If not, do you have ideas for addressing the
> >> concerns raised against patch 3?
> > 
> > Wahou, now i need to find the keys to the DeLorean to travel back in time.
> > 
> > This patch is a fix and should be applied without 1 or 3. Because today
> > basicly the pool is always emptied and never fill up. But as Thomas pointed
> > out there is already bit too much pool accross the stack. Proper solution
> > would be to work something inside the mm level or the architecture (i assume
> > AMD is mostly interested in x86 on that front).
> > 
> > Here the whole issue is really about allocating page with WC/UC cache
> > properties. Changing cache properties on page is really costly on several
> > level, like the kernel needs to break the huge linear mapping and populate
> > lower level to remap the page with proper cache attribute inside the kernel
> > mapping.
> > 
> > As far as i remember the kernel never goes back to huge page mapping when
> > restoring page cache attribute, which meaning that little by litte with
> > uptime you loose the whole huge page mapping benefit for everything and you
> > waste more memory.
> 
> That sounds pretty bad, but this patch might actually help a little for
> that by reducing the amount of huge page mappings that need to be broken up?

Not really, for limiting huge page mapping breakage you would need to allocate
page in same physical cluster so that only 1 huge page mapping needs to be
broken. It would be a bit like DMA32 or DMA16 physical range. So this would
obviously need some work at the MM level. At ttm level this can be more or
less implemented using GFP_DMA32 flag on page allocation but at the same time
doing that you kind of put more pressure on the first 4G of memory and i
think nowadays with the whole webbrowser consuming several GB of texture you
probably do not want to do that.


> > In meantime i think we need to apply this patch as it is really a fix to
> > make the code actually do what the comment and design pretends it does.
> 
> I agree.
> 
> BTW, maybe it should be split up into the actual fix (removing the
> npages assignment) and the NUM_PAGES_TO_ALLOC related simplification?

This would make 2 really small patch, patch is small as it is :) But why
not.

Cheers,
Jérôme

[PATCH 2/3] drm/ttm: fix object deallocation to properly fill in the page pool.

[Michel Dänzer]

Hi Jérôme,


On 13.08.2014 12:52, Jérôme Glisse wrote:
> From: Jérôme Glisse 
> 
> Current code never allowed the page pool to actualy fill in anyway. This fix
> it and also allow it to grow over its limit until it grow beyond the batch
> size for allocation and deallocation.
> 
> Signed-off-by: Jérôme Glisse 
> Reviewed-by: Mario Kleiner 
> Tested-by: Michel Dänzer 
> Cc: Thomas Hellstrom 
> Cc: Konrad Rzeszutek Wilk 
> ---
>  drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index c96db43..a076ff3 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -953,14 +953,9 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, 
> struct device *dev)
>   } else {
>   pool->npages_free += count;
>   list_splice(_dma->pages_list, >free_list);
> - npages = count;
> - if (pool->npages_free > _manager->options.max_size) {
> + if (pool->npages_free >= (_manager->options.max_size +
> +   NUM_PAGES_TO_ALLOC))
>   npages = pool->npages_free - _manager->options.max_size;
> - /* free at least NUM_PAGES_TO_ALLOC number of pages
> -  * to reduce calls to set_memory_wb */
> - if (npages < NUM_PAGES_TO_ALLOC)
> - npages = NUM_PAGES_TO_ALLOC;
> - }
>   }
>   spin_unlock_irqrestore(>lock, irq_flags);
>  
> 

Colleagues of mine have measured significant performance gains for some
workloads with this patch. Without it, a lot of CPU cycles are spent
changing the caching attributes of pages on allocation.

Note that the performance effect seems to mostly disappear when applying
patch 1 as well, so apparently 64MB is too small for the max pool size.

Is there any chance this patch could be applied without the
controversial patch 3? If not, do you have ideas for addressing the
concerns raised against patch 3?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

[PATCH 2/3] drm/ttm: fix object deallocation to properly fill in the page pool.

[Jerome Glisse]

On Mon, Jul 06, 2015 at 06:11:29PM +0900, Michel Dänzer wrote:
> 
> Hi Jérôme,
> 
> On 13.08.2014 12:52, Jérôme Glisse wrote:
> > From: Jérôme Glisse 
> > 
> > Current code never allowed the page pool to actualy fill in anyway. This fix
> > it and also allow it to grow over its limit until it grow beyond the batch
> > size for allocation and deallocation.
> > 
> > Signed-off-by: Jérôme Glisse 
> > Reviewed-by: Mario Kleiner 
> > Tested-by: Michel Dänzer 
> > Cc: Thomas Hellstrom 
> > Cc: Konrad Rzeszutek Wilk 
> > ---
> >  drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 9 ++---
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
> > b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > index c96db43..a076ff3 100644
> > --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > @@ -953,14 +953,9 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, 
> > struct device *dev)
> > } else {
> > pool->npages_free += count;
> > list_splice(_dma->pages_list, >free_list);
> > -   npages = count;
> > -   if (pool->npages_free > _manager->options.max_size) {
> > +   if (pool->npages_free >= (_manager->options.max_size +
> > + NUM_PAGES_TO_ALLOC))
> > npages = pool->npages_free - _manager->options.max_size;
> > -   /* free at least NUM_PAGES_TO_ALLOC number of pages
> > -* to reduce calls to set_memory_wb */
> > -   if (npages < NUM_PAGES_TO_ALLOC)
> > -   npages = NUM_PAGES_TO_ALLOC;
> > -   }
> > }
> > spin_unlock_irqrestore(>lock, irq_flags);
> >  
> > 
> 
> Colleagues of mine have measured significant performance gains for some
> workloads with this patch. Without it, a lot of CPU cycles are spent
> changing the caching attributes of pages on allocation.
> 
> Note that the performance effect seems to mostly disappear when applying
> patch 1 as well, so apparently 64MB is too small for the max pool size.
> 
> Is there any chance this patch could be applied without the
> controversial patch 3? If not, do you have ideas for addressing the
> concerns raised against patch 3?

Wahou, now i need to find the keys to the DeLorean to travel back in time.

This patch is a fix and should be applied without 1 or 3. Because today
basicly the pool is always emptied and never fill up. But as Thomas pointed
out there is already bit too much pool accross the stack. Proper solution
would be to work something inside the mm level or the architecture (i assume
AMD is mostly interested in x86 on that front).

Here the whole issue is really about allocating page with WC/UC cache
properties. Changing cache properties on page is really costly on several
level, like the kernel needs to break the huge linear mapping and populate
lower level to remap the page with proper cache attribute inside the kernel
mapping.

As far as i remember the kernel never goes back to huge page mapping when
restoring page cache attribute, which meaning that little by litte with
uptime you loose the whole huge page mapping benefit for everything and you
waste more memory.

Anyway just wanted to dump here my recolection and how i think patch 1 & 3
should be replaced by simply moving down this allocation infrastructure
inside core mm code. Where it should always have been.

In meantime i think we need to apply this patch as it is really a fix to
make the code actually do what the comment and design pretends it does.

Cheers,
Jérôme

[PATCH 2/3] drm/ttm: fix object deallocation to properly fill in the page pool.

[Konrad Rzeszutek Wilk]

On Tue, Aug 12, 2014 at 11:52:05PM -0400, Jérôme Glisse wrote:
> From: Jérôme Glisse 
> 
> Current code never allowed the page pool to actualy fill in anyway. This fix
> it and also allow it to grow over its limit until it grow beyond the batch
> size for allocation and deallocation.
> 
> Signed-off-by: Jérôme Glisse 
> Reviewed-by: Mario Kleiner 
> Tested-by: Michel Dänzer 
> Cc: Thomas Hellstrom 
> Cc: Konrad Rzeszutek Wilk 

Reviewed-by: Konrad Rzeszutek Wilk 
> ---
>  drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index c96db43..a076ff3 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -953,14 +953,9 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, 
> struct device *dev)
>   } else {
>   pool->npages_free += count;
>   list_splice(_dma->pages_list, >free_list);
> - npages = count;
> - if (pool->npages_free > _manager->options.max_size) {
> + if (pool->npages_free >= (_manager->options.max_size +
> +   NUM_PAGES_TO_ALLOC))
>   npages = pool->npages_free - _manager->options.max_size;
> - /* free at least NUM_PAGES_TO_ALLOC number of pages
> -  * to reduce calls to set_memory_wb */
> - if (npages < NUM_PAGES_TO_ALLOC)
> - npages = NUM_PAGES_TO_ALLOC;
> - }
>   }
>   spin_unlock_irqrestore(>lock, irq_flags);
>  
> -- 
> 1.9.3
> 

[PATCH 2/3] drm/ttm: fix object deallocation to properly fill in the page pool.

[Jérôme Glisse]

From: J?r?me Glisse 

Current code never allowed the page pool to actualy fill in anyway. This fix
it and also allow it to grow over its limit until it grow beyond the batch
size for allocation and deallocation.

Signed-off-by: J?r?me Glisse 
Reviewed-by: Mario Kleiner 
Tested-by: Michel D?nzer 
Cc: Thomas Hellstrom 
Cc: Konrad Rzeszutek Wilk 
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c 
b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index c96db43..a076ff3 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -953,14 +953,9 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, struct 
device *dev)
} else {
pool->npages_free += count;
list_splice(_dma->pages_list, >free_list);
-   npages = count;
-   if (pool->npages_free > _manager->options.max_size) {
+   if (pool->npages_free >= (_manager->options.max_size +
+ NUM_PAGES_TO_ALLOC))
npages = pool->npages_free - _manager->options.max_size;
-   /* free at least NUM_PAGES_TO_ALLOC number of pages
-* to reduce calls to set_memory_wb */
-   if (npages < NUM_PAGES_TO_ALLOC)
-   npages = NUM_PAGES_TO_ALLOC;
-   }
}
spin_unlock_irqrestore(>lock, irq_flags);

-- 
1.9.3