[PATCH v4 0/3] dma-mapping: Patches for speeding up allocation

[PATCH v4 0/3] dma-mapping: Patches for speeding up allocation

[Douglas Anderson]

This series of 3 patches will speed up memory allocation in dma-mapping
quite a bit.

The first patch ("ARM: dma-mapping: Optimize allocation") is hopefully
not terribly controversial: it merely doesn't try as hard to allocate
big chunks once it gets the first failure.  Since it's unlikely that
further big chunks will help (they're not likely to be virtually aligned
anyway), this should give a big speedup with no real regression to speak
of.  Yes, things could be made better, but this seems like a sane start.

The second patch ("common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE
attribute") models MADV_NOHUGEPAGE as I understand it.  Hopefully folks
are happy with following that lead.  It does nothing by itself.

The third patch ("ARM: dma-mapping: Use DMA_ATTR_NOHUGEPAGE hint to
optimize allocation") simply applies the 2nd patch.  Again it's pretty
simple.  ...and again it does nothing by itself.

Notably missing from this series is the fourth patch that adds teeth to
the second and third.  You can find that out of tree at
<https://chromium-review.googlesource.com/#/c/320498/>.  Unfortunately
the rk3288_vpu, which is what I'm working on, is out of tree.

All testing was done on the chromeos kernel-3.14.  Sanity (compile /
boot) testing was done on a v4.4-rc6-based kernel.

Also note that v2 of this series had an extra patch
<https://patchwork.kernel.org/patch/7888861/> that would attempt to sort
the allocation results to opportunistically get some extra alignment.  I
dropped that, but it could be re-introduced if there was interest.  I
found that it did give a little extra alignment sometimes, but maybe not
enough to justify the extra complexity.  It also was a bit half-baked
since it really should have tried harder to ensure alignment.

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
- Use DMA_ATTR_SEQUENTIAL hint patch new for v3.

Changes in v2:
- No longer just 1 page at a time, but gives up higher order quickly.
- Only tries important higher order allocations that might help us.

Douglas Anderson (3):
  ARM: dma-mapping: Optimize allocation
  common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute
  ARM: dma-mapping: Use DMA_ATTR_NOHUGEPAGE hint to optimize allocation

 Documentation/DMA-attributes.txt | 23 +++++++++++++++++++++++
 arch/arm/mm/dma-mapping.c        | 38 ++++++++++++++++++++++++--------------
 include/linux/dma-attrs.h        |  1 +
 3 files changed, 48 insertions(+), 14 deletions(-)

-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v4 1/3] ARM: dma-mapping: Optimize allocation

[Douglas Anderson]

The __iommu_alloc_buffer() is expected to be called to allocate pretty
sizeable buffers.  Upon simple tests of video I saw it trying to
allocate 4,194,304 bytes.  The function tries to allocate large chunks
in order to optimize IOMMU TLB usage.

The current function is very, very slow.

One problem is the way it keeps trying and trying to allocate big
chunks.  Imagine a very fragmented memory that has 4M free but no
contiguous pages at all.  Further imagine allocating 4M (1024 pages).
We'll do the following memory allocations:
- For page 1:
  - Try to allocate order 10 (no retry)
  - Try to allocate order 9 (no retry)
  - ...
  - Try to allocate order 0 (with retry, but not needed)
- For page 2:
  - Try to allocate order 9 (no retry)
  - Try to allocate order 8 (no retry)
  - ...
  - Try to allocate order 0 (with retry, but not needed)
- ...
- ...

Total number of calls to alloc() calls for this case is:
  sum(int(math.log(i, 2)) + 1 for i in range(1, 1025))
  => 9228

The above is obviously worse case, but given how slow alloc can be we
really want to try to avoid even somewhat bad cases.  I timed the old
code with a device under memory pressure and it wasn't hard to see it
take more than 120 seconds to allocate 4 megs of memory! (NOTE: testing
was done on kernel 3.14, so possibly mainline would behave
differently).

A second problem is that allocating big chunks under memory pressure
when we don't need them is just not a great idea anyway unless we really
need them.  We can make due pretty well with smaller chunks so it's
probably wise to leave bigger chunks for other users once memory
pressure is on.

Let's adjust the allocation like this:

1. If a big chunk fails, stop trying to hard and bump down to lower
   order allocations.
2. Don't try useless orders.  The whole point of big chunks is to
   optimize the TLB and it can really only make use of 2M, 1M, 64K and
   4K sizes.

We'll still tend to eat up a bunch of big chunks, but that might be the
right answer for some users.  A future patch could possibly add a new
DMA_ATTR that would let the caller decide that TLB optimization isn't
important and that we should use smaller chunks.  Presumably this would
be a sane strategy for some callers.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Changes in v4:
- Added Marek's ack

Changes in v3: None
Changes in v2:
- No longer just 1 page at a time, but gives up higher order quickly.
- Only tries important higher order allocations that might help us.

 arch/arm/mm/dma-mapping.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 0eca3812527e..bc9cebfa0891 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1122,6 +1122,9 @@ static inline void __free_iova(struct dma_iommu_mapping *mapping,
 	spin_unlock_irqrestore(&mapping->lock, flags);
 }
 
+/* We'll try 2M, 1M, 64K, and finally 4K; array must end with 0! */
+static const int iommu_order_array[] = { 9, 8, 4, 0 };
+
 static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 					  gfp_t gfp, struct dma_attrs *attrs)
 {
@@ -1129,6 +1132,7 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 	int count = size >> PAGE_SHIFT;
 	int array_size = count * sizeof(struct page *);
 	int i = 0;
+	int order_idx = 0;
 
 	if (array_size <= PAGE_SIZE)
 		pages = kzalloc(array_size, GFP_KERNEL);
@@ -1162,22 +1166,24 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 	while (count) {
 		int j, order;
 
-		for (order = __fls(count); order > 0; --order) {
-			/*
-			 * We do not want OOM killer to be invoked as long
-			 * as we can fall back to single pages, so we force
-			 * __GFP_NORETRY for orders higher than zero.
-			 */
-			pages[i] = alloc_pages(gfp | __GFP_NORETRY, order);
-			if (pages[i])
-				break;
+		order = iommu_order_array[order_idx];
+
+		/* Drop down when we get small */
+		if (__fls(count) < order) {
+			order_idx++;
+			continue;
 		}
 
-		if (!pages[i]) {
-			/*
-			 * Fall back to single page allocation.
-			 * Might invoke OOM killer as last resort.
-			 */
+		if (order) {
+			/* See if it's easy to allocate a high-order chunk */
+			pages[i] = alloc_pages(gfp | __GFP_NORETRY, order);
+
+			/* Go down a notch at first sign of pressure */
+			if (!pages[i]) {
+				order_idx++;
+				continue;
+			}
+		} else {
 			pages[i] = alloc_pages(gfp, 0);
 			if (!pages[i])
 				goto error;
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Douglas Anderson]

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@chromium.org>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.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
+-------------------
+
+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
+  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.
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,
 };
 
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Robin Murphy]

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@chromium.org>
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.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,
>   };
>
>

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Doug Anderson]

