page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

[Christoph Hellwig]

On Wed, Jun 30, 2021 at 01:34:17AM +0000, John Stultz wrote:
> This adds a shrinker controlled page pool, extracted
> out of the ttm_pool logic, and abstracted out a bit
> so it can be used by other non-ttm drivers.

Can you explain in detail why you need a differnt page pool over the one
maintained by the page allocator?  Fragmenting the memory into all kinds
of pools has lots of downsides, so the upsides need to be explained in
detail.

Re: page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

[Christian König]

Am 07.07.21 um 08:38 schrieb Christoph Hellwig:
> On Wed, Jun 30, 2021 at 01:34:17AM +0000, John Stultz wrote:
>> This adds a shrinker controlled page pool, extracted
>> out of the ttm_pool logic, and abstracted out a bit
>> so it can be used by other non-ttm drivers.
> Can you explain in detail why you need a differnt page pool over the one
> maintained by the page allocator?  Fragmenting the memory into all kinds
> of pools has lots of downsides, so the upsides need to be explained in
> detail.

Well, the original code all this is based on already had the comment 
that this really belong into the page allocator.

The key point is traditionally only GPUs used uncached and 
write-combined memory in such large quantities that having a pool for 
them makes sense.

Because of this we had this separately to reduce the complexity in the 
page allocator to handle another set of complexity of allocation types.

For the upside, for those use cases it means huge performance 
improvements for those drivers. See the numbers John provided in the 
cover letter.

But essentially at least I would be totally fine moving this into the 
page allocator, but moving it outside of TTM already helps with this 
goal. So this patch set is certainly a step into the right direction.

Regards,
Christian.

Re: page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

[Christoph Hellwig]

On Wed, Jul 07, 2021 at 09:10:26AM +0200, Christian K??nig wrote:
> Well, the original code all this is based on already had the comment that
> this really belong into the page allocator.
> 
> The key point is traditionally only GPUs used uncached and write-combined
> memory in such large quantities that having a pool for them makes sense.
> 
> Because of this we had this separately to reduce the complexity in the page
> allocator to handle another set of complexity of allocation types.
> 
> For the upside, for those use cases it means huge performance improvements
> for those drivers. See the numbers John provided in the cover letter.
> 
> But essentially at least I would be totally fine moving this into the page
> allocator, but moving it outside of TTM already helps with this goal. So
> this patch set is certainly a step into the right direction.

Unless I'm badly misreading the patch and this series there is nothing
about cache attributes in this code.  It just allocates pages, zeroes
them, eventually hands them out to a consumer and registers a shrinker
for its freelist.

If OTOH it actually dealt with cachability that should be documented
in the commit log and probably also the naming of the implementation.

Re: page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

[Christian König]

Am 07.07.21 um 09:14 schrieb Christoph Hellwig:
> On Wed, Jul 07, 2021 at 09:10:26AM +0200, Christian K??nig wrote:
>> Well, the original code all this is based on already had the comment that
>> this really belong into the page allocator.
>>
>> The key point is traditionally only GPUs used uncached and write-combined
>> memory in such large quantities that having a pool for them makes sense.
>>
>> Because of this we had this separately to reduce the complexity in the page
>> allocator to handle another set of complexity of allocation types.
>>
>> For the upside, for those use cases it means huge performance improvements
>> for those drivers. See the numbers John provided in the cover letter.
>>
>> But essentially at least I would be totally fine moving this into the page
>> allocator, but moving it outside of TTM already helps with this goal. So
>> this patch set is certainly a step into the right direction.
> Unless I'm badly misreading the patch and this series there is nothing
> about cache attributes in this code.  It just allocates pages, zeroes
> them, eventually hands them out to a consumer and registers a shrinker
> for its freelist.
>
> If OTOH it actually dealt with cachability that should be documented
> in the commit log and probably also the naming of the implementation.

Mhm, good point. In this case all left is the clearing overhead from the 
allocation hot path to the free path and I'm not really sure why the 
core page allocator shouldn't do that as well.

Regards,
Christian.

Re: page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

[John Stultz]

On Tue, Jul 6, 2021 at 11:38 PM Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Jun 30, 2021 at 01:34:17AM +0000, John Stultz wrote:
> > This adds a shrinker controlled page pool, extracted
> > out of the ttm_pool logic, and abstracted out a bit
> > so it can be used by other non-ttm drivers.
>
> Can you explain in detail why you need a differnt page pool over the one
> maintained by the page allocator?  Fragmenting the memory into all kinds
> of pools has lots of downsides, so the upsides need to be explained in
> detail.

So, as Christian mentioned, on the TTM side it's useful, as they are
trying to avoid TLB flushes when changing caching attributes.

For the dmabuf system heap purposes, the main benefit is moving the
page zeroing to the free path, rather than the allocation path. This
on its own doesn't save much, but allows us to defer frees (and thus
the zeroing) to the background, which can get that work out of the hot
path.

thanks
-john

Re: page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

[John Stultz]

On Wed, Jul 7, 2021 at 12:15 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Jul 07, 2021 at 09:10:26AM +0200, Christian K??nig wrote:
> > Well, the original code all this is based on already had the comment that
> > this really belong into the page allocator.
> >
> > The key point is traditionally only GPUs used uncached and write-combined
> > memory in such large quantities that having a pool for them makes sense.
> >
> > Because of this we had this separately to reduce the complexity in the page
> > allocator to handle another set of complexity of allocation types.
> >
> > For the upside, for those use cases it means huge performance improvements
> > for those drivers. See the numbers John provided in the cover letter.
> >
> > But essentially at least I would be totally fine moving this into the page
> > allocator, but moving it outside of TTM already helps with this goal. So
> > this patch set is certainly a step into the right direction.
>
> Unless I'm badly misreading the patch and this series there is nothing
> about cache attributes in this code.  It just allocates pages, zeroes
> them, eventually hands them out to a consumer and registers a shrinker
> for its freelist.
>
> If OTOH it actually dealt with cachability that should be documented
> in the commit log and probably also the naming of the implementation.

So the cache attributes are still managed in the tmm_pool code. This
series just pulls the pool/shrinker management out into common code so
we can use it in the dmabuf heap code without duplicating things.

thanks
-john

Re: page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

[Christoph Hellwig]

On Wed, Jul 07, 2021 at 12:35:23PM -0700, John Stultz wrote:
> So, as Christian mentioned, on the TTM side it's useful, as they are
> trying to avoid TLB flushes when changing caching attributes.
> 
> For the dmabuf system heap purposes, the main benefit is moving the
> page zeroing to the free path, rather than the allocation path. This
> on its own doesn't save much, but allows us to defer frees (and thus
> the zeroing) to the background, which can get that work out of the hot
> path.

I really do no think that is worth it to fragment the free pages.

Re: page pools, was Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

[Christian König]

Am 08.07.21 um 06:20 schrieb Christoph Hellwig:
> On Wed, Jul 07, 2021 at 12:35:23PM -0700, John Stultz wrote:
>> So, as Christian mentioned, on the TTM side it's useful, as they are
>> trying to avoid TLB flushes when changing caching attributes.
>>
>> For the dmabuf system heap purposes, the main benefit is moving the
>> page zeroing to the free path, rather than the allocation path. This
>> on its own doesn't save much, but allows us to defer frees (and thus
>> the zeroing) to the background, which can get that work out of the hot
>> path.
> I really do no think that is worth it to fragment the free pages.

And I think functionality like that should be part of the common page 
allocator.

I mean we already have __GFP_ZERO, why not have a background kernel 
thread which zeros free pages when a CPU core is idle? (I'm pretty sure 
we already have that somehow).

Christian.