From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755884AbXLWLOy (ORCPT ); Sun, 23 Dec 2007 06:14:54 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751355AbXLWLOp (ORCPT ); Sun, 23 Dec 2007 06:14:45 -0500 Received: from gw-colo-pa.panasas.com ([66.238.117.130]:18034 "EHLO cassoulet.panasas.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750850AbXLWLOn (ORCPT ); Sun, 23 Dec 2007 06:14:43 -0500 Message-ID: <476E41D1.50606@panasas.com> Date: Sun, 23 Dec 2007 13:09:05 +0200 From: Boaz Harrosh User-Agent: Thunderbird 2.0.0.9 (X11/20071031) MIME-Version: 1.0 To: Benjamin Herrenschmidt , james.bottomley@steeleye.com CC: linux-kernel@vger.kernel.org, Andrew Morton , rdreier@cisco.com, linux-scsi@vger.kernel.org, rmk@arm.linux.org.uk, davem@davemloft.net, ralf@linux-mips.org, Christoph Hellwig Subject: Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd References: <20071221023011.4C4BADDDFA@ozlabs.org> In-Reply-To: <20071221023011.4C4BADDDFA@ozlabs.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 23 Dec 2007 11:09:09.0285 (UTC) FILETIME=[3D95B550:01C84554] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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