Skip to contentSkip to conversation

generic device DMA implementation

RFC Adam J. Richter 5 participants 7 messages Dec 6, 2002 – Dec 8, 2002

lore
Share & tools
lore

Adam J. Richter

author
#0 lore raw
Quoted 1 line · David Gibson

On Thu, Dec 05, 2002 at 06:08:22PM -0800, Adam J. Richter wrote:

[...]

Quoted 10 lines
Quoted 9 lines
	In linux-2.5.50/drivers/net/lasi_82596.c, the macros

CHECK_{WBACK,INV,WBACK_INV} have definitions like:

#define CHECK_WBACK(addr,len) \
do { if (!dma_consistent) dma_cache_wback((unsigned long)addr,len); } while (0)

These macros are even used in IO paths like i596_rx(). The
"if()" statement in each of these macros is the extra branch that disappears on most architectures under James's proposal.

Quoted 8 lines

Erm... I have no problem with the macros that James's proposal would use to take away this branch - I would expect to use exactly the same ones. It's just the notion of "try to get consistent memory, but get me any old memory otherwise" that I'm not so convinced by.

In any case, on platforms where the dma_malloc() could really return either consistent or non-consistent memory, James's sync macros would have to have an equivalent branch within.

Yeah, I should have said "because then you don't have to have a
branch for the case where the platform always or *never* returns consistent memory on a give machine."

Quoted 6 lines
Quoted 5 lines
Quoted 1 line

What performance advantages of consistent memory?

[...] For
example, pci_sync_single is 55 lines of C code in
linux-2.5.50/arch/sparc64/kernel/pci_iommu.c.

Quoted 6 lines

Hmm... fair enough. Ok, I can see the point of a fall back to non-consistent approach given that. So I guess the idea makes sense, so long as dma_malloc() (without the consistent flag) is taken to be "give me DMAable memory, consistent or not, whichever is cheaper for this platform" rather than "give me DMAable memory, consistent if possible". It was originally presented as the latter which misled me.

As long as dma_sync_maybe works with the addresses returned by
dma_malloc and dma_malloc only returns the types of memory that the callers claims to be prepared to deal with, the decision about what kind of memory dma_malloc should return when it has a choice is up to the platform implementation.

Quoted 2 lines

I think the change to the parameters which I suggested in a reply to James makes this a bit clearer.

I previously suggested some of the changes in your description:
name them dma_{malloc,free} (which James basically agrees with), have a flags field. However, given that it's a parameter and you're going to pass a constant symbol like DMA_CONSISTENT or DMA_INCONSISTENT to it, it doesn't really matter if its an enum or an int to start with, as it could be changed later with minimal or zero driver changes.

I like your term DMA_CONSISTENT better than
DMA_CONFORMANCE_CONSISTANT. I think the word "conformance" in there does not reduce the time that it takes to figure out what the symbol means. I don't think any other facility will want to use the terms DMA_{,IN}CONSISTENT, so I prefer that we go with the more medium sized symbol.

Naming the parameter to dma_malloc "bus" would imply that it
will not look at individual device information like dma_mask, which is wrong. Putting the flags field in the middle of the parameter list will make the dma_malloc and dma_free lists unnnecessarily different. I think these two were just oversights in your posting.

Anyhow, I think we're in full agreement at this point on the
substantive stuff at this point.

Adam J. Richter     __     ______________   575 Oroville Road
[email protected]     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America

"Free Software For The Rest Of Us."

#1 lore raw
Quoted 5 lines · [email protected] said:

I like your term DMA_CONSISTENT better than DMA_CONFORMANCE_CONSISTANT
. I think the word "conformance" in there does not reduce the time that it takes to figure out what the symbol means. I don't think any other facility will want to use the terms DMA_{,IN}CONSISTENT, so I prefer that we go with the more medium sized symbol.

I'm not so keen on this. The idea of this parameter is not to tell the allocation routine what type of memory you would like, but to tell it what type of memory the driver can cope with. I think for the inconsistent case, DMA_INCONSISTENT looks like the driver is requiring inconsistent memory, and expecting to get it. I'm open to changing the "CONFORMANCE" part, but I'd like to name these parameters something that doesn't imply they're requesting a type of memory.

James

#2 lore raw
Quoted 5 lines · James Bottomley

I'm not so keen on this. The idea of this parameter is not to tell the allocation routine what type of memory you would like, but to tell it what type of memory the driver can cope with. I think for the inconsistent case, DMA_INCONSISTENT looks like the driver is requiring inconsistent memory, and expecting to get it.

Of course if they're flags, then `DMA_CONSISTENT | DMA_INCONSISTENT' is pretty obvious...

-Miles

Signature · 2 lines
P.S.  All information contained in the above letter is false,
      for reasons of military security.
#3 lore raw
In reply to James Bottomley
Quoted 7 lines · James Bottomley
Quoted 5 lines · [email protected] said:

I like your term DMA_CONSISTENT better than DMA_CONFORMANCE_CONSISTANT
. I think the word "conformance" in there does not reduce the time that it takes to figure out what the symbol means. I don't think any other facility will want to use the terms DMA_{,IN}CONSISTENT, so I prefer that we go with the more medium sized symbol.

Quoted 8 lines

I'm not so keen on this. The idea of this parameter is not to tell the allocation routine what type of memory you would like, but to tell it what type of memory the driver can cope with. I think for the inconsistent case, DMA_INCONSISTENT looks like the driver is requiring inconsistent memory, and expecting to get it. I'm open to changing the "CONFORMANCE" part, but I'd like to name these parameters something that doesn't imply they're requesting a type of memory.

