generic device DMA implementation
Review
Quoted 7 lines
(quoted diff) +4
-0+ +Consistent memory is memory for which a write by either the device or +the processor can immediately be read by the processor or device +without having to worry about caching effects.
This is not entirely correct:
The driver must use the normal memory barrier instructions even in coherent memory. Could you copy the section about wmb() from DMA-mapping into your new documentation?
+
+Warnings: Memory coherency operates at a granularity called the cache +line width. In order for memory mapped by this API to operate +correctly, the mapped region must begin exactly on a cache line +boundary and end exactly on one (to prevent two separately mapped +regions from sharing a single cache line). Since the cache line size +may not be known at compile time, the API will not enforce this +requirement. Therefore, it is recommended that driver writers who +don't take special care to determine the cache line size at run time +only map virtual regions that begin and end on page boundaries (which +are guaranteed also to be cache line boundaries).
+
Noone obeys that rule, and it's not trivial to fix it.
- kmalloc (32,GFP_KERNEL) returns a 32-byte object, even if the cache line size is 128 bytes. The 4 objects in the cache line could be used by four different users.
- sendfile() with an odd offset.
Is it really impossible to work around that in the platform specific code? In the worst case, the arch code could memcopy to/from a cacheline aligned buffer.
--
Manfred
Quoted 3 lines · [email protected] said:
This is not entirely correct: The driver must use the normal memory barrier instructions even in coherent memory. Could you copy the section about
wmb()from DMA-mapping into your new documentation?
I made the name change from consistent to coherent (at David Mosberger's) request to address at least some of this.
I suppose I can add it as a note to dma_alloc_coherent too.
Quoted 1 line
Noone obeys that rule, and it's not trivial to fix it.
Any driver that disobeys this rule today with the pci_ API is prone to cache related corruption on non-coherent architectures.
Quoted 3 lines
Is it really impossible to work around that in the platform specific code? In the worst case, the arch code could memcopy to/from a cacheline aligned buffer.
Well, it's not impossible, but I don't believe it can be done efficiently. And since it can't be done efficiently, I don't believe it's right to impact the drivers that are properly written to take caching effects into account.
Isn't the better solution to let the platform maintainers negotiate with the driver maintainers to get those drivers they care about fixed?
James
Quoted 7 lines · James Bottomley
Quoted 3 lines
Noone obeys that rule, and it's not trivial to fix it.
Any driver that disobeys this rule today with the
pci_API is prone to cache related corruption on non-coherent architectures.
The networking core disobeys the rule in the sendfile implementation. Depending on the cacheline size, even small TCP packets might disobey the rule. The problem is not restricted to drivers.
Quoted 8 lines
Quoted 7 lines
Is it really impossible to work around that in the platform specific code? In the worst case, the arch code could memcopy to/from a cacheline aligned buffer.
Quoted 5 lines
Well, it's not impossible, but I don't believe it can be done efficiently. And since it can't be done efficiently, I don't believe it's right to impact the drivers that are properly written to take caching effects into account.
Isn't the better solution to let the platform maintainers negotiate with the driver maintainers to get those drivers they care about fixed?
I agree that the performance will be bad, but like misaligned memory access, the arch code should support it. Leave the warning about bad performance in the documentation, and modify the drivers where it actually matters. Your new documentation disagrees with the current implementation, and that is just wrong.
And in the case of networking, someone must do the double buffering. Doing it within dma_map_single() would avoid modifying every pci driver.
--
Manfred
Quoted 2 lines · [email protected] said:
Your new documentation disagrees with the current implementation, and that is just wrong.
I don't agree that protecting users from cache line overlap misuse is current implementation. It's certainly not on parisc which was the non-coherent platform I chose to model this with, which platforms do it now for the pci_ API?
James
Quoted 11 lines · James Bottomley
Quoted 4 lines · [email protected] said:
Your new documentation disagrees with the current implementation, and that is just wrong.
I don't agree that protecting users from cache line overlap misuse is current implementation. It's certainly not on parisc which was the non-coherent platform I chose to model this with, which platforms do it now for the
pci_API?
You are aware that "users" is not one or two drivers that noone uses, it's the whole networking stack.
What do you propose to fix sendfile() and networking with small network packets [e.g. two 64 byte packets within a 128 byte cache line]?
One platforms that handles it is Miles Bader's memcopy based dma_map_single() implementation.
http://marc.theaimsgroup.com/?l=linux-kernel&m=103907087825616&w=2
And obviously i386, i.e. all archs with empty dma_map_single() functions.
I see three options:
- modify the networking core, and enforce that a cache line is never shared between users for such archs. Big change. Often not necessary - some nics must double buffer internally anyway.
- modify every driver that doesn't do double buffering, and enable double buffering on the affected archs. Even larger change.
- do the double buffering in dma_map_single() & co.
One problem for double buffering in dma_map_single() is that it would double buffer too often: for example, the start of the rx buffers is usually misaligned by the driver, to ensure that the IP headers are aligned. The rest of the cacheline is unused, but it's not possible to give that information to dma_map_single().
--
Manfred
Quoted 2 lines · [email protected] said:
You are aware that "users" is not one or two drivers that noone uses, it's the whole networking stack.
I am aware of this. I'm also aware that it is *currently* broken with the old API on all non-coherent arch's bar the one you point out.
All I actually did was document the existing problem, I think.
How bad actually is it? Networking seems to work fine for me on non-coherent parisc. Whereas, when I had this cache line overlap problem in a SCSI driver, I was seeing corruption all over the place.
The problem really only occurs if the CPU can modify part of a cache line while a device has modified memory belonging to another part. Now a flush from the CPU will destroy the device data (or an invalidate from the driver destroy the CPU's data). The problem is effectively rendered harmless if only data going in the same direction shares a cache line (even if it is for different devices). It strikes me that this is probably true for network data and would explain the fact that I haven't seen any obvious network related corruption.
James
Quoted 3 lines · James Bottomley
The problem really only occurs if the CPU can modify part of a cache line while a device has modified memory belonging to another part. Now a flush from the CPU will destroy the device data (or an invalidate from the driver destroy the CPU's data). The problem is effectively rendered harmless if only data going in the same direction shares a cache line (even if it is for different devices). It strikes me that this is probably true for network data and would explain the fact that I haven't seen any obvious network related corruption.
Yes. Networking usually generates exclusive cachelines.
I'm aware of two special cases:
If multiple kmalloc buffers fit into one cacheline, then it can happen all the time. But the smallest kmalloc buffer is 64 bytes [assuming page size > 4096]. Is your cache line >= 128 bytes?
Or sendfile() of a mmap'ed file that is modified by userspace. That is the recommended approach for zerocopy tx, but I'm not sure which apps actually use that. IIRC DaveM mentioned the approach.
Additionally, the TCP checksum could catch the corruption and resent the packet - you wouldn't notice the corruptions, unless you use hw checksums.
--
Manfred
Quoted 3 lines · [email protected] said:
If multiple kmalloc buffers fit into one cacheline, then it can happen
all the time. But the smallest kmalloc buffer is 64 bytes [assuming
page size > 4096].
Actually, I did forget to mention that on parisc non-coherent, the minimum kmalloc allocation is the cache line width, so that problem cannot occur.
Hmm, perhaps that is an easier (and faster) approach to fixing the problems on non-coherent platforms?
James
Quoted 9 lines · James Bottomley
Quoted 5 lines · [email protected] said:
If multiple kmalloc buffers fit into one cacheline, then it can happen all the time. But the smallest kmalloc buffer is 64 bytes [assuming page size > 4096].
Quoted 5 lines
Actually, I did forget to mention that on parisc non-coherent, the minimum kmalloc allocation is the cache line width, so that problem cannot occur.
Hmm, perhaps that is an easier (and faster) approach to fixing the problems on non-coherent platforms?
How do you want to fix sendfile()?
Note that I'm thinking along the same line as reading an unaligned integer: the arch must provide a trap handler that emulates misaligned reads, but it should never happen, except if someone manually creates an IP packet with odd options to perform an DoS attack. Restricting kmalloc is obviously faster, but I'm not convinced that this really catches all corner cases.
A memcpy() based dma_map implementation would have another advantage: enable it on i386, and you'll catch everyone that violates the dma spec immediately.
The only problem is that the API is bad - networking buffers are usually 2 kB allocations, 2 kB aligned.
The actual data area that is passed to pci_map_single starts at offset 2 and has an odd length. It's not possible to pass the information to dma_map_single() that the rest of the cacheline is unused and that double buffering is not required, despite the misaligned case. Except for sendfile(), then double buffering is necessary.
--
Manfred