James.Bottomley@xxxxxxxxxxxxxxxxxxxxx wrote on Tue, 01 Jan 2008 10:27 -0600:
> On Tue, 2008-01-01 at 11:08 -0500, Pete Wyckoff wrote:
> > James.Bottomley@xxxxxxxxxxxxxxxxxxxxx wrote on Mon, 31 Dec 2007 16:43 -0600:
> > > This patch relaxes the default SCSI DMA alignment from 512 bytes to 4
> > > bytes. I remember from previous discussions that usb and firewire have
> > > sector size alignment requirements, so I upped their alignments in the
> > > respective slave allocs.
> > >
> > > The reason for doing this is so that we don't get such a huge amount of
> > > copy overhead in bio_copy_user() for udev. (basically all inquiries it
> > > issues can now be directly mapped).
> >
> > Great change. Here's another patch. It allows a queue
> > dma_alignment of 0 bytes, for drivers that can move data
> > at byte granularity.
> >
> > Two drivers try to turn off DMA alignment restrictions by
> > setting the dma_alignment to zero:
> >
> > drivers/scsi/iscsi_tcp.c: blk_queue_dma_alignment(sdev->request_queue, 0);
> > drivers/scsi/scsi_tgt_lib.c: blk_queue_dma_alignment(q, 0);
> >
> > But they end up doing copies due to the way that queue_dma_alignment()
> > returns 511 in this case.
[..]
> > From 90ee6d61ad71a024815eee2b416edb40c6b01256 Mon Sep 17 00:00:00 2001
> > From: Pete Wyckoff <pw@xxxxxxx>
> > Date: Tue, 1 Jan 2008 10:23:02 -0500
> > Subject: [PATCH] block: allow queue dma_alignment of zero
> >
> > Let queue_dma_alignment return 0 if it was specifically set to 0.
> > This permits devices with no particular alignment restrictions to
> > use arbitrary user space buffers without copying.
> >
> > Signed-off-by: Pete Wyckoff <pw@xxxxxxx>
> > ---
> > include/linux/blkdev.h | 7 +------
> > 1 files changed, 1 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index aa3090c..eea31c2 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -860,12 +860,7 @@ static inline int bdev_hardsect_size(struct block_device *bdev)
> >
> > static inline int queue_dma_alignment(struct request_queue *q)
> > {
> > - int retval = 511;
> > -
> > - if (q && q->dma_alignment)
> > - retval = q->dma_alignment;
> > -
> > - return retval;
> > + return q ? q->dma_alignment : 511;
> > }
> >
> > /* assumes size > 256 */
>
> This is certainly a possible patch. What assurance do you have that it
> doesn't upset block devices who set zero assuming they get the default
> alignment?
Code inspection of the initialization and use of this field. I hope
anybody who sees a mistake here will point it out.
1. Initialization
Most users call blk_init_queue{,_node} to alloc and initialize a new
request_queue. This leads to initialization of dma_alignment to 511
in blk_queue_make_request().
The rest of the callers use blk_alloc_queue{,_node}. Most of those
call blk_queue_make_request() with a custom make_request_fn a few
lines later, similarly causing dma_alignment to be initialized to
non-zero. The loop and pktcdvd drivers require a bit of reading to
convince oneself, but also can be seen to call
blk_queue_make_request() before using the queue.
2. Use of blk_queue_dma_alignment() to set, queue_dma_alignment() and
->dma_alignment to get
Inspection of the 20-odd spots that match "dma_alignment" shows that
none of them set zero to expect the default.
Definitely a valid concern, but it seems to be a safe change, at
least for in-tree users. Do you think a new request_queue flag for
zero-aware drivers and a WARN_ON that checks for zero and !zero_aware
would be worthwhile?
Without this change, we can go as low as two-byte alignment on
buffer start and length by setting dma_alignment to 1, but will do a
full copy if (buffer&1) or (length&1).
-- Pete
-
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
