linux-kernel - Re: [PATCH] staging: ion: Rework ion_map_dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 Oct 2018 19:53:00 -0700
From:   John Stultz <john.stultz@...aro.org>
To:     Laura Abbott <labbott@...hat.com>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Beata Michalska <Beata.Michalska@....com>,
        Matt Szczesiak <matt.szczesiak@....com>,
        Anders Pedersen <Anders.Pedersen@....com>,
        John Reitan <John.Reitan@....com>,
        Liam Mark <lmark@...eaurora.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] staging: ion: Rework ion_map_dma_buf() to minimize re-mapping

On Fri, Oct 12, 2018 at 10:51 AM, Laura Abbott <labbott@...hat.com> wrote:
>
> I suspect most of the cost of the dma_map/dma_unmap is from the
> cache flushing and not the actual mapping operations. If this
> is the case, another option might be to figure out how to
> incorporate dma_attrs so drivers can use DMA_ATTR_SKIP_CPU_SYNC
> to decide when they actually want to sync.

So just to confirm on this point, I basically tested the following
change (whitespace corrupt, sorry):

diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 24cb666..e76b0e2 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -273,8 +273,8 @@ static struct sg_table *ion_map_dma_buf(struct
dma_buf_attachment *attachment,

        table = a->table;

-       if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
-                       direction))
+       if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
+                             direction,  DMA_ATTR_SKIP_CPU_SYNC))
                return ERR_PTR(-ENOMEM);

        return table;
@@ -284,7 +284,7 @@ static void ion_unmap_dma_buf(struct
dma_buf_attachment *attachment,
                              struct sg_table *table,
                              enum dma_data_direction direction)
 {
-       dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+       dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents,
direction, DMA_ATTR_SKIP_CPU_SYNC);
 }

 static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)


And indeed, that performed similarly to the pre 4.12 ION code (and it
also had some of the same image caching error garbage we've seen w/
4.9 era kernels, which my earlier patch didn't have).

So yes, it seems having some way to conditionally skip cpu sync would
be good.  Though I'm not sure what sort of interface to using this you
might have in mind?

thanks
-john