PCI DMA to small buffers on cache-incoherent arch

I recently fixed some problems in the 2.4 USB stack that caused crashes on the PowerPC 440GP (and probably other cache-incoherent architectures). However, some parts of my fix have raised some questions on the linux-usb-devel and linuxppc-embedded mailing lists and I would like to raise these issues here so that we can get a definitive answer. I would especially appreciate comments from David Miller and other PCI DMA experts.

The problem that caused crashes on cache-incoherent architectures (my specific system uses a PPC 440GP but this should apply in general) was the following. The USB stack was doing PCI DMA into buffers that were allocated on the stack, which causes stack corruption: on the PPC 440GP, pci_map_single() with PCI_DMA_FROMDEVICE just invalidates the cache for the region being mapped. However, if a buffer is smaller than a cache line, then two bad things can happen.

First, there may be valid data outside the buffer but in the same cache line that has not been flushed to main memory yet. In that case when the cache is invalidated the new data is lost and any access to that memory will get the old data from main memory. Second, access to the cache line after the cache has been invalidated but before the DMA has completed will pull the cache line back into processor cache and the DMA buffer will have invalid data.

The solution to this was simply to use kmalloc() to allocate buffers instead of using automatic variables. On the PPC 440GP this is definitely safe because the 440GP's has 32 byte cache lines and kmalloc() will never return a buffer that is smaller than 32 bytes or not 32-byte aligned. However, this leads to my first question: is this safe on all architectures? Could there be a cache-incoherent architecture where kmalloc() returned a buffer smaller than a cache line?

The second question is related to this. There are other parts of the USB stack where structures are defined:

	struct something {
	        /* ... some members ... */
	        char buffer[SMALLER_THAN_L1_CACHE_LINE_SIZE];
	        /* ... some more members ... */
	};

Then a struct something is kmalloc()'ed and buffer is used to DMA into. However, even though the overall structure is aligned on a cache line boundary, buffer is not aligned. It seems to me that this is not safe because access to members near buffer during a DMA could cause corruption (as detailed above). In my patch I changed code like this to look like

	struct something {
	        /* ... some members ... */
	        char *buffer;
	        /* ... some more members ... */
	};

and then kmalloc()'ed buffer separately when kmalloc()'ing a struct something.

However, there is some question about whether changing these buffers is really necessary. Code like the above doesn't cause immediately obvious crashes the way buffers on the stack do (since we usually get lucky and don't touch the members near buffer around the DMA access). I felt that relying on this coincidence is not safe and should be fixed at the same time.

DMA-mapping.txt says kmalloc()'ed memory is OK for DMAs and does not mention cache alignment. So the question is: did I misunderstand the cache coherency issues or is my patch correct? At a higher level: how is this supposed to work? Should code doing DMA into a smallish buffer do

        buffer = kmalloc(max(BUFFER_SIZE, L1_CACHE_BYTES), GFP_XXX);

or is a kmalloc()'ed buffer always safe? Does the code doing DMA need to worry about the cache alignment of its buffer?

In any case this probably needs to be documented better. Once I understand the answer I'll write up a patch to DMA-mapping.txt so no one has to rediscover all this.

Thanks,
Roland

Yes, DMAing to the stack is definitely a bug, thats mentioned in Documentation/DMA-mapping.txt. We used to vmalloc our kernel stacks on ppc64 and that picked up all sorts of DMA violations.

I just checked 2.5 and noticed the scsi code is _still_ DMAing to the stack! Maybe it would be worth having a debug option for x86 where it uses vmalloc for kernel stack allocation :)

Anyway attached is a patch from Todd Inglett that I updated for 2.5.

Anton

===== drivers/scsi/scsi_scan.c 1.13 vs edited =====

@@ -368,7 +368,6 @@
 	unsigned int dev;
 	unsigned int lun;
 	unsigned char *scsi_result;
-	unsigned char scsi_result0[256];
 	Scsi_Device *SDpnt;
 	Scsi_Device *SDtail;
 

@@ -390,9 +389,7 @@
 		 */
 		scsi_initialize_queue(SDpnt, shpnt);
 		SDpnt->request_queue.queuedata = (void *) SDpnt;
-		/* Make sure we have something that is valid for DMA purposes */
-		scsi_result = ((!shpnt->unchecked_isa_dma)
-			       ? &scsi_result0[0] : kmalloc(512, GFP_DMA));
+		scsi_result = kmalloc(512, GFP_DMA);
 	}
 
 	if (scsi_result == NULL) {

@@ -532,10 +529,9 @@
 		kfree((char *) SDpnt);
 	}
 