Hi,

On Fri, Jan 8, 2016 at 5:10 AM, Robin Murphy <robin.murphy@arm.com> wrote:
>> +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.

Done.  I'm running out a paint, so crossing my fingers that this is
the final color.  ;)


>> +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?

Done.


>> +  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).

Hmm.  Well, the original ambiguity was sorta intentional.
Specifically anyone accessing this data through an MMU is likely to
have a TLB and allocating large chunks is more likely to increase the
efficiency of that TLB.  If Linux today can't manage to take advantage
of these large chunks to optimize CPU TLB efficiency that's not really
something I think we need to take into account in the API.  The API
should be OK even as Linux changes if possible...

If we happen to have no MMU at all between the DMA device and the
memory then presumably it need to be totally contiguous.  That would
be OK.  Presumably if the client knew that there was no MMU it would
specify DMA_ATTR_FORCE_CONTIGUOUS anyway...  ...and if the DMA
subsubsystem wanted to make things contiguous despite the "no huge
page" hint that doesn't violate the hint--it is legal to ignore it.


I'm really just a visitor to the DMA subsystem, though.  I would up
here down the rabbit hole of chasing down a bug.  If I'm totally
misunderstanding something please correct me.  ;)


-Doug

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Russell King - ARM Linux]

On Thu, Jan 07, 2016 at 04:36:44PM -0800, Douglas Anderson wrote:
> This patch adds the DMA_ATTR_NOHUGEPAGE attribute to the DMA-mapping
> subsystem.

It would be nicer to keep things in the positive-logic sense if at all
possible: flags that indicate "we don't want something" tend to end up
with double or triple negatives somewhere which makes understanding
the code much harder.  It's a shame we have MADV_NOHUGEPAGE...

That's not a strong view, but a preference.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Doug Anderson]

Hi,

On Fri, Jan 8, 2016 at 5:35 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 07, 2016 at 04:36:44PM -0800, Douglas Anderson wrote:
>> This patch adds the DMA_ATTR_NOHUGEPAGE attribute to the DMA-mapping
>> subsystem.
>
> It would be nicer to keep things in the positive-logic sense if at all
> possible: flags that indicate "we don't want something" tend to end up
> with double or triple negatives somewhere which makes understanding
> the code much harder.  It's a shame we have MADV_NOHUGEPAGE...
>
> That's not a strong view, but a preference.

Thanks for your thoughts.  I agree that double-negatives can be confusing...

IMHO There are two problems with changing to DMA_ATTR_HUGE_PAGE, though:

1. I have to go and touch all existing DMA-mapping code to set
DMA_ATTR_HUGE_PAGE.  That will be a big patchset and touch more code,
making it more likely to break something.  If I have to do that I can,
but I prefer not to because changing defaults like this tends to make
for subtle bugs.  One other thing to think about is that it's my
understanding that a large chunk of the ARM developers out there are
working on various differing versions of the kernel.  If you've got
drivers in your tree that haven't been patched to account for the new
default but you pick my patch you won't get a compile time
error--you'll get a very subtle performance regression.  Maybe we
don't care too much about those out-of-tree and old kernel folks, but
it's something to think about.

2. Personally I think of this attribute like a tristate with 3 values:

A) Do whatever you think best.  Maybe allocate huge pages if it's
super easy and there are lots of huge pages around.  AKA similar to
today's behavior.

B) Try extra hard to get huge pages.  Maybe do some extra sorting of
pages to build up big ones.  Maybe do a little extra collection.
Something like that.

C) Don't waste even en extra CPU cycle getting huge pages.  I don't need them.

Encoding a tristate with bitfields basically means you need two
opposite bit definitions: DMA_ATTR_NO_HUGE_PAGE and DMA_ATTR_HUGE_PAGE
(and specifying both is an error case).

Right now I only need two states of the theoretical tristate: A) and
C).  I could add "DMA_ATTR_HUGE_PAGE" in my patch, but I have no
patches queued up to use it.  As Christoph Hellwig has reinforced in
his reply, folks tend not to want unused code in the kernel, so my
thoughts are that someone can add this second attribute when they have
a use for it.


-Doug

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Russell King - ARM Linux]

On Fri, Jan 08, 2016 at 03:05:13PM -0800, Doug Anderson wrote:
> 1. I have to go and touch all existing DMA-mapping code to set
> DMA_ATTR_HUGE_PAGE.  That will be a big patchset and touch more code,
> making it more likely to break something.
...

Indeed, I was actually thinking of a positive "prefer/only use/force
smaller pages" thing rather than "allow huge pages" as a way to get
rid of the "no huge pages" negative as a way to get around that.
It has the same meaning when set as DMA_ATTR_NO_HUGE_PAGE but
avoids the problem of wondering what

	!dma_get_attr(DMA_ATTR_NO_HUGE_PAGE, attrs)

means.

I wasn't thinking of DMA_ATTR_HUGE_PAGE as that would certainly be
wrong when CONFIG_HAVE_DMA_ATTRS is disabled (when dma_get_attr()
always returns 0.)

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Doug Anderson]

Russell,

On Fri, Jan 8, 2016 at 3:18 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 08, 2016 at 03:05:13PM -0800, Doug Anderson wrote:
>> 1. I have to go and touch all existing DMA-mapping code to set
>> DMA_ATTR_HUGE_PAGE.  That will be a big patchset and touch more code,
>> making it more likely to break something.
> ...
>
> Indeed, I was actually thinking of a positive "prefer/only use/force
> smaller pages" thing rather than "allow huge pages" as a way to get
> rid of the "no huge pages" negative as a way to get around that.
> It has the same meaning when set as DMA_ATTR_NO_HUGE_PAGE but
> avoids the problem of wondering what
>
>         !dma_get_attr(DMA_ATTR_NO_HUGE_PAGE, attrs)
>
> means.
>
> I wasn't thinking of DMA_ATTR_HUGE_PAGE as that would certainly be
> wrong when CONFIG_HAVE_DMA_ATTRS is disabled (when dma_get_attr()
> always returns 0.)

