Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Mon, Jan 07 2008 at 8:53 +0200, FUJITA Tomonori [EMAIL PROTECTED] wrote: On Sun, 23 Dec 2007 13:09:05 +0200 Boaz Harrosh [EMAIL PROTECTED] wrote: On Fri, Dec 21 2007 at 4:30 +0200, Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly by some low level drivers (that typically happens with USB mass storage). This is a problem on non cache coherent architectures such as embedded PowerPCs where the sense buffer can share cache lines with other structure members, which leads to various forms of corruption. This uses the newly defined __dma_buffer annotation to enforce that on such platforms, the sense_buffer is contained within its own cache line. This has no effect on cache coherent architectures. Signed-off-by: Benjamin Herrenschmidt [EMAIL PROTECTED] --- include/scsi/scsi_cmnd.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-merge.orig/include/scsi/scsi_cmnd.h 2007-12-21 13:07:14.0 +1100 +++ linux-merge/include/scsi/scsi_cmnd.h2007-12-21 13:07:29.0 +1100 @@ -88,7 +88,7 @@ struct scsi_cmnd { working on */ #define SCSI_SENSE_BUFFERSIZE 96 - unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE]; + unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE] __dma_buffer; /* obtained by REQUEST SENSE when * CHECK CONDITION is received on original * command (auto-sense) */ This has the potential of leaving a big fat ugly hole in the middle of scsi_cmnd. I would suggest of *just* moving the sense_buffer array to be the *first member* of struct scsi_cmnd. The command itself is already cache aligned, allocated by the proper flags to it's slab. And put a fat comment near it's definition. This is until a proper fix is sent. I have in my Q a proposition for a more prominent solution, which I will send next month. Do to short comings in the sense handling and optimizations, but should definitely cover this problem. The code should have time to be discussed and tested, so it is only 2.6.26 material. For the duration of the 2.6.25 kernel we can live with a reorder of scsi_cmnd members, if it solves such a grave bug for some ARCHs. Boaz [RFD below] My proposed solution will be has follows: 1. Since the scsi protocol mandates an immediate REQUEST_SENSE after an error in effect the Q is frozen until the REQUEST_SENSE command returns. 2. The scsi-ml needs the sense buffer for its normal operation, independent from the ULD's request of the sence_buffer or not at request-sense. But in effect, 90% of all scsi-requests come with ULD's allocated buffer for sense, that is copied to, on command completion. 3. 99% of all commands complete successfully, so if an optimization is proposed for the successful case, sacrificing a few cycles for the error case than thats a good thing. My suggestion is to have a per Q, driver-overridable, sense buffer that is DMAed/written to by drivers. At the end of the REQUEST_SENSE command one of 2 things will be done. Either copy the sense to the ULD's supplied buffer, or if not available, allocate one from a dedicated mem_cache pool. So we are completely saving 92 bytes from scsi_cmnd by replacing the buffer with a pointer. We can always read the sense into a per Q buffer. And 10% of %1 of the times we will need to allocate a sense buffer from a dedicated mem_cache I would say thats a nice optimization. The changes to scsi_error/scsi_cmnd and friends, is pretty strait forward. But it depends on a conversion of 4/5 drivers to the new scsi_eh API for REQUEST_SENSE. I have only converted these drivers that interfered with the accessors effort + 1 other places. But there are a few more places that are not converted. Once done. The implementation can easily change with no affect on drivers. I think that removing the sense_buffer array from scsi_cmnd effects lots of LLDs. As I wrote in other mail, many LLDs assume that scsi_cmnd:sense_buffer is always available. Another big task is to take care about auto sense. Have you already had some patches? I've just started to work on this and I'd like to push that fix into 2.6.25. Tomo Hi. Since you ask to push this into 2.6.25, I have ask permission to prioritize this effort, as until now it was on a back burner. I have only done 3 drivers up to now. (out of something like 15) I have seen 4 patterns of sense use. 1. Driver allocated sense that is memcpy'ed in interrupt time to cmnd-sense. The sense is automatically fetched by controller and is usually pre-mapped. 2. dma-map of cmnd-sense prior to each command. similar to case-1 but DMAed directly into cmnd-sense buffer. (AutoSense) 3. Synchronous request for sense, like the places I sent patches for.
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Mon, 07 Jan 2008 15:25:36 +0200 Boaz Harrosh [EMAIL PROTECTED] wrote: On Mon, Jan 07 2008 at 8:53 +0200, FUJITA Tomonori [EMAIL PROTECTED] wrote: On Sun, 23 Dec 2007 13:09:05 +0200 Boaz Harrosh [EMAIL PROTECTED] wrote: On Fri, Dec 21 2007 at 4:30 +0200, Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly by some low level drivers (that typically happens with USB mass storage). This is a problem on non cache coherent architectures such as embedded PowerPCs where the sense buffer can share cache lines with other structure members, which leads to various forms of corruption. This uses the newly defined __dma_buffer annotation to enforce that on such platforms, the sense_buffer is contained within its own cache line. This has no effect on cache coherent architectures. Signed-off-by: Benjamin Herrenschmidt [EMAIL PROTECTED] --- include/scsi/scsi_cmnd.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-merge.orig/include/scsi/scsi_cmnd.h 2007-12-21 13:07:14.0 +1100 +++ linux-merge/include/scsi/scsi_cmnd.h 2007-12-21 13:07:29.0 +1100 @@ -88,7 +88,7 @@ struct scsi_cmnd { working on */ #define SCSI_SENSE_BUFFERSIZE96 - unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE]; + unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE] __dma_buffer; /* obtained by REQUEST SENSE when * CHECK CONDITION is received on original * command (auto-sense) */ This has the potential of leaving a big fat ugly hole in the middle of scsi_cmnd. I would suggest of *just* moving the sense_buffer array to be the *first member* of struct scsi_cmnd. The command itself is already cache aligned, allocated by the proper flags to it's slab. And put a fat comment near it's definition. This is until a proper fix is sent. I have in my Q a proposition for a more prominent solution, which I will send next month. Do to short comings in the sense handling and optimizations, but should definitely cover this problem. The code should have time to be discussed and tested, so it is only 2.6.26 material. For the duration of the 2.6.25 kernel we can live with a reorder of scsi_cmnd members, if it solves such a grave bug for some ARCHs. Boaz [RFD below] My proposed solution will be has follows: 1. Since the scsi protocol mandates an immediate REQUEST_SENSE after an error in effect the Q is frozen until the REQUEST_SENSE command returns. 2. The scsi-ml needs the sense buffer for its normal operation, independent from the ULD's request of the sence_buffer or not at request-sense. But in effect, 90% of all scsi-requests come with ULD's allocated buffer for sense, that is copied to, on command completion. 3. 99% of all commands complete successfully, so if an optimization is proposed for the successful case, sacrificing a few cycles for the error case than thats a good thing. My suggestion is to have a per Q, driver-overridable, sense buffer that is DMAed/written to by drivers. At the end of the REQUEST_SENSE command one of 2 things will be done. Either copy the sense to the ULD's supplied buffer, or if not available, allocate one from a dedicated mem_cache pool. So we are completely saving 92 bytes from scsi_cmnd by replacing the buffer with a pointer. We can always read the sense into a per Q buffer. And 10% of %1 of the times we will need to allocate a sense buffer from a dedicated mem_cache I would say thats a nice optimization. The changes to scsi_error/scsi_cmnd and friends, is pretty strait forward. But it depends on a conversion of 4/5 drivers to the new scsi_eh API for REQUEST_SENSE. I have only converted these drivers that interfered with the accessors effort + 1 other places. But there are a few more places that are not converted. Once done. The implementation can easily change with no affect on drivers. I think that removing the sense_buffer array from scsi_cmnd effects lots of LLDs. As I wrote in other mail, many LLDs assume that scsi_cmnd:sense_buffer is always available. Another big task is to take care about auto sense. Have you already had some patches? I've just started to work on this and I'd like to push that fix into 2.6.25. Tomo Hi. Since you ask to push this into 2.6.25, I have ask permission to prioritize this effort, as until now it was on a back burner. There are no short-term solusions and seems that __dma_buffer will not be merged. It would be better to fix this soon though it's a bit hard to fix this before 2.6.25, I think. I have only done 3 drivers up to now. (out of something like 15) I have seen 4 patterns of sense use. 1.
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Sun, 23 Dec 2007 13:09:05 +0200 Boaz Harrosh [EMAIL PROTECTED] wrote: On Fri, Dec 21 2007 at 4:30 +0200, Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly by some low level drivers (that typically happens with USB mass storage). This is a problem on non cache coherent architectures such as embedded PowerPCs where the sense buffer can share cache lines with other structure members, which leads to various forms of corruption. This uses the newly defined __dma_buffer annotation to enforce that on such platforms, the sense_buffer is contained within its own cache line. This has no effect on cache coherent architectures. Signed-off-by: Benjamin Herrenschmidt [EMAIL PROTECTED] --- include/scsi/scsi_cmnd.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-merge.orig/include/scsi/scsi_cmnd.h 2007-12-21 13:07:14.0 +1100 +++ linux-merge/include/scsi/scsi_cmnd.h2007-12-21 13:07:29.0 +1100 @@ -88,7 +88,7 @@ struct scsi_cmnd { working on */ #define SCSI_SENSE_BUFFERSIZE 96 - unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE]; + unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE] __dma_buffer; /* obtained by REQUEST SENSE when * CHECK CONDITION is received on original * command (auto-sense) */ This has the potential of leaving a big fat ugly hole in the middle of scsi_cmnd. I would suggest of *just* moving the sense_buffer array to be the *first member* of struct scsi_cmnd. The command itself is already cache aligned, allocated by the proper flags to it's slab. And put a fat comment near it's definition. This is until a proper fix is sent. I have in my Q a proposition for a more prominent solution, which I will send next month. Do to short comings in the sense handling and optimizations, but should definitely cover this problem. The code should have time to be discussed and tested, so it is only 2.6.26 material. For the duration of the 2.6.25 kernel we can live with a reorder of scsi_cmnd members, if it solves such a grave bug for some ARCHs. Boaz [RFD below] My proposed solution will be has follows: 1. Since the scsi protocol mandates an immediate REQUEST_SENSE after an error in effect the Q is frozen until the REQUEST_SENSE command returns. 2. The scsi-ml needs the sense buffer for its normal operation, independent from the ULD's request of the sence_buffer or not at request-sense. But in effect, 90% of all scsi-requests come with ULD's allocated buffer for sense, that is copied to, on command completion. 3. 99% of all commands complete successfully, so if an optimization is proposed for the successful case, sacrificing a few cycles for the error case than thats a good thing. My suggestion is to have a per Q, driver-overridable, sense buffer that is DMAed/written to by drivers. At the end of the REQUEST_SENSE command one of 2 things will be done. Either copy the sense to the ULD's supplied buffer, or if not available, allocate one from a dedicated mem_cache pool. So we are completely saving 92 bytes from scsi_cmnd by replacing the buffer with a pointer. We can always read the sense into a per Q buffer. And 10% of %1 of the times we will need to allocate a sense buffer from a dedicated mem_cache I would say thats a nice optimization. The changes to scsi_error/scsi_cmnd and friends, is pretty strait forward. But it depends on a conversion of 4/5 drivers to the new scsi_eh API for REQUEST_SENSE. I have only converted these drivers that interfered with the accessors effort + 1 other places. But there are a few more places that are not converted. Once done. The implementation can easily change with no affect on drivers. I think that removing the sense_buffer array from scsi_cmnd effects lots of LLDs. As I wrote in other mail, many LLDs assume that scsi_cmnd:sense_buffer is always available. Another big task is to take care about auto sense. Have you already had some patches? I've just started to work on this and I'd like to push that fix into 2.6.25. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, Dec 21 2007 at 4:30 +0200, Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly by some low level drivers (that typically happens with USB mass storage). This is a problem on non cache coherent architectures such as embedded PowerPCs where the sense buffer can share cache lines with other structure members, which leads to various forms of corruption. This uses the newly defined __dma_buffer annotation to enforce that on such platforms, the sense_buffer is contained within its own cache line. This has no effect on cache coherent architectures. Signed-off-by: Benjamin Herrenschmidt [EMAIL PROTECTED] --- include/scsi/scsi_cmnd.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-merge.orig/include/scsi/scsi_cmnd.h 2007-12-21 13:07:14.0 +1100 +++ linux-merge/include/scsi/scsi_cmnd.h 2007-12-21 13:07:29.0 +1100 @@ -88,7 +88,7 @@ struct scsi_cmnd { working on */ #define SCSI_SENSE_BUFFERSIZE96 - unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE]; + unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE] __dma_buffer; /* obtained by REQUEST SENSE when * CHECK CONDITION is received on original * command (auto-sense) */ This has the potential of leaving a big fat ugly hole in the middle of scsi_cmnd. I would suggest of *just* moving the sense_buffer array to be the *first member* of struct scsi_cmnd. The command itself is already cache aligned, allocated by the proper flags to it's slab. And put a fat comment near it's definition. This is until a proper fix is sent. I have in my Q a proposition for a more prominent solution, which I will send next month. Do to short comings in the sense handling and optimizations, but should definitely cover this problem. The code should have time to be discussed and tested, so it is only 2.6.26 material. For the duration of the 2.6.25 kernel we can live with a reorder of scsi_cmnd members, if it solves such a grave bug for some ARCHs. Boaz [RFD below] My proposed solution will be has follows: 1. Since the scsi protocol mandates an immediate REQUEST_SENSE after an error in effect the Q is frozen until the REQUEST_SENSE command returns. 2. The scsi-ml needs the sense buffer for its normal operation, independent from the ULD's request of the sence_buffer or not at request-sense. But in effect, 90% of all scsi-requests come with ULD's allocated buffer for sense, that is copied to, on command completion. 3. 99% of all commands complete successfully, so if an optimization is proposed for the successful case, sacrificing a few cycles for the error case than thats a good thing. My suggestion is to have a per Q, driver-overridable, sense buffer that is DMAed/written to by drivers. At the end of the REQUEST_SENSE command one of 2 things will be done. Either copy the sense to the ULD's supplied buffer, or if not available, allocate one from a dedicated mem_cache pool. So we are completely saving 92 bytes from scsi_cmnd by replacing the buffer with a pointer. We can always read the sense into a per Q buffer. And 10% of %1 of the times we will need to allocate a sense buffer from a dedicated mem_cache I would say thats a nice optimization. The changes to scsi_error/scsi_cmnd and friends, is pretty strait forward. But it depends on a conversion of 4/5 drivers to the new scsi_eh API for REQUEST_SENSE. I have only converted these drivers that interfered with the accessors effort + 1 other places. But there are a few more places that are not converted. Once done. The implementation can easily change with no affect on drivers. The reason I've started with this work is because the SCSI standard permits up to 260 bytes of sense, which you guest right, is needed by the OSD command set. Boaz - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, 21 Dec 2007 13:30:08 +1100 Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly by some low level drivers (that typically happens with USB mass storage). Should that not be fixed in USB storage by using pci_alloc_coherent on the PCI device of the hub not peeing directly into kernel space ? It's also incomplete as a fix because I don't see what guarantees the buffer size will always exceed cache line size Alan - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, Dec 21, 2007 at 10:33:26AM +, Alan Cox wrote: On Fri, 21 Dec 2007 13:30:08 +1100 Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly by some low level drivers (that typically happens with USB mass storage). Should that not be fixed in USB storage by using pci_alloc_coherent on the PCI device of the hub not peeing directly into kernel space ? That's what I said, but Ben seems fixated on this particular fix. -- Intel are signing my paycheques ... these opinions are still mine Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, Dec 21, 2007 at 02:30:28PM +0100, Thomas Bogendoerfer wrote: there are SCSI host drivers, which also DMA to the sense buffer like sgiwd93.c for example. Yes ... and there are others which don't, for example qla2xxx and sym53c8xx. -- Intel are signing my paycheques ... these opinions are still mine Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, Dec 21, 2007 at 07:00:25AM -0700, Matthew Wilcox wrote: On Fri, Dec 21, 2007 at 02:30:28PM +0100, Thomas Bogendoerfer wrote: there are SCSI host drivers, which also DMA to the sense buffer like sgiwd93.c for example. Yes ... and there are others which don't, for example qla2xxx and sym53c8xx. well I don't care which way to go. I'd prefer a pointer to an allocated sense buffer, but maybe that's just me. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessary a good idea.[ RFC1925, 2.3 ] - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
It's also incomplete as a fix because I don't see what guarantees the buffer size will always exceed cache line size There's a macro trick that adds a pad member after the buffer too, so that it gets rounded up to the cacheline size: +#define __dma_aligned __attribute__((aligned(ARCH_MIN_DMA_ALIGNMENT))) +#define __dma_buffer__dma_buffer_line(__LINE__) +#define __dma_buffer_line(line) __dma_aligned;\ +char __dma_pad_##line[0] __dma_aligned So that part is OK at least. - R. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, 2007-12-21 at 10:33 +, Alan Cox wrote: On Fri, 21 Dec 2007 13:30:08 +1100 Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly by some low level drivers (that typically happens with USB mass storage). Should that not be fixed in USB storage by using pci_alloc_coherent on the PCI device of the hub not peeing directly into kernel space ? All dumb SCSI drivers have this problem, USB storage is just one of them. This would also allow to remove bounce buffering that some non-dumb ones are doing in fact. There's another solution jejb was talking about involving reworking the allocation of the sense buffer to make it always under driver control etc... but that's the kind of SCSI surgery that I'm not prepared to do especially not for .25 and without much HW to test with. It's also incomplete as a fix because I don't see what guarantees the buffer size will always exceed cache line size How is that a problem ? The annotation will make sure the buffer doesn't share cache lines. It forces alignmenet before and pads after. Ben. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, 2007-12-21 at 06:16 -0700, Matthew Wilcox wrote: On Fri, Dec 21, 2007 at 10:33:26AM +, Alan Cox wrote: On Fri, 21 Dec 2007 13:30:08 +1100 Benjamin Herrenschmidt [EMAIL PROTECTED] wrote: The sense buffer ins scsi_cmnd can nowadays be DMA'ed into directly by some low level drivers (that typically happens with USB mass storage). Should that not be fixed in USB storage by using pci_alloc_coherent on the PCI device of the hub not peeing directly into kernel space ? That's what I said, but Ben seems fixated on this particular fix. That means more understanding of the SCSI code than I have right now and a _lot_ more time than I have right now. It's not only USB storage, it's anything that does DMA and uses the generic error handling. Ben. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html