-	/* If we allocated a buffer so we could do DMA, free it now */
-	if (scsi_result != &scsi_result0[0] && scsi_result != NULL) {
-		kfree(scsi_result);
-	} {
+	kfree(scsi_result);
+
+	{
 		Scsi_Device *sdev;
 		Scsi_Cmnd *scmd;
 

===== drivers/scsi/sr_ioctl.c 1.13 vs edited =====

@@ -344,7 +344,12 @@
 	Scsi_CD *SCp = cdi->handle;
 	u_char sr_cmd[10];
 	int result, target = minor(cdi->dev);
-	unsigned char buffer[32];
+	unsigned char *buffer = kmalloc(512, GFP_DMA);
+
+	if (buffer == NULL) {
+		printk("SCSI DMA pool exhausted.");
+		return -ENOMEM;
+	}
 
 	memset(sr_cmd, 0, sizeof(sr_cmd));
 

@@ -417,6 +422,7 @@
 		return -EINVAL;
 	}
 
+	kfree(buffer);
 #if 0
 	if (result)
 		printk("DEBUG: sr_audio: result for ioctl %x: %x\n", cmd, result);

From: Roland Dreier <[email protected]>
Date: 08 Jun 2002 13:38:53 -0700

The USB stack was doing PCI DMA into buffers that were allocated on the stack

This is illegal.

which causes stack corruption: on the PPC
440GP, pci_map_single() with PCI_DMA_FROMDEVICE just invalidates the cache for the region being mapped. However, if a buffer is smaller than a cache line, then two bad things can happen.

There is no allocation scheme legal for PCI DMA which gives you smaller than a cacheline of data, this includes SLAB. This is why stack buffers and the like are illegal for PCI DMA.

The solution to this was simply to use kmalloc() to allocate buffers instead of using automatic variables.

Right.

However, this leads to my first question: is this safe on all architectures?

It must be. If the architecture allows SLAB to give smaller than cacheline sized data, it must handle PCI DMA map/unmap flushing in an appropriate fashion (ie. handle sub-cacheline buffers).

DMA-mapping.txt says kmalloc()'ed memory is OK for DMAs and does not mention cache alignment.

It doesn't mention cache alignment because that is an implementation specific issue. The user of the interfaces need not be concerned about any of this.

There need be no changes to the documentation. If you do as the documentation states (use kmalloc or get_free_page to get your buffers) then it will just work.

Franks a lot,
David S. Miller
[email protected]

David> There is no allocation scheme legal for PCI DMA which gives David> you smaller than a cacheline of data, this includes SLAB. David> This is why stack buffers and the like are illegal for PCI David> DMA.

David> If the architecture allows SLAB to give smaller than David> cacheline sized data, it must handle PCI DMA map/unmap David> flushing in an appropriate fashion (ie. handle David> sub-cacheline buffers).

Thanks, that's great information. However, there is one question you didn't cover. How about using a sub-cache-line piece of a kmalloc()'ed buffer (eg the case I described of using one member from a struct as a DMA buffer)? As far as I can see there is no guarantee that this will always work. Do you agree that this should be treated as a bug and fixed when it is found? Or should we leave that usage unless it is observed causing problems (since we almost always get lucky and don't touch the rest of the cache line near the DMA)?

Thanks,
Roland

Anton> I just checked 2.5 and noticed the scsi code is _still_ Anton> DMAing to the stack! Maybe it would be worth having a debug Anton> option for x86 where it uses vmalloc for kernel stack Anton> allocation :)

Good catches, just a few comments on the patch.

Anton> Anyway attached is a patch from Todd Inglett that I updated Anton> for 2.5.

===== drivers/scsi/scsi_scan.c 1.13 vs edited =====
- /* Make sure we have something that is valid for DMA purposes */
- scsi_result = ((!shpnt->unchecked_isa_dma)
- ? &scsi_result0[0] : kmalloc(512, GFP_DMA));
+ scsi_result = kmalloc(512, GFP_DMA);

I don't think you should always use GFP_DMA here... probably better to do:

		scsi_result = kmalloc(512,
                                      shpnt->unchecked_isa_dma ? GFP_DMA : GFP_ATOMIC);

And similarly this:

===== drivers/scsi/sr_ioctl.c 1.13 vs edited =====
+ unsigned char *buffer = kmalloc(512, GFP_DMA);