Ah, that makes so much more sense now!  :)  So you were suggesting
something like DMA_ATTR_SMALL_PAGES_OK.  Then you if we wanted all
possible states you'd have 0 vs. DMA_ATTR_SMALL_PAGES_OK vs.
DMA_ATTR_HUGE_PAGE?  That would avoid the double-negative but does
have the downside that it's less obvious that DMA_ATTR_SMALL_PAGES_OK
is the opposite of DMA_ATTR_HUGE_PAGE.


I think I still have a bit of a bias towards matching the MADV API,
but I also am happy to change things if that's what people want.  How
about if I see other people chiming in saying that they'd prefer
something like "DMA_ATTR_SMALL_PAGES_OK" then I'll change it,
otherwise I'll leave it as-is (since you said you didn't have a strong
opinion on it).


-Doug

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Christoph Hellwig]

On Fri, Jan 08, 2016 at 03:31:29PM -0800, Doug Anderson wrote:
> Ah, that makes so much more sense now!  :)  So you were suggesting
> something like DMA_ATTR_SMALL_PAGES_OK.  Then you if we wanted all
> possible states you'd have 0 vs. DMA_ATTR_SMALL_PAGES_OK vs.
> DMA_ATTR_HUGE_PAGE?  That would avoid the double-negative but does
> have the downside that it's less obvious that DMA_ATTR_SMALL_PAGES_OK
> is the opposite of DMA_ATTR_HUGE_PAGE.

or DMA_ATTR_4K_PAGES if that's what you want.  We have at least 4k, 8k,
16k and 64k page support in the kernel, not sure if 32k and 256k ever
made it mainline.  What does your hardware actually require?

This needs to be documented properly and hpefully also reflected in the
name of the fag.  Otherwise we'll end up with a giant desaster like
GFP_DMA that means something slightly different on every architecture.

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Tomasz Figa]

On Sat, Jan 9, 2016 at 4:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Jan 08, 2016 at 03:31:29PM -0800, Doug Anderson wrote:
>> Ah, that makes so much more sense now!  :)  So you were suggesting
>> something like DMA_ATTR_SMALL_PAGES_OK.  Then you if we wanted all
>> possible states you'd have 0 vs. DMA_ATTR_SMALL_PAGES_OK vs.
>> DMA_ATTR_HUGE_PAGE?  That would avoid the double-negative but does
>> have the downside that it's less obvious that DMA_ATTR_SMALL_PAGES_OK
>> is the opposite of DMA_ATTR_HUGE_PAGE.
>
> or DMA_ATTR_4K_PAGES if that's what you want.

Or maybe just DMA_ATTR_USE_SMALL_PAGES?

> We have at least 4k, 8k,
> 16k and 64k page support in the kernel, not sure if 32k and 256k ever
> made it mainline.  What does your hardware actually require?

The hardware in question doesn't require any specific page size - the
IOMMU can map anything in granularity of 4KiB, which I believe
corresponds to the small page size on its platform (4KiB, ARM). Also
basically this attribute would translate into alloc_pages() with zero
order, so I guess that means small pages. (or we could perhaps make it
DMA_ATTR_ALLOC_SINGLE_PAGES...)

>
> This needs to be documented properly and hpefully also reflected in the
> name of the fag.  Otherwise we'll end up with a giant desaster like
> GFP_DMA that means something slightly different on every architecture.

Agreed.

Best regards,
Tomasz

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Christoph Hellwig]

On Thu, Jan 07, 2016 at 04:36:44PM -0800, 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.

A user of this features seems to be missing in the series.  Please don't
add any clutter with unclear usage to the kernel unless there is a real
need which can be deonstrated in form of patches and numbers.

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Doug Anderson]

Hi,

On Fri, Jan 8, 2016 at 5:42 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Jan 07, 2016 at 04:36:44PM -0800, 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.
>
> A user of this features seems to be missing in the series.  Please don't
> add any clutter with unclear usage to the kernel unless there is a real
> need which can be deonstrated in form of patches and numbers.

In my cover letter I tried to address this.  See
<http://comments.gmane.org/gmane.linux.kernel/2121364>.  I said:

