SCSI breakage on non-cache coherent architectures

I'm not sure what is the best way to fix that. Internally, I've done some test whacking some ____cacheline_aligned in the scsi_cmnd data structure to verify I no longer get random SLAB corruption when using my USB but that significantly bloats the size of the structure on archs such as ppc64 that don't need it and have a large cache line size.

Unfortunately, I don't think there's any existing Kconfig symbol or arch provided #define to tell us that we are on a non-coherent arch afaik that could be used to make that conditional.

Another option would be to kmalloc the buffer (wasn't it the case before btw ?) but I suppose some people will scream at the idea due to how the command pools are done...

From: Benjamin Herrenschmidt <[email protected]>
Date: Mon, 19 Nov 2007 16:35:23 +1100

Quoted 10 lines

I'm not sure what is the best way to fix that. Internally, I've done some test whacking some ____cacheline_aligned in the scsi_cmnd data structure to verify I no longer get random SLAB corruption when using my USB but that significantly bloats the size of the structure on archs such as ppc64 that don't need it and have a large cache line size.

Unfortunately, I don't think there's any existing Kconfig symbol or arch provided #define to tell us that we are on a non-coherent arch afaik that could be used to make that conditional.

Quoted 3 lines

Another option would be to kmalloc the buffer (wasn't it the case before btw ?) but I suppose some people will scream at the idea due to how the command pools are done...

You could make a ____dma_cacheline_aligned and use that. It seems pretty reasonable.

Quoted 5 lines

From: Benjamin Herrenschmidt <[email protected]>
Date: Mon, 19 Nov 2007 16:35:23 +1100

You could make a ____dma_cacheline_aligned and use that. It seems pretty reasonable.

I was thinking about that. What archs would need it ? arm, mips, what else ?

From: Benjamin Herrenschmidt <[email protected]>
Date: Tue, 20 Nov 2007 06:51:14 +1100

Quoted 4 lines · David Miller

Quoted 3 lines

From: Benjamin Herrenschmidt <[email protected]>
Date: Mon, 19 Nov 2007 16:35:23 +1100

Quoted 5 lines

Quoted 2 lines

You could make a ____dma_cacheline_aligned and use that. It seems pretty reasonable.

I was thinking about that. What archs would need it ? arm, mips, what else ?

The sparc32 port would need it too.

Do you still think we should introduce this __dma_cacheline_aligned ? Do you see other cases of drivers where it would be useful ? It tend to agree with your earlier statement that drivers doing that are broken and should be using a separate allocator for DMA'ble objects (in fact, on non-cache coherent archs, kmalloc is just fine).

1) Require that entire buffers are commited by call sites, and thus "embedding" DMA'd within non-DMA stuff isn't allowed

2) Add the __dma_cacheline_aligned tag.

But note that with #2 it could get quite ugly because the alignment and size both have a minimum that needs to be enforced, not just the alignment alone. So either:

struct foo {
        unsigned int other_unrelated_stuff;

        struct object dma_thing __dma_cacheline_aligned;

        unsigned int more_nondma_stuff __dma_cacheline_aligned;
};

BTW. What is the status nowadays with skb's ?

2) Add the __dma_cacheline_aligned tag.

But note that with #2 it could get quite ugly because the alignment and size both have a minimum that needs to be enforced, not just the alignment alone. So either:

struct foo {
	unsigned int other_unrelated_stuff;

	struct object dma_thing __dma_cacheline_aligned;

	unsigned int more_nondma_stuff __dma_cacheline_aligned;
};

or:

struct foo {
	unsigned int other_unrelated_stuff;

	union {
		struct object dma_thing __dma_cacheline_aligned;
		char __pad[(sizeof(object) + DMA_CACHELINE_SIZE &
			   ~DMA_CACHELINE_SIZE)];
	} u;

	unsigned int more_nondma_stuff __dma_cacheline_aligned;
};

I wrapped this ugliness up inside the macro back in what I posted in 2002 (http://lkml.org/lkml/2002/6/12/234):

#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(L1_CACHE_BYTES))); \
        char __dma_pad_ ## line [0] __attribute__

((aligned(L1_CACHE_BYTES)))

then you just need to tag the actual member like:

        char foo[3] __dma_buffer;

FYI, Here's what I have for the SCSI change. I haven't updated drivers to care for the new return code though, help appreciated with that as I don't know much about these drivers.

Quoted 17 lines · David Miller

From: Benjamin Herrenschmidt <[email protected]>
Date: Mon, 19 Nov 2007 16:35:23 +1100

Quoted 13 lines

I'm not sure what is the best way to fix that. Internally, I've done some test whacking some ____cacheline_aligned in the scsi_cmnd data structure to verify I no longer get random SLAB corruption when using my USB but that significantly bloats the size of the structure on archs such as ppc64 that don't need it and have a large cache line size.

Unfortunately, I don't think there's any existing Kconfig symbol or arch provided #define to tell us that we are on a non-coherent arch afaik that could be used to make that conditional.

Another option would be to kmalloc the buffer (wasn't it the case before btw ?) but I suppose some people will scream at the idea due to how the command pools are done...

Quoted 2 lines

You could make a ____dma_cacheline_aligned and use that. It seems pretty reasonable.

I was thinking about that. What archs would need it ? arm, mips, what else ?

Quoted 19 lines · Benjamin Herrenschmidt

Quoted 17 lines · David Miller

From: Benjamin Herrenschmidt <[email protected]>
Date: Mon, 19 Nov 2007 16:35:23 +1100

Quoted 13 lines

I'm not sure what is the best way to fix that. Internally, I've done some test whacking some ____cacheline_aligned in the scsi_cmnd data structure to verify I no longer get random SLAB corruption when using my USB but that significantly bloats the size of the structure on archs such as ppc64 that don't need it and have a large cache line size.

Unfortunately, I don't think there's any existing Kconfig symbol or arch provided #define to tell us that we are on a non-coherent arch afaik that could be used to make that conditional.

Another option would be to kmalloc the buffer (wasn't it the case before btw ?) but I suppose some people will scream at the idea due to how the command pools are done...

Quoted 5 lines

Quoted 2 lines

You could make a ____dma_cacheline_aligned and use that. It seems pretty reasonable.

I was thinking about that. What archs would need it ? arm, mips, what else ?

older parisc

Actually, we already established on IRC that the lasi700 driver doesn't need this, principally because the parisc architecture doesn't do an invalidate for DMA_FROM_DEVICE but a flush and invalidate (architecturally, if you read our manuals, even pdc is entitled to write back dirty lines, so it's not clear there's actually an invalidate
instruction we can use). This is also one possible temporary fix for
the other architectures if we can't get a different method to work nicely.

Quoted 8 lines

Actually, we already established on IRC that the lasi700 driver doesn't need this, principally because the parisc architecture doesn't do an invalidate for DMA_FROM_DEVICE but a flush and invalidate (architecturally, if you read our manuals, even pdc is entitled to write back dirty lines, so it's not clear there's actually an invalidate
instruction we can use). This is also one possible temporary fix for
the other architectures if we can't get a different method to work nicely.

I think doing a writeback and invalidate is a very fragile way to deal with DMA into the middle of a data structure. It may work OK for now, but you have to make sure forever into the future that no codepath anywhere else ever touches the cacheline that you're DMAing into while the DMA is pending. It just leaves a hidden trap that is too easy to step on, because the architectures that get pretty much all testing all have cache-coherent DMA.

Reviving my ancient __dma_buffer patch seems far preferable to me.

We're talking about trying to fix this for 2.4; which is already at -rc3 ... Is an entire arch change for dma alignment really a merge candidate at this stage?

Quoted 3 lines · James Bottomley

We're talking about trying to fix this for 2.4; which is already at -rc3 ... Is an entire arch change for dma alignment really a merge candidate at this stage?

Well, as I said before... it's a matter of what seems to be the less likely to break something right ?

On one side, I'm doing surgery on code I barely know, the scsi error handling, and now it seems I also have to fixup a handful of drivers that aren't the most obvious pieces of code around.

On the other side, Roland proposal is basically just adding a macro that can be empty for everybody but a handful of archs, and stick it onto one field in one structure...

The later has about 0 chances to actually break something or cause a regression. I wouldn't say that about the former.

Now, I will see if I manage to fixup the NCR drivers to pass a pre-allocated buffer (USB storage I think can pass NULL as it's not calling prep in atomic context). But then, it complicates the matter because that means "restore" will have to know whether prep allocated the buffer or not, thus more fields to add to the save struct, it's getting messy, unless we decide -all- callers are responsible for the buffer allocation (hrm... maybe the best approach).

The other one I'm hitting now is that the SCSI layer nowadays embeds the

sense_buffer inside the scsi_cmnd structure without any kind of alignment whatsoever. I've been hitting irregulary is a crash on SCSI command completion that seems to be related to corruption of the "request" pointer in struct scsi_cmnd and I think it might be the cause. I'm now trying to setup a proper repro-case.

Quoted 1 line · Benjamin Herrenschmidt

The other one I'm hitting now is that the SCSI layer nowadays embeds the

'nowadays'? It has always been so.

Quoted 5 lines

sense_buffer inside the scsi_cmnd structure without any kind of alignment whatsoever. I've been hitting irregulary is a crash on SCSI command completion that seems to be related to corruption of the "request" pointer in struct scsi_cmnd and I think it might be the cause. I'm now trying to setup a proper repro-case.

What other drivers do is DMA to their own allocation and then memcpy to the sense buffer.

There is a movement to allocate the sense data as its own sg list, but I don't think that patch has even been posted yet.

Quoted 5 lines

What other drivers do is DMA to their own allocation and then memcpy to the sense buffer.

There is a movement to allocate the sense data as its own sg list, but I don't think that patch has even been posted yet.

I'd like to be rid of it inside the command for various reasons: every command has one of these, and they're expensive in the allocation (at 96 bytes). There's no reason we have to allocate and free that amount of space with every command. In theory, the number of these is bounded at the queue depth, in practice, there's usually only one, and this DMA alignment issue does requires most drivers to double copy.

I'd like to be rid of it inside the command for various reasons: every command has one of these, and they're expensive in the allocation (at 96 bytes). There's no reason we have to allocate and free that amount of space with every command. In theory, the number of these is bounded at the queue depth, in practice, there's usually only one, and this DMA alignment issue does requires most drivers to double copy.

Quoted 1 line · Benjamin Herrenschmidt

The other one I'm hitting now is that the SCSI layer nowadays embeds the

'nowadays'? It has always been so.

Quoted 5 lines

sense_buffer inside the scsi_cmnd structure without any kind of alignment whatsoever. I've been hitting irregulary is a crash on SCSI command completion that seems to be related to corruption of the "request" pointer in struct scsi_cmnd and I think it might be the cause. I'm now trying to setup a proper repro-case.

What other drivers do is DMA to their own allocation and then memcpy to the sense buffer.

There is a movement to allocate the sense data as its own sg list, but I don't think that patch has even been posted yet.

I've been debugging various issues on the PowerPC 44x embedded architecture which happens to have non-coherent PCI DMA.

One of the problem I'm hitting is that one really need to enforce kmalloc alignement to cache lines or bad things will happen (among others with USB), for some reasons, powerpc failed to do so, I fixed it.

Quoted 6 lines

I've been debugging various issues on the PowerPC 44x embedded architecture which happens to have non-coherent PCI DMA.

One of the problem I'm hitting is that one really need to enforce kmalloc alignement to cache lines or bad things will happen (among others with USB), for some reasons, powerpc failed to do so, I fixed it.

Heh... I hit the same problem literally 5 years ago:

    http://lwn.net/Articles/1783/

I implemented the __dma_buffer annotation:

    http://lwn.net/Articles/2269/

But DaveM said we should just use the PCI pool code instead:

    http://lwn.net/Articles/2270/