should probably use GFP_KERNEL (or possibly a check of unchecked_isa_dma as above, though this doesn't seem to be needed based on the existing code).

Best,
Roland

From: Roland Dreier <[email protected]>
Date: 08 Jun 2002 17:40:24 -0700

Or should we leave that usage unless it is observed causing problems (since we almost always get lucky and don't touch the rest of the cache line near the DMA)?

I think passing in a 4 byte chunk and assuming the rest of the cacheline is unmodified is a valid expectation the more I think about it.

This means what MIPS is doing is wrong. For partial cacheline bits it can't do the invalidate thing.

Roland> Or should we leave that usage unless it is observed Roland> causing problems (since we almost always get lucky and Roland> don't touch the rest of the cache line near the DMA)?

David> I think passing in a 4 byte chunk and assuming the rest of David> the cacheline is unmodified is a valid expectation the more David> I think about it.

Just to make sure I'm reading this correctly, you're saying that as long as a buffer is OK for DMA, it should be OK to use a sub-cache-line chunk as a DMA buffer via pci_map_single(), and accessing the rest of the cache line should be OK at any time before, during and after the DMA.

David> This means what MIPS is doing is wrong. For partial David> cacheline bits it can't do the invalidate thing.

If I understand you, this means non-cache-coherent PPC is wrong as well -- pci_map_single() goes through consistent_sync() and turns into:

	case PCI_DMA_FROMDEVICE:	/* invalidate only */
		invalidate_dcache_range(start, end);
		break;

What alternate implementation are you proposing?

Thanks,
Roland

On a non-SMP system, would it be OK to map all the memory without memory coherency enabled? You seem to be implying that one only needs to implement some mechanism in pci_map_single() to handle flushing cache lines (write back, then invalidate).

This would be useful for Macs.

From: Roland Dreier <[email protected]>
Date: 08 Jun 2002 18:26:12 -0700

Just to make sure I'm reading this correctly, you're saying that as long as a buffer is OK for DMA, it should be OK to use a sub-cache-line chunk as a DMA buffer via pci_map_single(), and accessing the rest of the cache line should be OK at any time before, during and after the DMA.

Yes.

David> This means what MIPS is doing is wrong. For partial David> cacheline bits it can't do the invalidate thing.

If I understand you, this means non-cache-coherent PPC is wrong as well -- pci_map_single() goes through consistent_sync() and turns into:

   	case PCI_DMA_FROMDEVICE:	/* invalidate only */
   		invalidate_dcache_range(start, end);
   		break;

What alternate implementation are you proposing?

For non-cacheline aligned chunks in the range "start" to "end" you must perform a cache writeback and invalidate. To preserve the data outside of the DMA range.

From: "Albert D. Cahalan" <[email protected]>
Date: Sat, 8 Jun 2002 21:30:31 -0400 (EDT)

On a non-SMP system, would it be OK to map all the memory without memory coherency enabled? You seem to be implying that one only needs to implement some mechanism in pci_map_single() to handle flushing cache lines (write back, then invalidate).

This would be useful for Macs.

It's just avoiding flushing by effecting flushing the cache after every load/store the cpu does, so of course it would work.

It would be slow as hell, but it would work.

Roland> Just to make sure I'm reading this correctly, you're Roland> saying that as long as a buffer is OK for DMA, it should Roland> be OK to use a sub-cache-line chunk as a DMA buffer via Roland> pci_map_single(), and accessing the rest of the cache line Roland> should be OK at any time before, during and after the DMA.

David> Yes.

Roland> What alternate implementation are you proposing?

David> For non-cacheline aligned chunks in the range "start" to David> "end" you must perform a cache writeback and invalidate. To David> preserve the data outside of the DMA range.

Doesn't this still have a problem if you touch data in the same cache line as the DMA buffer after the pci_map but before the DMA takes place? The CPU will pull the cache line back in and it might not see the data the DMA brought in.

It seems to me that to be totally safe, pci_unmap would have to save the non-aligned part outside the buffer to temporary storage, do an invalidate, and then copy back the non-aligned part.

In any case if this is what pci_map is supposed to do then we have to fix up several architectures at least...

Thanks,
Roland

I don't see why it would have to be slow. Mapping the memory with coherency disabled will reduce bus traffic for all the regular kernel code doing non-DMA stuff. (coherency requires that the CPU generate address-only bus operations)

The obvious concern is that some kernel code might touch a cache line after the invalidate but before the DMA is done. If that could be considered a bug, then every non-SMP Mac could gain some speed by turning off coherency. Maybe the x86 MTRRs allow for something similar.

For device --> memory DMA:

1. write back cache lines that cross unaligned boundries
2. start the DMA
3. invalidate the above cache lines
4. invalidate cache lines that are fully inside the DMA

For memory --> device DMA:

1. write back all cache lines affected by the DMA
2. start the DMA
3. invalidate the above cache lines

Am Sonntag, 9. Juni 2002 07:29 schrieb David S. Miller:

Does this mean that this piece of memory does have to be declared uncacheable until DMA is finished ?
How else do you solve th problem of validity during DMA and especially after DMA ?

Regards
Oliver

Why the third step ? That data should still
be valid.

Regards
Oliver

I made a mistake, but perhaps it is a good one. There is no need to invalidate the cache lines, but I'd guess that commonly the data won't be used again. Doing the invalidate would free up some cache lines, meaning that the CPU would have empty slots to use for other stuff.

Am Sonntag, 9. Juni 2002 08:57 schrieb Albert D. Cahalan:

Then this should be made commonly available and could be used eg. on kfree.
The choice of kicking the data out or not could then be made.

Regards
Oliver

What if some of the cache lines inside the DMA region are dirty and...

get evicted now when the CPU is doing something else?

You really need to:

1. write back cache lines that cross unaligned boundries
3. invalidate the above cache lines
2. start the DMA
4. invalidate cache lines that are fully inside the DMA

which is precisely we do on ARM.

What's the point of (3) ? The data in memory hasn't been changed by DMA. What if we were writing out a page that was mmaped into a process, and the process wrote to that page while we were DMAing ?

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

No. Not on processors where the cache doesn't snoop DMA accesses to memory.

This is the problem scenario. Suppose we are doing DMA to a buffer B and also independently accessing a variable X which is not part of B. Suppose that X and the beginning of B are both in cache line C.

	CPU				I/O device

1. write back & invalidate cache
line(s) containing B
2. start DMA
3. read X: cache line C now
present in cache
4. DMA write to B:
cache line C updated in memory
5. write X: cache line C now
dirty in cache
6. Signal DMA completion

Now at this point the driver calls pci_unmap_single or whatever. What is pci_unmap_single to do? If it does nothing, or does a writeback, we lose the DMA data. If it does an invalidate we lose the value written to X. Clearly, neither is correct.

The bottom line is that we can only have one writer to any given cache line at a time. If a buffer is being used for DMA from a device to memory, the cpu MUST NOT write to any cache line that overlaps the buffer.

Fundamentally, this is because the hardware lacks the ability to transfer the "ownership" of the cache line between the cpu and the DMA device on demand, and so we must manage that in software. The only safe way to allow the cpu to write to the cache line during a DMA transfer is active is to pause the DMA, invalidate the cache line, do the write, write back and invalidate the cache line, and restart the DMA.

Paul.

Note that this typically happen with network drivers, some infos in the skbuf getting lost when they happen to share a cache line with the data portion of the skbuf. When writing a driver specific to a non-coherent CPU, it can be worked around by reserving enough space, but "generic" PCI drivers are still affected.

On those architectures, the core skbuf alloc routines should probably make sure the data portion don't share a cache line with other informations.

Ben.

Ugh. Yes, I guess the very act of starting the DMA could evict a few. In that case, your method won't work either.

It sure would be nice to get the DMA started early, but that would require some way to keep new stuff out of the cache.

Well gee, the MPC7400 has such an ability. I don't know if it's fast, but one could do:

1. write back cache lines that cross unaligned boundries
2. sync
3. set L2CR[L2DO] and L2CR[L2IO] (and maybe HID0[DLOCK] too)
4. start the DMA
5. invalidate all cache lines affected by the DMA
6. undo step 3 (clear L2CR[L2DO], L2CR[L2IO], HID0[DLOCK])

Most likely the above needs another sync or two.

------ write-back happens during DMA; data is ruined ------

As noted in another email, that was a mistake. It might be good to invalidate anyway, since that frees up cache lines for other uses. The process won't get a chance to touch the page unless you have SMP or (maybe) preemption.

Am Sonntag, 9. Juni 2002 10:48 schrieb Russell King:

Starting DMA has to be the very last step.

Regards
Oliver

From: Oliver Neukum <[email protected]>
Date: Sun, 9 Jun 2002 08:45:49 +0200

Am Sonntag, 9. Juni 2002 07:29 schrieb David S. Miller:

Does this mean that this piece of memory does have to be declared uncacheable until DMA is finished ?
How else do you solve th problem of validity during DMA and especially after DMA ?

You flush either before/after depending upon whether the cpu caches are writeback in nature or not, and the cpu is not allowed to touch those addresses while the device is doing the DMA.

From: Paul Mackerras <[email protected]>
Date: Sun, 9 Jun 2002 19:51:58 +1000 (EST)

This is the problem scenario. Suppose we are doing DMA to a buffer B and also independently accessing a variable X which is not part of B. Suppose that X and the beginning of B are both in cache line C.

I see what the problem is. Hmmm...

I'm trying to specify this such that knowledge of cachelines and whatnot don't escape the arch specific code, ho hum... Looks like that isn't possible.

So we need some kind of cache_aligned macro in our USB data structures if they contain a buffer. Which macro would we have to use ? Is there a macro conditional to incoherent architectures so we don't have to waste RAM needlessly ?

Regards
Oliver

From: Oliver Neukum <[email protected]>
Date: Mon, 10 Jun 2002 12:00:14 +0200

So we need some kind of cache_aligned macro in our USB data structures if they contain a buffer. Which macro would we have to use ?

For now use SMP_CACHE_BYTES I guess.

Paul> This is the problem scenario. Suppose we are doing DMA to a Paul> buffer B and also independently accessing a variable X which Paul> is not part of B. Suppose that X and the beginning of B are Paul> both in cache line C.

David> I see what the problem is. Hmmm...

David> I'm trying to specify this such that knowledge of David> cachelines and whatnot don't escape the arch specific code, David> ho hum... Looks like that isn't possible.

So is the consensus now that in general drivers should make sure any buffers passed to pci_map/unmap are aligned to SMP_CACHE_BYTES (and a multiple of SMP_CACHE_BYTES in size)? In other words if a driver uses an unaligned buffer it should be fixed unless we can prove (and document in a comment :) that it won't cause problems on an arch without cache coherency and with a writeback cache.

Thanks,
Roland

David> For non-cacheline aligned chunks in the range "start" to David> "end" you must perform a cache writeback and invalidate. To David> preserve the data outside of the DMA range.

Roland> Doesn't this still have a problem if you touch data in the Roland> same cache line as the DMA buffer after the pci_map but Roland> before the DMA takes place? The CPU will pull the cache Roland> line back in and it might not see the data the DMA brought Roland> in.

Roland> It seems to me that to be totally safe, pci_unmap would Roland> have to save the non-aligned part outside the buffer to Roland> temporary storage, do an invalidate, and then copy back Roland> the non-aligned part.

Replying to myself.... Anyway, I realized that even my idea above is wrong. I don't see _any_ safe way to share a cache line between a DMA buffer and other data. Access to the cache line might pull the cache line back in and write it back at any time, which could corrupt the DMA'ed data. I don't see a way to hide the existence of cache lines etc. from the driver.

Best,
Roland

And how about we don't call it SMP_CACHE_BYTES too? The processors where this matters certainly aren't doing SMP...

Tom Rini (TR1265)
http://gate.crashing.org/~trini/

Definitely we should call it something different so we can limit it to architectures that need it.

Regards
Oliver

Roland> So is the consensus now that in general drivers should Roland> make sure any buffers passed to pci_map/unmap are aligned Roland> to SMP_CACHE_BYTES (and a multiple of SMP_CACHE_BYTES in Roland> size)? In other words if a driver uses an unaligned Roland> buffer it should be fixed unless we can prove (and Roland> document in a comment :) that it won't cause problems on Roland> an arch without cache coherency and with a writeback Roland> cache.

Tom> And how about we don't call it SMP_CACHE_BYTES too? The Tom> processors where this matters certainly aren't doing SMP...

Fair enough... there is of course L1_CACHE_BYTES but I'm not positive that's always the right thing. If we want to introduce a new constant then we will have to touch every arch (which is not necessarily a killer but it means "fixed" drivers won't compile for everyone until their arch is fixed).

What would you propose? I don't have strong feelings about the exact form of a solution but I would like to decide something so we can have a standard way of fixing drivers that use unaligned DMA buffers (and convincing maintainers to apply the patches).

Thanks,
Roland

No. We should just make it come out to a nop for arches that don't need it. Otherwise we'll end up with ugly things like:
#ifdef CONFIG_NOT_CACHE_COHERENT
...
#else
...
#endif

All over things like USB...

Tom Rini (TR1265)
http://gate.crashing.org/~trini/

Am Montag, 10. Juni 2002 19:29 schrieb Tom Rini:

You are right.

Regards
Oliver

DMA_ALIGN, DMA_ALIGN_BYTES or DMA_CACHE_BYTES.

Hmm, thinking about this some more (and just rambling). On some ARMs (maybe other CPUs as well) each cache line has two dirty bits - one for each half. If only half the cache line is dirty, it will only write out the dirty half when evicting it. In this case, we'd want:

#define L1_CACHE_BYTES 32
#define DMA_CACHE_BYTES 16

which will probably work as long as we handle stuff carefully in the architecture specific layer.

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Perhaps provide macros in asm/pci.h that will:

- Take a buffer size and add an appropriate amount (one cache line for alignment and the remainder to fill out the last cache line) to be used for kmalloc(), etc, eg:

#define DMA_SIZE_ROUNDUP(size) \
((size + 2*SMP_CACHE_BYTES - 1) & ~(SMP_CACHE_BYTES - 1))

- Take a buffer address (as returned from kmalloc() with the modified size from above) and round it up to a cacheline boundary, eg:

#define DMA_BUFFER_ALIGN(ptr) \
(((unsigned long)ptr + SMP_CACHE_BYTES - 1) & ~(SMP_CACHE_BYTES - 1))

These two, in conjunction, would provide a buffer that's aligned on a cacheline boundary and ends on a cacheline boundary. Kind of ugly, but would be sufficient and would hide the cacheline size specifics. Cache-coherent platforms would just returned the original argument.

Thanks,
Will

Err, I should say any case where the buffer may not be cache-aligned (this would enable the use of DMA to stack...). For allocation routines that give a cache-aligned buffer, this is obviously not needed (but would just add one cacheline to the size).

Why not just make some dmalloc() macro in pci.h which will do the nessecory magic resizing and alignments? seems a lot easier to do...

Mark Zealey
[email protected]; [email protected]

That won't be enough. We need a solution for buffers that are parts of structures.

Regards
Oliver

Tom> No. We should just make it come out to a nop for arches that Tom> don't need it. Otherwise we'll end up with ugly things like:
Tom> #ifdef CONFIG_NOT_CACHE_COHERENT ... #else ... #endif Tom> All over things like USB...

Good point. How about the following: add a file to each arch named say, <asm/dma_buffer.h>, that defines a macro __dma_buffer. This macro would be used as follows to mark DMA buffers (example taken from <linux/usb.h>):

struct usb_device {
/* ... stuff deleted ... */

struct usb_bus *bus; /* Bus we're part of */

struct usb_device_descriptor descriptor __dma_buffer; /* Descriptor */
struct usb_config_descriptor *config; /* All of the configs */

/* ... more stuff deleted ... */
};

Then cache-coherent architectures like i386 can just do

#define __dma_buffer

while PPC can do

#ifdef CONFIG_NOT_CACHE_COHERENT

#define __dma_buffer __dma_buffer_line(__LINE__) #define __dma_buffer_line(line) __dma_buffer_expand_line(line)

#define __dma_buffer_expand_line(line) \
	__attribute__ ((aligned(SMP_CACHE_BYTES))); \
	char __dma_pad_ ## line [0] __attribute__ ((aligned(SMP_CACHE_BYTES)))

#else /* CONFIG_NOT_CACHE_COHERENT */

#define __dma_buffer

#endif /* CONFIG_NOT_CACHE_COHERENT */

C purists will point out that this is not guaranteed to work since the compiler can reorder structure members. However I'm sure that there are many, many other places in the kernel where we are counting on gcc not to reorder structures.

Comments?

Thanks,
Roland

[snip]

SMP_CACHE_BYTES is non-sensical on 4xx (and 8xx) since they don't do SMP..

Tom Rini (TR1265)
http://gate.crashing.org/~trini/

Tom> SMP_CACHE_BYTES is non-sensical on 4xx (and 8xx) since they Tom> don't do SMP..

True but <asm/cache.h> defines it anyway... of course it would be no problem to use L1_CACHE_BYTES and in fact that probably makes sense because we're talking about PPC-only macros (other arches would have their own definition).

Best,
Roland

Right, to L1_CACHE_BYTES even. :) So we should probably use that directly.

Well, ARM (whom we (PPC)) borrowed some ideas from will set it to L1_CACHE_BYTES too, from the sound of rmk. So it might even be consistent among the non coherent processors. :)

Tom Rini (TR1265)
http://gate.crashing.org/~trini/

Wait a second, forget all of this cache alignment crap. If we can avoid drivers seeing it, we should by all means necessary.

We should just tell people to use PCI pools and be done with it. That way all the complexity about buffer alignment and all this other crapola lives strictly inside of the PCI pool code.

David> Wait a second, forget all of this cache alignment crap. If David> we can avoid drivers seeing it, we should by all means David> necessary.

David> We should just tell people to use PCI pools and be done David> with it. That way all the complexity about buffer David> alignment and all this other crapola lives strictly inside David> of the PCI pool code.

That's fine but there are drivers (USB, etc) doing

	struct something {
	        int field1;
	        char dma_buffer[SMALLER_THAN_CACHE_LINE];
	        int field2;
	};

	struct something *dev = kmalloc(sizeof *dev, GFP_KERNEL);

Do they have to change to

	struct something {
	        int field1;
	        char *dma_buffer;
	        int field2;
	};

	struct something *dev = kmalloc(sizeof *dev, GFP_KERNEL);
        dev->dma_buffer = kmalloc(SMALLER_THAN_CACHE_LINE, GFP_KERNEL);

(This is always safe because as you said kmalloc can never return a slab that's not safe for DMA) I don't see how PCI pools help here.

Best,
Roland

In the USB case, the "something" represents a device, and the "dma_buffer" is something you want to send-to / receive-from the device.

kmallocing the transfer buffers is a problem for suspend. Doing the "you get the transfer buffer with the device" is really useful, because you know that the device configuration will be returned. But we might need to re-init the device after a suspend, and [I've been told that] memory allocation may deadlock under these circumstances.

Would it be enought to move the transfer buffers to the start of the device struct, and then pad it up to a cacheline?

http://conf.linux.org.au. 22-25Jan2003. Perth, Australia. Birds in Black.

Brad> Would it be enought to move the transfer buffers to the Brad> start of the device struct, and then pad it up to a Brad> cacheline?

Something like the __dma_buffer macro I posted earlier makes it even simpler. I'll make a patch that adds <asm/dma_buffer.h> so we have something concrete to discuss.

Best,
Roland

From: Roland Dreier <[email protected]>
Date: 10 Jun 2002 21:04:30 -0700

That's fine but there are drivers (USB, etc) doing

   	struct something {
   	        int field1;
   	        char dma_buffer[SMALLER_THAN_CACHE_LINE];
   	        int field2;
   	};
   
   	struct something *dev = kmalloc(sizeof *dev, GFP_KERNEL);

Do they have to change to

How about allocating struct something using pci_pool?

From: Roland Dreier <[email protected]>
Date: 10 Jun 2002 21:24:35 -0700

Something like the __dma_buffer macro I posted earlier makes it even simpler. I'll make a patch that adds <asm/dma_buffer.h> so we have something concrete to discuss.

Why don't you just allocate the "struct something" from PCI pools? If you don't have the pci_dev from that point, make some callback into the host adapter driver so you can get at it.

   	struct something {
   	        int field1;
   	        char dma_buffer[SMALLER_THAN_CACHE_LINE];
   	        int field2;
   	};
   
   	struct something *dev = kmalloc(sizeof *dev, GFP_KERNEL);

David> How about allocating struct something using pci_pool?

The problem is the driver can't safely touch field1 or field2 near the DMA (it might pull the cache line back in too soon, or dirty the cache line and have it written back on top of DMA'ed data)

Best,
Roland

From: Roland Dreier <[email protected]>
Date: 10 Jun 2002 21:39:27 -0700

David> How about allocating struct something using pci_pool?

The problem is the driver can't safely touch field1 or field2 near the DMA (it might pull the cache line back in too soon, or dirty the cache line and have it written back on top of DMA'ed data)

Oh duh, I see, then go to making the thing a pointer to the dma area.

Am Dienstag, 11. Juni 2002 06:38 schrieb David S. Miller:

That would mean doing things like memory allocations for one single byte. Also it would affect things like the scsi layer (sense buffer in the structure).
And it would be additional overhead for everybody for the benefit of a small minority. An alignment macro could be turned into a nop on some architectures.

Regards
Oliver

From: Oliver Neukum <[email protected]>
Date: Tue, 11 Jun 2002 08:23:52 +0200

That would mean doing things like memory allocations for one single byte. Also it would affect things like the scsi layer (sense buffer in the structure).
And it would be additional overhead for everybody for the benefit of a small minority. An alignment macro could be turned into a nop on some architectures.

The problem is people are going to get it wrong if we expose all of this cacheline crap to the drivers.

Am Dienstag, 11. Juni 2002 08:38 schrieb David S. Miller:

You don't have to fully expose it. We make a simple rule like:
If you want to do DMA to a variable it needs "DMA_ALIGN after the name in the declaration". Architectures could define it, with generic nop. People will get it wrong by doing DMA to members of structures otherwise. There's no really painless way to solve this.
You have to introduce a new rule anyway.

Regards
Oliver

From: Oliver Neukum <[email protected]>
Date: Tue, 11 Jun 2002 09:38:52 +0200

You don't have to fully expose it. We make a simple rule like:
If you want to do DMA to a variable it needs "DMA_ALIGN after the name in the declaration". Architectures could define it, with generic nop. People will get it wrong by doing DMA to members of structures otherwise. There's no really painless way to solve this.
You have to introduce a new rule anyway.

The DMA_ALIGN attribute doesn't work, on some systems the PCI cacheline size is determined at boot time not compile time.

From: "David S. Miller" <[email protected]>
Date: Tue, 11 Jun 2002 00:36:25 -0700 (PDT)

The DMA_ALIGN attribute doesn't work, on some systems the PCI cacheline size is determined at boot time not compile time.

Another note, it could be per-PCI controller what this cacheline size is. We'll need to pass in a pdev to the alignment interfaces to do this correctly.

So none of this can be done at compile time folks.

Am Dienstag, 11. Juni 2002 09:43 schrieb David S. Miller:

Could you please explain this ?

I thought this was a problem of a CPU dirtying a cache line that overlaps with an area being DMAed into. So the determining factor should be the granularity of the dirty status of the CPU.

Are there really PCI controllers which have to physically write much more than is transfered ?

Now really puzzeled
Oliver

From: Oliver Neukum <[email protected]>
Date: Tue, 11 Jun 2002 10:07:19 +0200

Are there really PCI controllers which have to physically write much more than is transfered ?

On sparc64 the cacheline size can be either 64 or 128 bytes. It's a bus characteristic, so we have to get at the PCI controller info.

Am Dienstag, 11. Juni 2002 10:15 schrieb David S. Miller:

A sparc64 is unlikely to be short on memory, or is it ? What's wrong with always aligning on 128 bytes on sparc64 ? A runtime check would be expensive.

Regards
Oliver

From: Oliver Neukum <[email protected]>
Date: Tue, 11 Jun 2002 14:06:14 +0200

A sparc64 is unlikely to be short on memory, or is it ? What's wrong with always aligning on 128 bytes on sparc64 ? A runtime check would be expensive.

Maybe on arch FOO, target X needs no alignment when using PCI controller Y, but for PCI controller Z it does need alignment.

Access to a cache line can't be shared safely. Disable the cache for that page while its ownership is in doubt and you can happily share the RAM though. If we find another CPU which fetches cache lines preemptively, we may have to do that anyway.

--
dwmw2

Why is this a problem? So you just make a static inline that takes the pdev and does the Right Thing at runtime... It's implementation- dependent whether it's a compile-time macro or a more elaborate inline...

Am Dienstag, 11. Juni 2002 14:04 schrieb David S. Miller:

Still does that justify the overhead and the complications ? Couldn't we provide for the worst case in a generic kernel and make it a compile time option ?

If I understand you correctly, we even couldn't use kmalloc() for allocating the buffers. IMHO you cannot expose that to driver writers and hope to get a useful result.
So what are the alternatives ?
We could either use a bounce buffer or disable caching for the page in question, which has its own set of problems.

Regards
Oliver

David> Maybe on arch FOO, target X needs no alignment when using David> PCI controller Y, but for PCI controller Z it does need David> alignment.

I'm not sure I understand this objection. kmalloc() is will return a buffer that is safe for DMA for all controllers. Any compile-time static alignment of structure members would similarly do worst-case alignment. Also I have to admit that I don't quite understand the issue you're raising here. Obviously the CPU's cache line size doesn't change based on the PCI controller. Are you referring to PCI device's CLS register? I don't see how that could matter since the bus master shouldn't ever write beyond the buffer the CPU gives it.

In any case, given drivers that have:

        struct something {
                int field1;
                char dma_buffer[SMALLER_THAN_CACHELINE];
                int field2;
        };

        struct something *dev = kmalloc(sizeof *dev, GFP_KERNEL);
        /* do DMA into dev->dma_buffer */

I know of several ways to make this safe:

1) Add a static alignment macro and change the declaration to

        struct something {
                int field1;
                char dma_buffer[SMALLER_THAN_CACHELINE] __dma_buffer;
                int field2;
        };

__dma_buffer would be a NOP on cache coherent architectures (i386, etc) but might introduce some alignment not strictly necessary on architectures (???)

2) Change the code to

        struct something {
                int field1;
                char *dma_buffer;
                int field2;
        };

        struct something *dev = kmalloc(sizeof *dev, GFP_KERNEL);
        dev->dma_buffer = kmalloc(SMALLER_THAN_CACHELINE, GFP_KERNEL);
        /* do DMA into dev->dma_buffer */

