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] [day] [month] [year] [list]

Date:   Sun, 21 Oct 2018 02:13:39 -0700
From:   Laura Abbott <labbott@...hat.com>
To:     John Stultz <john.stultz@...aro.org>
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 10/17/2018 07:53 PM, John Stultz wrote:
> 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?
> 

I hadn't quite gotten anything fully formed but I think solving this
will require changes at the dma-buf layer. One idea I had was allowing
dma_attrs to be set per attachment, the other was extending the
dma_buf_map_attachment to take an attrs argument. I'm at OSSEU this
week but I didn't want to forget about this. I'll see if I can turn
this into a more coherent proposal unless you get to it first.

Thanks,
Laura

> thanks
> -john
>