lkml.org 
[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
From
Date

> 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



\
 
 \ /
  Last update: 2007-12-24 00:09    [W:0.119 / U:1.944 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site