This always uses strictly more memory than 1) above, complicates the code, may introduce bugs (do we know if anyone did *dev_copy = *dev anywhere?).

3) Change the code to

        struct something {
                int field1;
                char *dma_buffer;
                int field2;
        };

        struct something *dev = kmalloc(sizeof *dev, GFP_KERNEL);
        /* find pci_device */
        dev->dma_buffer = aligned_pci_alloc(pci_device, SMALLER_THAN_CACHELINE);
        /* do DMA into dev->dma_buffer */

This may use less memory than 1) or 2) above on some architectures but will use more than 1) on cache-coherent architectures. It makes the code even more complex since now the code that allocates the dma buffer has to know which PCI device will use it (for example, in USB, the hub driver is separated from the HCD driver, which is who knows about the PCI bus).

4) David Woodhouse's suggestion of turning off caching for pages where unaligned DMA is in progress. This may be doable but seems quite complex. Also, drivers will probably some way of aligning their buffers to avoid turning of caching, which means that this approach and the above are complementary.

Best,
Roland

Hi,

You introduce a possible null pointer dereference here, don't you?

// Assume this fails:
struct something *dev = kmalloc(sizeof(struct something), GFP_KERNEL); // dev is a NULL pointer now.
dev->dma_buffer = aligned_pci_alloc(pci_device, SMALLER_THAN_CACHELINE); // Big bang

Just wondering...

Regards,
Thunder

German attitude becoming        |	Thunder from the hill at ngforever
rightaway popular:		|
       "Get outa my way,  	|	free inhabitant not directly
    for I got a mobile phone!"	|	belonging anywhere

Thunder> You introduce a possible null pointer dereference here, Thunder> don't you?

I left out the error checking for the allocations everywhere in my email. It wasn't real code and I thought that testing for NULL all over the place would obscure the real point.

R.

Hi,

Sure thing. It was just my newly introduced paranoia, I have some things like them to fix.

Regards,
Thunder

German attitude becoming        |	Thunder from the hill at ngforever
rightaway popular:		|
       "Get outa my way,  	|	free inhabitant not directly
    for I got a mobile phone!"	|	belonging anywhere

What about an arch that can be both coherent or incoherent (the same kernel binary would boot both) ?

I can also imagine quite a bunch of embedded stuffs where you may have both coherent and non-coherent devices depending on the bus the live on or on bridge bugs.

For your example, I don't buy it. You could well design the USB urb allocation in such a way that they are passed down the controller of a given device.

Ben.

From: William Jhun <[email protected]>
Date: Mon, 10 Jun 2002 11:07:40 -0700

Perhaps provide macros in asm/pci.h that will:

You don't understand, I think. I want to avoid the drivers doing any of the "align this, align that" stuff. I want the allocation to do it for them, that way the code is in one place.

Urbs are not the problem. An urb abstractly speaking is just a description of io. It does not contain a buffer, just a pointer to it.

However many drivers allocate some of these buffers together with their device descriptors, which would, if a special allocator must be used, become impossible.
Usbcore could allocate bounce buffers, but performance would suck.

If I understand both Davids correctly this is the solution. Buffers for dma must be allocated seperately using a special allocation function which is given the device so it can allocate correctly. David B wants a bus specific pointer to a function in the generic driver structure, right ?

Regards
Oliver

From: Oliver Neukum <[email protected]>
Date: Wed, 12 Jun 2002 14:02:53 +0200

If I understand both Davids correctly this is the solution. Buffers for dma must be allocated seperately using a special allocation function which is given the device so it can allocate correctly. David B wants a bus specific pointer to a function in the generic driver structure, right ?

Eventually it could be exactly that, via the generic driver struct.

For now it can be a usb specific thing, and when the generic device stuff is ready we just go:

#define usb_pool_alloc(...) dev_pool_alloc(...)

without having to touch any of the driver call sites.

Am Mittwoch, 12. Juni 2002 13:47 schrieb David S. Miller:

That means that all buffers must be allocated seperately. Is it worth that ? How about skbuffs ?

Regards
Oliver

Then let's have those as part of the generic device struct, with the default ones pointing to the parent bus ones.

That way, a couple of generic ones could be set at the root of the device tree for fully coherent or fully incoherent archs, and bus drivers would have the ability to affect their child devices ones.

Ben.

No, I do understand your point. And this does not bring "knowledge of cachelines and whatnot" into the driver; those "macros" could similarly be calls to arch-specific code that acts based on a pdev. I was simply trying to think of a compromise between that and massively changing the interface by which a driver obtains buffers. And I assume alloc_skb() and others would need to change otherwise. How would you specify if your skb data needs to be PCI DMA-able? What about net drivers not using DMA at all?

Thanks,
Will

Hi!

But upper bound is certainly known at compile time, right?
Pavel

(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

From: Pavel Machek <[email protected]>
Date: Wed, 12 Jun 2002 11:06:39 +0200

But upper bound is certainly known at compile time, right?

That's true.

From: Oliver Neukum <[email protected]>
Date: Tue, 11 Jun 2002 16:23:30 +0200

Still does that justify the overhead and the complications ? Couldn't we provide for the worst case in a generic kernel and make it a compile time option ?

I already posted that using the worst case scenerio would be an acceptable solution to this problem, I consider it a closed issue and you can ignore my earlier gripes on this issue.

From: Oliver Neukum <[email protected]>
Date: Wed, 12 Jun 2002 14:08:26 +0200

That means that all buffers must be allocated seperately. Is it worth that ? How about skbuffs ?

skbuffs are fine, they are already minimally aligned to SMP_CACHE_BYTES and also have the same minimum length.

The only problematic case are as in the eepro100.c driver where it tries to use part of the skbuff data area for the receive descriptor.