linux-kernel - Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR

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:	Fri, 8 Jan 2016 13:10:09 +0000
From:	Robin Murphy <robin.murphy@....com>
To:	Douglas Anderson <dianders@...omium.org>,
	Russell King <linux@....linux.org.uk>
Cc:	Tomasz Figa <tfiga@...omium.org>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Pawel Osciak <pawel@...iak.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>, corbet@....net,
	akpm@...ux-foundation.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE
 attribute

Hi Doug,

On 08/01/16 00:36, Douglas Anderson wrote:
> This patch adds the DMA_ATTR_NOHUGEPAGE attribute to the DMA-mapping
> subsystem.
>
> This attribute can be used as a hint to the DMA-mapping subsystem that
> it's likely not worth it to try to allocation large pages behind the
> scenes.  Large pages are likely to make an IOMMU TLB work more
> efficiently but may not be worth it.  See the Documentation contained in
> this patch for more details about this attribute and when to use it.
>
> Note that the name of the hint (DMA_ATTR_NOHUGEPAGE) is based on the
> name MADV_NOHUGEPAGE, which has the same meaning.  If we have expected
> users, we could also add MADV_HUGEPAGE which has the opposite meaning of
> this hint.
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> Acked-by: Marek Szyprowski <m.szyprowski@...sung.com>
> ---
> Changes in v4:
> - renamed DMA_ATTR_SEQUENTIAL to DMA_ATTR_NOHUGEPAGE
> - added Marek's ack
>
> Changes in v3:
> - add DMA_ATTR_SEQUENTIAL attribute new for v3
>
> Changes in v2: None
>
>   Documentation/DMA-attributes.txt | 23 +++++++++++++++++++++++
>   include/linux/dma-attrs.h        |  1 +
>   2 files changed, 24 insertions(+)
>
> diff --git a/Documentation/DMA-attributes.txt b/Documentation/DMA-attributes.txt
> index 18dc52c4f2a0..0a2f56e9c5bd 100644
> --- a/Documentation/DMA-attributes.txt
> +++ b/Documentation/DMA-attributes.txt
> @@ -100,3 +100,26 @@ allocated by dma_alloc_attrs() function from individual pages if it can
>   be mapped as contiguous chunk into device dma address space. By
>   specifying this attribute the allocated buffer is forced to be contiguous
>   also in physical memory.
> +
> +DMA_ATTR_NOHUGEPAGE
> +-------------------

Bikeshed: DMA_ATTR_NO_HUGEPAGE (or even DMA_ATTR_NO_HUGE_PAGE) would be 
more consistent with the naming style of the other attributes.

> +
> +This is a hint to the DMA-mapping subsystem that it's probably not worth
> +the time to try to allocate memory to in a way that gives better TLB
> +efficiency (AKA it's not worth trying to build the mapping out of larger
> +pages).  You might want to specify this if:
> +- You know that the accesses to this memory won't thrash the TLB.
> +  You might know that the accesses are likely to be sequential or
> +  that they aren't sequential but it's unlikely you'll ping-ping

                                                          ^ping-pong?

> +  between many addresses that are likely to be in different physical
> +  pages.
> +- You know that the penalty of TLB misses while accessing the
> +  memory will be small enough to be inconsequential.  If you are
> +  doing a heavy operation like decryption or decompression this
> +  might be the case.
> +- You know that the DMA mapping is fairly transitory.  If you expect
> +  the mapping to have a short lifetime then it may be worth it to
> +  optimize allocation (avoid coming up with large pages) instead of
> +  getting the slight performance win of larger pages.
> +Setting this hint doesn't guarantee that you won't get huge pages, but it
> +means that we won't try quite as hard to get them.

Nice detailed description, but I do worry it's a bit too ambiguous - it 
still parses perfectly well if you assume the references are to CPU TLBs 
and CPU accesses, rather than IOMMU TLBs and device accesses, especially 
given that the CPU is equally relevant to coherent DMA and there may not 
be an IOMMU at all. I assume that's not intentional, because otherwise 
it's also not quite accurate (I did once try to understand why we still 
have to split a CPU huge page for DMA even with a corresponding IOMMU 
huge page, but I remember getting completely lost somewhere in the 
bowels of the mm code).

Other than that, though, the rest of the series looks fine to me.

Thanks,
Robin.

> diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h
> index 99c0be00b47c..678662a235d1 100644
> --- a/include/linux/dma-attrs.h
> +++ b/include/linux/dma-attrs.h
> @@ -18,6 +18,7 @@ enum dma_attr {
>   	DMA_ATTR_NO_KERNEL_MAPPING,
>   	DMA_ATTR_SKIP_CPU_SYNC,
>   	DMA_ATTR_FORCE_CONTIGUOUS,
> +	DMA_ATTR_NOHUGEPAGE,
>   	DMA_ATTR_MAX,
>   };
>
>