Re: [RFC][PATCH 5/6] dma-buf: system_heap: Add pagepool support to system heap

[John Stultz]

On Tue, Sep 29, 2020 at 9:46 PM Chris Goldsworthy
<cgoldswo@xxxxxxxxxxxxxx> wrote:
>
> On 2020-09-25 21:24, John Stultz wrote:
> > Reuse/abuse the pagepool code from the network code to speed
> > up allocation performance.
> >
> > This is similar to the ION pagepool usage, but tries to
> > utilize generic code instead of a custom implementation.
> >
> > Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> > Cc: Liam Mark <lmark@xxxxxxxxxxxxxx>
> > Cc: Laura Abbott <labbott@xxxxxxxxxx>
> > Cc: Brian Starkey <Brian.Starkey@xxxxxxx>
> > Cc: Hridya Valsaraju <hridya@xxxxxxxxxx>
> > Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > Cc: Sandeep Patil <sspatil@xxxxxxxxxx>
> > Cc: Ørjan Eide <orjan.eide@xxxxxxx>
> > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > Cc: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > Cc: Simon Ser <contact@xxxxxxxxxxx>
> > Cc: James Jones <jajones@xxxxxxxxxx>
> > Cc: linux-media@xxxxxxxxxxxxxxx
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> > ---
> >  drivers/dma-buf/heaps/Kconfig       |  1 +
> >  drivers/dma-buf/heaps/system_heap.c | 32 +++++++++++++++++++++++++----
> >  2 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma-buf/heaps/Kconfig
> > b/drivers/dma-buf/heaps/Kconfig
> > index a5eef06c4226..f13cde4321b1 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -1,6 +1,7 @@
> >  config DMABUF_HEAPS_SYSTEM
> >       bool "DMA-BUF System Heap"
> >       depends on DMABUF_HEAPS
> > +     select PAGE_POOL
> >       help
> >         Choose this option to enable the system dmabuf heap. The system
> > heap
> >         is backed by pages from the buddy allocator. If in doubt, say Y.
> > diff --git a/drivers/dma-buf/heaps/system_heap.c
> > b/drivers/dma-buf/heaps/system_heap.c
> > index 882a632e9bb7..9f57b4c8ae69 100644
> > --- a/drivers/dma-buf/heaps/system_heap.c
> > +++ b/drivers/dma-buf/heaps/system_heap.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/scatterlist.h>
> >  #include <linux/slab.h>
> >  #include <linux/vmalloc.h>
> > +#include <net/page_pool.h>
> >
> >  struct dma_heap *sys_heap;
> >
> > @@ -46,6 +47,7 @@ struct dma_heap_attachment {
> >  static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP,
> > LOW_ORDER_GFP};
> >  static const unsigned int orders[] = {8, 4, 0};
> >  #define NUM_ORDERS ARRAY_SIZE(orders)
> > +struct page_pool *pools[NUM_ORDERS];
> >
> >  static struct sg_table *dup_sg_table(struct sg_table *table)
> >  {
> > @@ -264,13 +266,17 @@ static void system_heap_dma_buf_release(struct
> > dma_buf *dmabuf)
> >       struct system_heap_buffer *buffer = dmabuf->priv;
> >       struct sg_table *table;
> >       struct scatterlist *sg;
> > -     int i;
> > +     int i, j;
> >
> >       table = &buffer->sg_table;
> >       for_each_sg(table->sgl, sg, table->nents, i) {
> >               struct page *page = sg_page(sg);
> >
> > -             __free_pages(page, compound_order(page));
> > +             for (j = 0; j < NUM_ORDERS; j++) {
> > +                     if (compound_order(page) == orders[j])
> > +                             break;
> > +             }
> > +             page_pool_put_full_page(pools[j], page, false);
> >       }
> >       sg_free_table(table);
> >       kfree(buffer);
> > @@ -300,8 +306,7 @@ static struct page
> > *alloc_largest_available(unsigned long size,
> >                       continue;
> >               if (max_order < orders[i])
> >                       continue;
> > -
> > -             page = alloc_pages(order_flags[i], orders[i]);
> > +             page = page_pool_alloc_pages(pools[i], order_flags[i]);
> >               if (!page)
> >                       continue;
> >               return page;
> > @@ -406,6 +411,25 @@ static const struct dma_heap_ops system_heap_ops =
> > {
> >  static int system_heap_create(void)
> >  {
> >       struct dma_heap_export_info exp_info;
> > +     int i;
> > +
> > +     for (i = 0; i < NUM_ORDERS; i++) {
> > +             struct page_pool_params pp;
> > +
> > +             memset(&pp, 0, sizeof(pp));
> > +             pp.order = orders[i];
> > +             pp.dma_dir = DMA_BIDIRECTIONAL;
> > +             pools[i] = page_pool_create(&pp);
> > +
> > +             if (IS_ERR(pools[i])) {
> > +                     int j;
> > +
> > +                     pr_err("%s: page pool creation failed!\n", __func__);
> > +                     for (j = 0; j < i; j++)
> > +                             page_pool_destroy(pools[j]);
> > +                     return PTR_ERR(pools[i]);
> > +             }
> > +     }
> >
> >       exp_info.name = "system";
> >       exp_info.ops = &system_heap_ops;
>
> This is cool, I didn't know about this pooling code under /net/core.
> Nice and compact.

Oh, bummer. I just realized when allocating w/ __GFP_ZERO from the
page-pool, the logic doesn't actually clear pages when pulling from
the cache.
So unfortunately this is what accounts for much of the performance
benefit I was seeing with this approach, so I'll have to retract my
claim on the performance gain with this. :(

I've got a first pass at zeroing the pages we put into the pool, but
the numbers are not so great just yet so I've got some further work to
do.

thanks
-john