[lkml]   [2007]   [Dec]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [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 <> 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 <>
    > ---
    > 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

    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.

    [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.


     \ /
      Last update: 2007-12-23 12:17    [W:0.036 / U:1.656 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site