Well, actually I was thinking of the flags as a bitmask, not an enum, so I was assuming (flags==0) for not-neccessarily-consistent memory. However, since having seen davem's comments, I agree with him that separate entry points is probably a better idea for API sanity.

Signature · 4 lines
David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson
#4 lore raw
In reply to Adam J. Richter
Quoted 18 lines · Adam J. Richter
Quoted 1 line · David Gibson

On Thu, Dec 05, 2002 at 06:08:22PM -0800, Adam J. Richter wrote:

[...]

Quoted 15 lines
Quoted 9 lines
	In linux-2.5.50/drivers/net/lasi_82596.c, the macros

CHECK_{WBACK,INV,WBACK_INV} have definitions like:

#define CHECK_WBACK(addr,len) \
do { if (!dma_consistent) dma_cache_wback((unsigned long)addr,len); } while (0)

These macros are even used in IO paths like i596_rx(). The
"if()" statement in each of these macros is the extra branch that disappears on most architectures under James's proposal.

Erm... I have no problem with the macros that James's proposal would use to take away this branch - I would expect to use exactly the same ones. It's just the notion of "try to get consistent memory, but get me any old memory otherwise" that I'm not so convinced by.

Quoted 7 lines
Quoted 3 lines

In any case, on platforms where the dma_malloc() could really return either consistent or non-consistent memory, James's sync macros would have to have an equivalent branch within.

Yeah, I should have said "because then you don't have to have a
branch for the case where the platform always or *never* returns consistent memory on a give machine."

Actually, no, since my idea was to remove the "consistent_alloc()" path from the driver entirely - leaving only the map/sync approach. That gives a result which is correct everywhere (afaict) but (as you've since pointed out) will perform poorly on platforms where the map/sync operations are expensive.

Quoted 29 lines
Quoted 6 lines
Quoted 5 lines
Quoted 1 line

What performance advantages of consistent memory?

[...] For
example, pci_sync_single is 55 lines of C code in
linux-2.5.50/arch/sparc64/kernel/pci_iommu.c.

Quoted 6 lines

Hmm... fair enough. Ok, I can see the point of a fall back to non-consistent approach given that. So I guess the idea makes sense, so long as dma_malloc() (without the consistent flag) is taken to be "give me DMAable memory, consistent or not, whichever is cheaper for this platform" rather than "give me DMAable memory, consistent if possible". It was originally presented as the latter which misled me.

As long as dma_sync_maybe works with the addresses returned by
dma_malloc and dma_malloc only returns the types of memory that the callers claims to be prepared to deal with, the decision about what kind of memory dma_malloc should return when it has a choice is up to the platform implementation.

Quoted 2 lines

I think the change to the parameters which I suggested in a reply to James makes this a bit clearer.

I previously suggested some of the changes in your description:
name them dma_{malloc,free} (which James basically agrees with), have a flags field. However, given that it's a parameter and you're going to pass a constant symbol like DMA_CONSISTENT or DMA_INCONSISTENT to it, it doesn't really matter if its an enum or an int to start with, as it could be changed later with minimal or zero driver changes.

Quoted 6 lines

I like your term DMA_CONSISTENT better than
DMA_CONFORMANCE_CONSISTANT. I think the word "conformance" in there does not reduce the time that it takes to figure out what the symbol means. I don't think any other facility will want to use the terms DMA_{,IN}CONSISTENT, so I prefer that we go with the more medium sized symbol.

Actually I think the "conformance" is actively misleading, since (to me) it implies that the function is always "trying" to get consistent memory, which it really isn't.

Quoted 5 lines

Naming the parameter to dma_malloc "bus" would imply that it
will not look at individual device information like dma_mask, which is wrong. Putting the flags field in the middle of the parameter list will make the dma_malloc and dma_free lists unnnecessarily different. I think these two were just oversights in your posting.

Yes indeed.

Signature · 4 lines
David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson
#5 lore raw
Quoted 5 lines · David Gibson

Actually, no, since my idea was to remove the "consistent_alloc()" path from the driver entirely - leaving only the map/sync approach. That gives a result which is correct everywhere (afaict) but (as you've since pointed out) will perform poorly on platforms where the map/sync operations are expensive.

As I've also pointed out in the past couple of days, doing this will mean that you then need to teach the drivers to align structures to cache line boundaries. Otherwise, you _will_ get into a situation where you _will_ loose data.

One such illustration of this is the tulip driver, with an array of 16-byte control/status blocks on a machine with a 32-byte cache line size.

I would rather keep the consistent_alloc() approach for allocating consistent memory, and align structures as they see fit, rather than having to teach the drivers to align appropriately. And you can be damned sure that driver writers are _not_ going to get the alignment right.

Signature · 2 lines
Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html
#6 lore raw
Quoted 11 lines · Russell King
Quoted 5 lines · David Gibson

Actually, no, since my idea was to remove the "consistent_alloc()" path from the driver entirely - leaving only the map/sync approach. That gives a result which is correct everywhere (afaict) but (as you've since pointed out) will perform poorly on platforms where the map/sync operations are expensive.

As I've also pointed out in the past couple of days, doing this will mean that you then need to teach the drivers to align structures to cache line boundaries. Otherwise, you _will_ get into a situation where you _will_ loose data.

Sure, but that's already an issue with the current streaming DMA API.

Signature · 4 lines
David Gibson			| For every complex problem there is a
[email protected]	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson