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
>
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
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-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
> 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. The hole would only be present on non coherent architectures though. We also still need the padding after the sense_buffer. But yeah, moving the sense buffer to first position would diminish the size of the hole I suppose. > 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. Re-oreder isn't enough. We need both re-order and the __dma_buffer annotation to ensure proper padding. Some archs have 64 or even 128 bytes cache line size. I agree that in the long run, a better solution should be done involving reworking the way the sense buffer is allocated. I started modifying the EH code to kmalloc it, but it gets very messy with the current interface between drivers and EH. I still have some patches around I can send, though some drivers need fixing (error handling on kmalloc failure). James seem to have a better idea of pre-allocating sense buffers per host based on how many they can have in flight which I haven't looked at. Ben. > 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. I think that's pretty much was James was proposing indeed. > 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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
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. The hole would only be present on non coherent architectures though. We also still need the padding after the sense_buffer. But yeah, moving the sense buffer to first position would diminish the size of the hole I suppose. 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. Re-oreder isn't enough. We need both re-order and the __dma_buffer annotation to ensure proper padding. Some archs have 64 or even 128 bytes cache line size. I agree that in the long run, a better solution should be done involving reworking the way the sense buffer is allocated. I started modifying the EH code to kmalloc it, but it gets very messy with the current interface between drivers and EH. I still have some patches around I can send, though some drivers need fixing (error handling on kmalloc failure). James seem to have a better idea of pre-allocating sense buffers per host based on how many they can have in flight which I haven't looked at. Ben. 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. I think that's pretty much was James was proposing indeed. 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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, Dec 21, 2007 at 06:16:41AM -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. there are SCSI host drivers, which also DMA to the sense buffer like sgiwd93.c for example. 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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
On Fri, Dec 21, 2007 at 06:16:41AM -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. there are SCSI host drivers, which also DMA to the sense buffer like sgiwd93.c for example. 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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
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) */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd
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) */
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
