On Fri, Dec 21 2007 at 4:30 +0200, Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> 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 <benh@xxxxxxxxxxxxxxxxxxx>
> ---
>
> 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.000000000 +1100
> +++ linux-merge/include/scsi/scsi_cmnd.h 2007-12-21 13:07:29.000000000 +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.
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