> Notably missing from this series is the fourth patch that adds teeth to
> the second and third.  You can find that out of tree at
> <https://chromium-review.googlesource.com/#/c/320498/>.  Unfortunately
> the rk3288_vpu, which is what I'm working on, is out of tree.

...but today I realized that I also needed to do work to get Exynos's
MFC codec using this patch and MFC _is_ upstream.  ...so my next
series will include the MFC patch.  Hopefully that will help address
your concerns.

Re: [PATCH v4 2/3] common: DMA-mapping: add DMA_ATTR_NOHUGEPAGE attribute

[Christoph Hellwig]

On Fri, Jan 08, 2016 at 03:05:23PM -0800, Doug Anderson wrote:
> In my cover letter I tried to address this.  See
> <http://comments.gmane.org/gmane.linux.kernel/2121364>.  I said:
> 
> > Notably missing from this series is the fourth patch that adds teeth to
> > the second and third.  You can find that out of tree at
> > <https://chromium-review.googlesource.com/#/c/320498/>.  Unfortunately
> > the rk3288_vpu, which is what I'm working on, is out of tree.

Yes, I read that.  And that's exactly why I said we should not add
crazy workarounds for code that's not even in the kernel.

> ...but today I realized that I also needed to do work to get Exynos's
> MFC codec using this patch and MFC _is_ upstream.  ...so my next
> series will include the MFC patch.  Hopefully that will help address
> your concerns.

Ok.

[PATCH v4 3/3] ARM: dma-mapping: Use DMA_ATTR_NOHUGEPAGE hint to optimize allocation

[Douglas Anderson]

If we know that TLB efficiency will not be an issue when memory is
accessed then it's not terribly important to allocate big chunks of
memory.  The whole point of allocating the big chunks was that it would
make TLB usage efficient.

As Marek Szyprowski indicated:
    Please note that mapping memory with larger pages significantly
    improves performance, especially when IOMMU has a little TLB
    cache. This can be easily observed when multimedia devices do
    processing of RGB data with 90/270 degree rotation
Image rotation is distinctly an operation that needs to bounce around
through memory, so it makes sense that TLB efficiency is important
there.

Video decoding, on the other hand, is a fairly sequential operation.
During video decoding it's not expected that we'll be jumping all over
memory.  Decoding video is also pretty heavy and the TLB misses aren't a
huge deal.  Presumably most HW video acceleration users of dma-mapping
will not care about huge pages and will set DMA_ATTR_NOHUGEPAGE.

Allocating big chunks of memory is quite expensive, especially if we're
doing it repeadly and memory is full.  In one (out of tree) usage model
it is common that arm_iommu_alloc_attrs() is called 16 times in a row,
each one trying to allocate 4 MB of memory.  This is called whenever the
system encounters a new video, which could easily happen while the
memory system is stressed out.  In fact, on certain social media
websites that auto-play video and have infinite scrolling, it's quite
common to see not just one of these 16x4MB allocations but 2 or 3 right
after another.  Asking the system even to do a small amount of extra
work to give us big chunks in this case is just not a good use of time.

Allocating big chunks of memory is also expensive indirectly.  Even if
we ask the system not to do ANY extra work to allocate _our_ memory,
we're still potentially eating up all big chunks in the system.
Presumably there are other users in the system that aren't quite as
flexible and that actually need these big chunks.  By eating all the big
chunks we're causing extra work for the rest of the system.  We also may
start making other memory allocations fail.  While the system may be
robust to such failures (as is the case with dwc2 USB trying to allocate
buffers for Ethernet data and with WiFi trying to allocate buffers for
WiFi data), it is yet another big performance hit.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Changes in v4:
- renamed DMA_ATTR_SEQUENTIAL to DMA_ATTR_NOHUGEPAGE
- added Marek's ack

Changes in v3:
- Use DMA_ATTR_SEQUENTIAL hint patch new for v3.

Changes in v2: None

 arch/arm/mm/dma-mapping.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index bc9cebfa0891..96d71bcb4c3a 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1158,6 +1158,10 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 		return pages;
 	}
 
+	/* Go straight to 4K chunks if caller says it's OK. */
+	if (dma_get_attr(DMA_ATTR_NOHUGEPAGE, attrs))
+		order_idx = ARRAY_SIZE(iommu_order_array) - 1;
+
 	/*
 	 * IOMMU can map any pages, so himem can also be used here
 	 */
-- 
2.6.0.rc2.230.g3dd15c0