[PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd - Kernel

This is a discussion on [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 14 of 14

Thread: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd

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

    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) */
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. 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 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. 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 +0000, Alan Cox wrote:
    > On Fri, 21 Dec 2007 13:30:08 +1100
    > 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).

    >
    > 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. 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 +0000, Alan Cox wrote:
    > > On Fri, 21 Dec 2007 13:30:08 +1100
    > > 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).

    > >
    > > 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. 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 +0000, Alan Cox wrote:
    > > On Fri, 21 Dec 2007 13:30:08 +1100
    > > 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).

    > >
    > > 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd


    On Fri, 2007-12-21 at 10:33 +0000, Alan Cox wrote:
    > On Fri, 21 Dec 2007 13:30:08 +1100
    > 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).

    >
    > 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. 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 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
    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. 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 wrote:

    > 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
    > 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  13. 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 wrote:
    > On Sun, 23 Dec 2007 13:09:05 +0200
    > Boaz Harrosh wrote:
    >
    >> 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
    >> 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. Where cmnd is re-used to map the REQUEST_SENSE read.

    4. Do nothing and let scsi_error issue a request sense asynchronously.

    What I did until now is, added a new API for case 1 - scsi_eh_cpy_sense(...) ,
    and 3,4 are covered by the existing (new) APIs. Case-2 is hardest and I'll
    fix it per-driver-case, like in iscsi it was very easy as it already have
    a per command structure allocated.

    I have not yet converted the scsi midlayer to any new system but my plan
    is:

    Stage 1
    1. convert all drivers to some new/old API where:
    Those drivers with case-2 above, that can DMA_FROM_DEVICE
    concurrently into more than one sense buffer, will change
    specifically, to pre-allocate a can_queue number of buffers.

    2. Change mid-layer to:
    a. Pre-allocate one dma-able sense buffer per Q.
    b. copy sense out of above, directly into ULD's allocated sense
    buffer which was put on cmnd->request->sense
    c. Those very *few* requests that come without ULD allocated
    sense, allocate one for them out of a mempool.
    (All regular file-system ULDs allocate a sense buffer)

    Stage 2
    I think I can see a way how to shuffle the mid-layer post-sense processing
    directly in copy-sense time. So in case ULD did not "request sense" the
    buffer can be discarded. But I'm not 100% sure about that.

    Let me write up a sketch of what I mean which will include 3 drivers
    that demonstrate above cases, and the mid-layer changes.
    Then we can discus it and see if it makes sense () to every body.
    But let me have time until tomorrow night for that.

    Boaz

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  14. 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 wrote:

    > On Mon, Jan 07 2008 at 8:53 +0200, FUJITA Tomonori wrote:
    > > On Sun, 23 Dec 2007 13:09:05 +0200
    > > Boaz Harrosh wrote:
    > >
    > >> 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
    > >> 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. 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)


    Not all of them are autosense. #2 is not related with autosense.

    I think that the point is that #1 and #2 might use as many sense
    buffers as can_queue. The differences are:

    - For #1, we can attach sense_buffer only when necessary.

    - For #2, we always need to attach sense_buffer before queuecommand.


    > 3. Synchronous request for sense, like the places I sent patches
    > for. Where cmnd is re-used to map the REQUEST_SENSE read.
    >
    > 4. Do nothing and let scsi_error issue a request sense asynchronously.
    >
    > What I did until now is, added a new API for case 1 - scsi_eh_cpy_sense(...) ,
    > and 3,4 are covered by the existing (new) APIs. Case-2 is hardest and I'll
    > fix it per-driver-case, like in iscsi it was very easy as it already have
    > a per command structure allocated.


    > I have not yet converted the scsi midlayer to any new system but my plan
    > is:
    >
    > Stage 1
    > 1. convert all drivers to some new/old API where:
    > Those drivers with case-2 above, that can DMA_FROM_DEVICE
    > concurrently into more than one sense buffer, will change
    > specifically, to pre-allocate a can_queue number of buffers.


    scsi-ml needs more sense buffers than can_queue since
    scsi_finish_command uses sense_buffer after calling
    scsi_device_unbusy.


    > 2. Change mid-layer to:
    > a. Pre-allocate one dma-able sense buffer per Q.
    > b. copy sense out of above, directly into ULD's allocated sense
    > buffer which was put on cmnd->request->sense
    > c. Those very *few* requests that come without ULD allocated
    > sense, allocate one for them out of a mempool.
    > (All regular file-system ULDs allocate a sense buffer)
    >
    > Stage 2
    > I think I can see a way how to shuffle the mid-layer post-sense processing
    > directly in copy-sense time. So in case ULD did not "request sense" the
    > buffer can be discarded. But I'm not 100% sure about that.
    >
    > Let me write up a sketch of what I mean which will include 3 drivers
    > that demonstrate above cases, and the mid-layer changes.
    > Then we can discus it and see if it makes sense () to every body.
    > But let me have time until tomorrow night for that.


    I see. I'll wait for your patches.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread