sg_ring: a ring of scatterlist arrays
Subsystems (13)
cover sg_ring: a ring of scatterlist arrays
This patch series is the start of my attempt to simplify and make explicit the chained scatterlist logic.
It's not complete: my SATA box boots and seems happy, but all the other users of SCSI need to be updated and checked. But I've gotten far enough to believe it's worth persuing.
Cheers,
Rusty.
PATCH 1/7 sg_ring: introduce sg_ring: a ring of scatterlist arrays.
This patch introduces 'struct sg_ring', a layer on top of scatterlist arrays. It meshes nicely with routines which expect a simple array of 'struct scatterlist' because it is easy to break down the ring into its constituent arrays.
The sg_ring header also encodes the maximum number of entries, useful for routines which populate an sg. We need never hand around a number of elements any more.
--- include/linux/sg_ring.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 74 insertions(+), 0 deletions(-) create mode 100644 include/linux/sgring.h
include/linux/sg_ring.h new file +107 -0
@@ -0,0 +1,107 @@ +#ifndef _LINUX_SG_RING_H +#define _LINUX_SG_RING_H +#include <linux/scatterlist.h> + +/** +* struct sg_ring - a ring of scatterlists +* @list: the list_head chaining them together +* @num: the number of valid sg entries +* @max: the maximum number of sg entries (size of the sg array). +* @sg: the array of scatterlist entries. +* +* This provides a convenient encapsulation of one or more scatter gather +* arrays. +*/ +struct sg_ring +{ +struct list_head list; +unsigned int num, max; +struct scatterlist sg[0 ]; +}; + +/* This helper declares an sg ring on the stack or in a struct. */ +#define DECLARE_SG_RING (name, max) \ + struct { \ + struct sg_ring ring; \ + struct scatterlist sg[max]; \ + } name + +/** +* sg_ring_init - initialize a scatterlist ring. +* @sg: the sg_ring. +* @max: the size of the trailing sg array. +* +* After initialization sg is alone in the ring. +*/ +static inline void sg_ring_init (struct sg_ring *sg,unsigned int max) +{ +#ifdef CONFIG_DEBUG_SG +unsigned int i; +for (i =0 ; i < max; i++) + sg->sg[i].sg_magic =SG_MAGIC ; +#endif +INIT_LIST_HEAD (&sg->list); + sg->max = max; +/* FIXME: This is to clear the page bits. */ +sg_init_table (sg->sg, sg->max); +} + +/** +* sg_ring_single - initialize a one-element scatterlist ring. +* @sg: the sg_ring. +* @buf: the pointer to the buffer. +* @buflen: the length of the buffer. +* +* Does sg_ring_init and also sets up first (and only) sg element. +*/ +static inline void sg_ring_single (struct sg_ring *sg, +const void *buf, +unsigned int buflen) +{ +sg_ring_init (sg,1 ); + sg->num =1 ; +sg_init_one (&sg->sg[0 ], buf, buflen); +} + +/** +* sg_ring_next - next array in a scatterlist ring. +* @sg: the sg_ring. +* @head: the sg_ring head. +* +* This will return NULL once @sg has looped back around to @head. +*/ +static inline struct sg_ring *sg_ring_next (struct sg_ring *sg, +const struct sg_ring *head) +{ + sg = list_first_entry(&sg->list, struct sg_ring, list); +if (sg == head) + sg =NULL ; +return sg; +} + +/* Helper for writing for loops */ +static inline struct sg_ring *sg_ring_iter (const struct sg_ring *head, +struct sg_ring *sg,unsigned int *i) +{ + (*i)++; +/* While loop lets us skip any zero-entry sg_ring arrays */ +while (*i == sg->num) { + *i =0 ; + sg =sg_ring_next (sg, head); +if (!sg) +break ; + } +return sg; +} + +/** +* sg_ring_for_each - iterate through an entire sg_ring ring +* @head: the head of the sg_ring. +* @sg: the sg_ring iterator. +* @i: an (unsigned) integer which refers to sg->sg[i]. +* +* The current scatterlist element is sg->sg[i]. +*/ +#define sg_ring_for_each (head, sg, i) \ + for (sg = head, i = 0; sg; sg = sg_ring_iter(head, sg, &i)) +#endif /* _LINUX_SG_RING_H */
PATCH 2/7 sg_ring: use in virtio.
Using sg_rings, we can join together scatterlists returned by other subsystems, without needing to allocate an extra element for chaining. This helps the virtio_blk device which wants to prepend and append metadata to the request's scatterlist.
As an added bonus, the old virtio layer used to pass a scatterlist array and two numbers indicating the number of readable and writable elements respectively; now we can simply hand two sg_rings which is much clearer (each sg_ring contains its own length).
--- drivers/block/virtio_blk.c | 33 ++++++++------- drivers/char/virtio_console.c | 13 +++--- drivers/net/virtio_net.c | 22 +++++----- drivers/virtio/virtio_ring.c | 90 ++++++++++++++++++++++++++-------------- include/linux/virtio.h | 12 ++--- 5 files changed, 99 insertions(+), 71 deletions(-)
drivers/block/virtio_blk.c +17 -16
@@ -5,8 +5,6 @@#include <linux/virtio.h> #include <linux/virtio_blk.h> #include <linux/scatterlist.h> - -#define VIRTIO_MAX_SG (3+MAX_PHYS_SEGMENTS)static unsigned char virtblk_index ='a' ;struct virtio_blk
@@ -24,8 +22,10 @@ struct virtio_blkmempool_t *pool; -/* Scatterlist: can be too big for stack. */ -struct scatterlist sg[VIRTIO_MAX_SG ]; +/* Scatterlist ring: can be too big for stack. */ +DECLARE_SG_RING (out,1 ); +DECLARE_SG_RING (in,1 ); +DECLARE_SG_RING (sg,MAX_PHYS_SEGMENTS ); };struct virtblk_req
@@ -70,8 +70,8 @@ static bool do_req(struct request_queuestatic bool do_req (struct request_queue *q,struct virtio_blk *vblk,struct request *req) { -unsigned long num, out, in;struct virtblk_req *vbr; +struct sg_ring *in; vbr =mempool_alloc (vblk->pool,GFP_ATOMIC );if (!vbr)
@@ -95,23 +95,24 @@ static bool do_req(struct request_queueif (blk_barrier_rq (vbr->req)) vbr->out_hdr.type |=VIRTIO_BLK_T_BARRIER ; -/* This init could be done at vblk creation time */ -sg_init_table (vblk->sg,VIRTIO_MAX_SG ); -sg_set_buf (&vblk->sg[0 ], &vbr->out_hdr,sizeof (vbr->out_hdr)); - num =blk_rq_map_sg (q, vbr->req, vblk->sg+1 ); -sg_set_buf (&vblk->sg[num+1 ], &vbr->in_hdr,sizeof (vbr->in_hdr)); +sg_ring_single (&vblk->out.ring, &vbr->out_hdr,sizeof (vbr->out_hdr)); +sg_ring_init (&vblk->sg.ring,ARRAY_SIZE (vblk->sg.sg)); + vblk->sg.ring.num =blk_rq_map_sg (q, vbr->req, vblk->sg.sg); +sg_ring_single (&vblk->in.ring, &vbr->in_hdr,sizeof (vbr->in_hdr));if (rq_data_dir (vbr->req) ==WRITE ) { vbr->out_hdr.type |=VIRTIO_BLK_T_OUT ; - out =1 + num; - in =1 ; +/* Chain write request onto output buffers. */ +list_add_tail (&vblk->sg.ring.list, &vblk->out.ring.list); + in = &vblk->in.ring; }else { vbr->out_hdr.type |=VIRTIO_BLK_T_IN ; - out =1 ; - in =1 + num; +/* Chain input (status) buffer at end of read buffers. */ +list_add_tail (&vblk->in.ring.list, &vblk->sg.ring.list); + in = &vblk->sg.ring; } -if (vblk->vq->vq_ops->add_buf (vblk->vq, vblk->sg, out, in, vbr)) { +if (vblk->vq->vq_ops->add_buf (vblk->vq, &vblk->out.ring, in, vbr)) {mempool_free (vbr, vblk->pool);return false; }
@@ -128,7 +129,7 @@ static void do_virtblk_request(struct rewhile ((req =elv_next_request (q)) !=NULL ) { vblk = req->rq_disk->private_data; -BUG_ON (req->nr_phys_segments >ARRAY_SIZE (vblk->sg)); +BUG_ON (req->nr_phys_segments >ARRAY_SIZE (vblk->sg.sg));/* If this request fails, stop queue and wait for something to finish to restart it. */
drivers/char/virtio_console.c +7 -6
@@ -54,15 +54,15 @@ static struct hv_ops virtio_cons;* immediately (lguest's Launcher does). */ static int put_chars (u32 vtermno,const char *buf,int count) { -struct scatterlist sg[1 ]; +DECLARE_SG_RING (sg,1 );unsigned int len;/* This is a convenient routine to initialize a single-elem sg list */ -sg_init_one (sg, buf, count); +sg_ring_single (&sg.ring, buf, count);/* add_buf wants a token to identify this buffer: we hand it any * non-NULL pointer, since there's only ever one buffer. */ -if (out_vq->vq_ops->add_buf (out_vq, sg,1 ,0 , (void *)1 ) ==0 ) { +if (out_vq->vq_ops->add_buf (out_vq, &sg.ring,NULL , (void *)1 ) ==0 ) {/* Tell Host to go! */ out_vq->vq_ops->kick (out_vq);/* Chill out until it's done with the buffer. */
@@ -78,11 +78,12 @@ static int put_chars(u32 vtermno, const* queue. */ static void add_inbuf (void ) { -struct scatterlist sg[1 ]; -sg_init_one (sg, inbuf,PAGE_SIZE ); +DECLARE_SG_RING (sg,1 ); + +sg_ring_single (&sg.ring, inbuf,PAGE_SIZE );/* We should always be able to add one buffer to an empty queue. */ -if (in_vq->vq_ops->add_buf (in_vq, sg,0 ,1 , inbuf) !=0 ) +if (in_vq->vq_ops->add_buf (in_vq,NULL , &sg.ring, inbuf) !=0 )BUG (); in_vq->vq_ops->kick (in_vq); }
drivers/net/virtio_net.c +13 -13
@@ -211,21 +211,21 @@ static void try_fill_recv(struct virtnetstatic void try_fill_recv (struct virtnet_info *vi) {struct sk_buff *skb; -struct scatterlist sg[1 +MAX_SKB_FRAGS ]; -int num, err; +DECLARE_SG_RING (sg,1 +MAX_SKB_FRAGS ); +int err; -sg_init_table (sg,1 +MAX_SKB_FRAGS ); +sg_ring_init (&sg.ring,1 +MAX_SKB_FRAGS );for (;;) { skb =netdev_alloc_skb (vi->dev,MAX_PACKET_LEN );if (unlikely (!skb))break ;skb_put (skb,MAX_PACKET_LEN ); -vnet_hdr_to_sg (sg, skb); - num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ; +vnet_hdr_to_sg (sg.sg, skb); + sg.ring.num =skb_to_sgvec (skb, sg.sg+1 ,0 , skb->len) +1 ;skb_queue_head (&vi->recv, skb); - err = vi->rvq->vq_ops->add_buf (vi->rvq, sg,0 , num, skb); + err = vi->rvq->vq_ops->add_buf (vi->rvq,NULL , &sg.ring, skb);if (err) {skb_unlink (skb, &vi->recv);kfree_skb (skb);
@@ -304,13 +304,13 @@ static int start_xmit(struct sk_buff *skstatic int start_xmit (struct sk_buff *skb,struct net_device *dev) {struct virtnet_info *vi =netdev_priv (dev); -int num, err; -struct scatterlist sg[1 +MAX_SKB_FRAGS ]; +int err; +DECLARE_SG_RING (sg,1 +MAX_SKB_FRAGS );struct virtio_net_hdr *hdr;const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;DECLARE_MAC_BUF (mac); -sg_init_table (sg,1 +MAX_SKB_FRAGS ); +sg_ring_init (&sg.ring,1 +MAX_SKB_FRAGS );pr_debug ("%s: xmit %p %s\n" , dev->name, skb,print_mac (mac, dest));
@@ -345,13 +345,13 @@ static int start_xmit(struct sk_buff *sk hdr->gso_size =0 ; } -vnet_hdr_to_sg (sg, skb); - num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ; +vnet_hdr_to_sg (sg.sg, skb); + sg.ring.num =skb_to_sgvec (skb, sg.sg+1 ,0 , skb->len) +1 ;__skb_queue_head (&vi->send, skb); - vi->stats.sendq_sglen += num; + vi->stats.sendq_sglen += sg.ring.num; again: - err = vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); + err = vi->svq->vq_ops->add_buf (vi->svq, &sg.ring,NULL , skb);if (err) { vi->stats.sendq_full++;
drivers/virtio/virtio_ring.c +50 -30
104 lines not on this page · raw message
include/linux/virtio.h +5 -7
@@ -3,7 +3,7 @@/* Everything a virtio driver needs to work with any particular virtio * implementation. */ #include <linux/types.h> -#include <linux/scatterlist.h> +#include <linux/sg_ring.h> #include <linux/spinlock.h> #include <linux/device.h> #include <linux/mod_devicetable.h>
@@ -27,9 +27,8 @@ struct virtqueue* virtqueue_ops - operations for virtqueue abstraction layer * @add_buf: expose buffer to other end * vq: the struct virtqueue we're talking about. -* sg: the description of the buffer(s). -* out_num: the number of sg readable by other side -* in_num: the number of sg which are writable (after readable ones) +* out: the scatter gather elements readable by other side (can be NULL) +* in: the scatter gather elements which are writable (can be NULL) * data: the token identifying the buffer. * Returns 0 or an error. * @kick: update after add_buf
@@ -56,9 +55,8 @@ struct virtqueue*/ struct virtqueue_ops {int (*add_buf)(struct virtqueue *vq, -struct scatterlist sg[], -unsigned int out_num, -unsigned int in_num, +struct sg_ring *out, +struct sg_ring *in,void *data);void (*kick)(struct virtqueue *vq);
net/9p/trans_virtio.c +6 -6
@@ -88,7 +88,7 @@ static int p9_virtio_write(struct p9_tra {struct virtio_chan *chan = (struct virtio_chan *) trans->priv;struct virtqueue *out_vq = chan->out_vq; -struct scatterlist sg[1 ]; +DECLARE_SG_RING (sg,1 );unsigned int len;P9_DPRINTK (P9_DEBUG_TRANS ,"9p debug: virtio write (%d)\n" , count);
@@ -97,11 +97,11 @@ static int p9_virtio_write(struct p9_traif (rest_of_page (buf) < count) count =rest_of_page (buf); -sg_init_one (sg, buf, count); +sg_ring_single (&sg.ring, buf, count);/* add_buf wants a token to identify this buffer: we hand it any * non-NULL pointer, since there's only ever one buffer. */ -if (out_vq->vq_ops->add_buf (out_vq, sg,1 ,0 , (void *)1 ) ==0 ) { +if (out_vq->vq_ops->add_buf (out_vq, &sg.ring,NULL , (void *)1 ) ==0 ) {/* Tell Host to go! */ out_vq->vq_ops->kick (out_vq);/* Chill out until it's done with the buffer. */
@@ -119,12 +119,12 @@ static int p9_virtio_write(struct p9_tra* queue. */ static void add_inbuf (struct virtio_chan *chan) { -struct scatterlist sg[1 ]; +DECLARE_SG_RING (sg,1 ); -sg_init_one (sg, chan->inbuf,PAGE_SIZE ); +sg_ring_single (&sg.ring, chan->inbuf,PAGE_SIZE );/* We should always be able to add one buffer to an empty queue. */ -if (chan->in_vq->vq_ops->add_buf (chan->in_vq, sg,0 ,1 , chan->inbuf)) +if (chan->in_vq->vq_ops->add_buf (chan->in_vq,&sg.ring,NULL ,chan->inbuf))BUG (); chan->in_vq->vq_ops->kick (chan->in_vq); }
PATCH 3/7 sg_ring: blk_rq_map_sg_ring as a counterpart to blk_rq_map_sg.
blk_rq_map_sg_ring as a counterpart to blk_rq_map_sg.
Obvious counterpart to blk_rq_map_sg.
--- block/ll_rw_blk.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/blkdev.h | 1 + 2 files changed, 56 insertions(+), 0 deletions(-)
block/ll_rw_blk.c +63 -0
@@ -31,6 +31,7 @@#include <linux/blktrace_api.h> #include <linux/fault-inject.h> #include <linux/scatterlist.h> +#include <linux/sg_ring.h> /* * for max sense size
@@ -1364,6 +1365,68 @@ new_segment:EXPORT_SYMBOL (blk_rq_map_sg); +/** +* blk_rq_map_sg_ring - map a request to a scatterlist ring. +* @q: the request queue this request applies to. +* @rq: the request to map +* @sg: the sg_ring to populate. +* +* There must be enough elements in the sg_ring(s) to map the request. +*/ +void blk_rq_map_sg_ring (struct request_queue *q,struct request *rq, +struct sg_ring *sg) +{ +struct bio_vec *bvec, *bvprv; +struct req_iterator iter; +int i, cluster; +struct sg_ring *head = sg; +struct scatterlist *sgprv; + + i =0 ; + cluster = q->queue_flags & (1 <<QUEUE_FLAG_CLUSTER ); + +/* +* for each bio in rq +*/ + bvprv =NULL ; + sgprv =NULL ; +rq_for_each_segment (bvec, rq, iter) { +int nbytes = bvec->bv_len; + +if (bvprv && cluster) { +if (sgprv->length + nbytes > q->max_segment_size) + goto new_segment; + +if (!BIOVEC_PHYS_MERGEABLE (bvprv, bvec)) + goto new_segment; +if (!BIOVEC_SEG_BOUNDARY (q, bvprv, bvec)) + goto new_segment; + + sgprv->length += nbytes; + }else { +new_segment: +sg_set_page (sg->sg + i, bvec->bv_page, nbytes, + bvec->bv_offset); + sgprv = sg->sg + i; +if (++i == sg->max) { + sg->num = i; + sg =sg_ring_next (sg, head); + i =0 ; + } + } + bvprv = bvec; + }/* segments in rq */ + +/* If we were still working on an sg_ring, set the number and +* clear any following sg_rings. */ +if (sg) { + sg->num = i; +for (sg =sg_ring_next (sg,head); sg; sg =sg_ring_next (sg,head)) + sg->num =0 ; + } +} +EXPORT_SYMBOL (blk_rq_map_sg_ring); +/* * the standard queue merge functions, can be overridden with device * specific ones if so desired
include/linux/blkdev.h +2 -0
@@ -777,6 +777,8 @@ extern void blk_ordered_complete_seq(strextern void blk_ordered_complete_seq (struct request_queue *,unsigned ,int );extern int blk_rq_map_sg (struct request_queue *,struct request *,struct scatterlist *); +struct sg_ring ; +extern void blk_rq_map_sg_ring (struct request_queue *,struct request *,struct sg_ring *);extern void blk_dump_rq_flags (struct request *,char *);extern void generic_unplug_device (struct request_queue *);extern void __generic_unplug_device (struct request_queue *);
PATCH 4/7 sg_ring: dma_map_sg_ring() helper
Obvious counterpart to dma_map_sg. Note that this is arch-independent code; sg_rings are backwards compatible with simple sg arrays.
--- drivers/base/dma-mapping.c | 13 +++++++++++++ include/linux/dma-mapping.h | 4 ++++ 2 files changed, 17 insertions(+), 0 deletions(-)
drivers/base/dma-mapping.c +54 -0
@@ -8,6 +8,7 @@*/ #include <linux/dma-mapping.h> +#include <linux/sg_ring.h> /* * Managed DMA API
@@ -162,6 +163,59 @@ void dmam_free_noncoherent(struct device }EXPORT_SYMBOL (dmam_free_noncoherent); +/** +* dma_map_sg_ring - Map an entire sg ring +* @dev: Device to free noncoherent memory for +* @sg: The sg_ring +* @direction: DMA_TO_DEVICE, DMA_FROM_DEVICE or DMA_BIDIRECTIONAL. +* +* This returns -ENOMEM if mapping fails. It's not clear that telling you +* it failed is useful though. +*/ +int dma_map_sg_ring (struct device *dev,struct sg_ring *sg, +enum dma_data_direction direction) +{ +struct sg_ring *i; +unsigned int num; + +for (i = sg; i; i =sg_ring_next (i, sg)) { +BUG_ON (i->num > i->max); + num =dma_map_sg (dev, i->sg, i->num, direction); +if (num ==0 && i->num !=0 ) + goto unmap; + } +return 0 ; + +unmap: +while (sg) { +dma_unmap_sg (dev, sg->sg, sg->num, direction); + sg =sg_ring_next (sg, i); + } +return -ENOMEM ; + +} +EXPORT_SYMBOL (dma_map_sg_ring); + +/** +* dma_unmap_sg_ring - Unmap an entire sg ring +* @dev: Device to free noncoherent memory for +* @sg: The sg_ring +* @direction: DMA_TO_DEVICE, DMA_FROM_DEVICE or DMA_BIDIRECTIONAL. +* +* Call after dma_map_sg_ring() succeeds. +*/ +void dma_unmap_sg_ring (struct device *dev,struct sg_ring *sg, +enum dma_data_direction direction) +{ +struct sg_ring *i; + +for (i = sg; i; i =sg_ring_next (i, sg)) { +BUG_ON (i->num > i->max); +dma_unmap_sg (dev, i->sg, i->num, direction); + } +} +EXPORT_SYMBOL (dma_unmap_sg_ring); +#ifdef ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY static void dmam_coherent_decl_release (struct device *dev,void *res)
include/linux/dma-mapping.h +6 -0
@@ -87,6 +87,12 @@ dma_mark_declared_memory_occupied(struct }#endif +struct sg_ring ; +extern int dma_map_sg_ring (struct device *dev,struct sg_ring *sg, +enum dma_data_direction direction); +extern void dma_unmap_sg_ring (struct device *dev,struct sg_ring *sg, +enum dma_data_direction direction); +/* * Managed DMA API */
PATCH 5/7 sg_ring: Convert core scsi code to sg_ring.
If nothing else, the simplification of this logic shows why I prefer sg_ring over scatterlist chaining.
drivers/block/cciss_scsi.c +11 -11
@@ -1191,28 +1191,28 @@ cciss_scatter_gather(struct pci_dev *pde struct scsi_cmnd *cmd) {unsigned int len; -struct scatterlist *sg; +struct sg_ring *sg;__u64 addr64; -int use_sg, i; +int count =0 , i; -BUG_ON (scsi_sg_count (cmd) >MAXSGENTRIES ); - - use_sg =scsi_dma_map (cmd); -if (use_sg) {/* not too many addrs? */ -scsi_for_each_sg (cmd, sg, use_sg, i) { - addr64 = (__u64 )sg_dma_address (sg); - len =sg_dma_len (sg); +if (scsi_dma_map (cmd) >=0 ) {/* not too many addrs? */ +sg_ring_for_each (scsi_sgring (cmd), sg, i) { + addr64 = (__u64 )sg_dma_address (&sg->sg[i]); + len =sg_dma_len (&sg->sg[i]); cp->SG[i].Addr.lower = (__u32) (addr64 & (__u64 )0x00000000FFFFFFFF ); cp->SG[i].Addr.upper = (__u32) ((addr64 >>32 ) & (__u64 )0x00000000FFFFFFFF ); cp->SG[i].Len = len; cp->SG[i].Ext =0 ;// we are not chaining + count++; } } - cp->Header.SGList = (__u8 ) use_sg;/* no. SGs contig in this cmd */ - cp->Header.SGTotal = (__u16 ) use_sg;/* total sgs in this cmd list */ + cp->Header.SGList = (__u8 ) count;/* no. SGs contig in this cmd */ + cp->Header.SGTotal = (__u16 ) count;/* total sgs in this cmd list */ + +BUG_ON (count >MAXSGENTRIES );return ; }
drivers/scsi/scsi_error.c +4 -7
@@ -619,18 +619,17 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd ses->data_direction = scmd->sc_data_direction; ses->bufflen = scmd->request_bufflen; ses->buffer = scmd->request_buffer; - ses->use_sg = scmd->use_sg; ses->resid = scmd->resid; ses->result = scmd->result;if (sense_bytes) { scmd->request_bufflen =min_t (unsigned,sizeof (scmd->sense_buffer), sense_bytes); -sg_init_one (&ses->sense_sgl, scmd->sense_buffer, - scmd->request_bufflen); - scmd->request_buffer = &ses->sense_sgl; + +sg_ring_single (&ses->sense_sg.ring, scmd->sense_buffer, + scmd->request_bufflen); + scmd->request_buffer = &ses->sense_sg; scmd->sc_data_direction =DMA_FROM_DEVICE ; - scmd->use_sg =1 ;memset (scmd->cmnd,0 ,sizeof (scmd->cmnd)); scmd->cmnd[0 ] =REQUEST_SENSE ; scmd->cmnd[4 ] = scmd->request_bufflen;
@@ -639,7 +638,6 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd scmd->request_buffer =NULL ; scmd->request_bufflen =0 ; scmd->sc_data_direction =DMA_NONE ; - scmd->use_sg =0 ;if (cmnd) {memset (scmd->cmnd,0 ,sizeof (scmd->cmnd));memcpy (scmd->cmnd, cmnd, cmnd_size);
@@ -678,7 +676,6 @@ void scsi_eh_restore_cmnd(struct scsi_cm scmd->sc_data_direction = ses->data_direction; scmd->request_bufflen = ses->bufflen; scmd->request_buffer = ses->buffer; - scmd->use_sg = ses->use_sg; scmd->resid = ses->resid; scmd->result = ses->result; }
drivers/scsi/scsi_lib.c +43 -105
239 lines not on this page · raw message
drivers/scsi/scsi_lib_dma.c +7 -13
@@ -15,22 +15,17 @@* scsi_dma_map - perform DMA mapping against command's sg lists * @cmd: scsi command * -* Returns the number of sg lists actually used, zero if the sg lists -* is NULL, or -ENOMEM if the mapping failed. +* Returns -ENOMEM if the mapping failed. */ int scsi_dma_map (struct scsi_cmnd *cmd) { -int nseg =0 ; - -if (scsi_sg_count (cmd)) { +if (scsi_sgring (cmd)) {struct device *dev = cmd->device->host->shost_gendev.parent; - nseg =dma_map_sg (dev,scsi_sglist (cmd),scsi_sg_count (cmd), - cmd->sc_data_direction); -if (unlikely (!nseg)) -return -ENOMEM ; +return dma_map_sg_ring (dev,scsi_sgring (cmd), + cmd->sc_data_direction); } -return nseg; +return 0 ; }EXPORT_SYMBOL (scsi_dma_map);
@@ -40,11 +35,10 @@ EXPORT_SYMBOL(scsi_dma_map);*/ void scsi_dma_unmap (struct scsi_cmnd *cmd) { -if (scsi_sg_count (cmd)) { +if (scsi_sgring (cmd)) {struct device *dev = cmd->device->host->shost_gendev.parent; -dma_unmap_sg (dev,scsi_sglist (cmd),scsi_sg_count (cmd), - cmd->sc_data_direction); +dma_unmap_sg_ring (dev,scsi_sgring (cmd),cmd->sc_data_direction); } }EXPORT_SYMBOL (scsi_dma_unmap);
drivers/scsi/scsi_tgt_lib.c +3 -3
@@ -358,15 +358,15 @@ static int scsi_tgt_init_cmd(struct scsistruct request *rq = cmd->request;int count; - cmd->use_sg = rq->nr_phys_segments; - cmd->request_buffer =scsi_alloc_sgtable (cmd, gfp_mask); + cmd->request_buffer =scsi_alloc_sgring (cmd, rq->nr_phys_segments, + gfp_mask);if (!cmd->request_buffer)return -ENOMEM ; cmd->request_bufflen = rq->data_len;dprintk ("cmd %p cnt %d %lu\n" , cmd, cmd->use_sg,rq_data_dir (rq)); - count =blk_rq_map_sg (rq->q, rq, cmd->request_buffer); + count =blk_rq_map_sg_ring (rq->q, rq, cmd->request_buffer);BUG_ON (count > cmd->use_sg); cmd->use_sg = count;return 0 ;
include/scsi/scsi_cmnd.h +3 -8
@@ -66,10 +66,6 @@ struct scsi_cmnd {struct timer_list eh_timeout;/* Used to time out the command. */ void *request_buffer;/* Actual requested buffer */ - -/* These elements define the operation we ultimately want to perform */ -unsigned short use_sg;/* Number of pieces of scatter-gather */ -unsigned short __use_sg;unsigned underflow;/* Return error if less than this amount is transferred */
@@ -128,14 +124,13 @@ extern void *scsi_kmap_atomic_sg(struct size_t *offset, size_t *len);extern void scsi_kunmap_atomic_sg (void *virt); -extern struct scatterlist *scsi_alloc_sgtable (struct scsi_cmnd *,gfp_t ); -extern void scsi_free_sgtable (struct scsi_cmnd *); +extern struct sg_ring *scsi_alloc_sgring (struct scsi_cmnd *,unsigned ,gfp_t ); +extern void scsi_free_sgring (struct scsi_cmnd *);extern int scsi_dma_map (struct scsi_cmnd *cmd);extern void scsi_dma_unmap (struct scsi_cmnd *cmd); -#define scsi_sg_count (cmd) ((cmd)->use_sg) -#define scsi_sglist (cmd) ((struct scatterlist *)(cmd)->request_buffer) +#define scsi_sgring (cmd) ((struct sg_ring *)(cmd)->request_buffer)#define scsi_bufflen (cmd) ((cmd)->request_bufflen)static inline void scsi_set_resid (struct scsi_cmnd *cmd,int resid)
include/scsi/scsi_eh.h +2 -3
@@ -1,7 +1,7 @@#ifndef _SCSI_SCSI_EH_H#define _SCSI_SCSI_EH_H -#include <linux/scatterlist.h> +#include <linux/sg_ring.h> #include <scsi/scsi_cmnd.h> struct scsi_device ;
@@ -75,10 +75,9 @@ struct scsi_eh_save {void *buffer;unsigned bufflen; -unsigned short use_sg;int resid; -struct scatterlist sense_sgl; +DECLARE_SG_RING (sense_sg,1 ); }; extern void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
PATCH 6/7 sg_ring: libata simplification
[This patch has been obsoleted: Tejun has a more complete one, but it's not in mainline yet, so this serves to make the next patch apply].
libata separates the single buffer case from the scatterlist case internally. It's not clear that this is necessary.
Remove the ATA_QCFLAG_SINGLE flag, and buf_virt pointer, and always initialize qc->nbytes in ata_sg_init().
It's possible that the ATA_QCFLAG_SG and ATA_QCFLAG_DMAMAP flags could be entirely removed, and we could use whether qc->__sg is NULL or not.
drivers/ata/libata-core.c +9 -103
@@ -1648,16 +1648,8 @@ unsigned ata_exec_internal_sg(struct atamemcpy (qc->cdb, cdb,ATAPI_CDB_LEN ); qc->flags |=ATA_QCFLAG_RESULT_TF ; qc->dma_dir = dma_dir; -if (dma_dir !=DMA_NONE ) { -unsigned int i, buflen =0 ; -struct scatterlist *sg; - -for_each_sg (sgl, sg, n_elem, i) - buflen += sg->length; - +if (dma_dir !=DMA_NONE )ata_sg_init (qc, sgl, n_elem); - qc->nbytes = buflen; - } qc->private_data = &wait; qc->complete_fn = ata_qc_complete_internal;
@@ -4543,9 +4535,6 @@ void ata_sg_clean(struct ata_queued_cmdWARN_ON (!(qc->flags &ATA_QCFLAG_DMAMAP ));WARN_ON (sg ==NULL ); -if (qc->flags &ATA_QCFLAG_SINGLE ) -WARN_ON (qc->n_elem >1 ); -VPRINTK ("unmapping %u sg elements\n" , qc->n_elem);/* if we padded the buffer out to 32-bit bound, and data
@@ -4566,16 +4555,6 @@ void ata_sg_clean(struct ata_queued_cmdmemcpy (addr + psg->offset, pad_buf, qc->pad_len);kunmap_atomic (addr,KM_IRQ0 ); } - }else { -if (qc->n_elem) -dma_unmap_single (ap->dev, -sg_dma_address (&sg[0 ]),sg_dma_len (&sg[0 ]), - dir); -/* restore sg */ - sg->length += qc->pad_len; -if (pad_buf) -memcpy (qc->buf_virt + sg->length - qc->pad_len, - pad_buf, qc->pad_len); } qc->flags &= ~ATA_QCFLAG_DMAMAP ;
@@ -4807,16 +4786,8 @@ void ata_noop_qc_prep(struct ata_queued_void ata_sg_init_one (struct ata_queued_cmd *qc,void *buf,unsigned int buflen) { - qc->flags |=ATA_QCFLAG_SINGLE ; - - qc->__sg = &qc->sgent; - qc->n_elem =1 ; - qc->orig_n_elem =1 ; - qc->buf_virt = buf; - qc->nbytes = buflen; - qc->cursg = qc->__sg; -sg_init_one (&qc->sgent, buf, buflen); +ata_sg_init (qc, &qc->sgent,1 ); }/**
@@ -4841,75 +4813,13 @@ void ata_sg_init(struct ata_queued_cmd * qc->n_elem = n_elem; qc->orig_n_elem = n_elem; qc->cursg = qc->__sg; -} - -/** -* ata_sg_setup_one - DMA-map the memory buffer associated with a command. -* @qc: Command with memory buffer to be mapped. -* -* DMA-map the memory buffer associated with queued_cmd @qc. -* -* LOCKING: -* spin_lock_irqsave(host lock) -* -* RETURNS: -* Zero on success, negative on error. -*/ - -static int ata_sg_setup_one (struct ata_queued_cmd *qc) -{ -struct ata_port *ap = qc->ap; -int dir = qc->dma_dir; -struct scatterlist *sg = qc->__sg; -dma_addr_t dma_address; -int trim_sg =0 ; - -/* we must lengthen transfers to end on a 32-bit boundary */ - qc->pad_len = sg->length &3 ; -if (qc->pad_len) { -void *pad_buf = ap->pad + (qc->tag *ATA_DMA_PAD_SZ ); -struct scatterlist *psg = &qc->pad_sgent; - -WARN_ON (qc->dev->class !=ATA_DEV_ATAPI ); - -memset (pad_buf,0 ,ATA_DMA_PAD_SZ ); - -if (qc->tf.flags &ATA_TFLAG_WRITE ) -memcpy (pad_buf, qc->buf_virt + sg->length - qc->pad_len, - qc->pad_len); - -sg_dma_address (psg) = ap->pad_dma + (qc->tag *ATA_DMA_PAD_SZ ); -sg_dma_len (psg) =ATA_DMA_PAD_SZ ; -/* trim sg */ - sg->length -= qc->pad_len; -if (sg->length ==0 ) - trim_sg =1 ; - -DPRINTK ("padding done, sg->length=%u pad_len=%u\n" , - sg->length, qc->pad_len); - } - -if (trim_sg) { - qc->n_elem--; - goto skip_map; - } - - dma_address =dma_map_single (ap->dev, qc->buf_virt, - sg->length, dir); -if (dma_mapping_error (dma_address)) { -/* restore sg */ - sg->length += qc->pad_len; -return -1 ; - } - -sg_dma_address (sg) = dma_address; -sg_dma_len (sg) = sg->length; - -skip_map: -DPRINTK ("mapped buffer of %d bytes for %s\n" ,sg_dma_len (sg), - qc->tf.flags &ATA_TFLAG_WRITE ?"write" :"read" ); - -return 0 ; + + qc->nbytes =0 ; +while (n_elem) { + qc->nbytes += sg->length; + n_elem--; + sg =sg_next (sg); + } }/**
@@ -6028,9 +5938,6 @@ void ata_qc_issue(struct ata_queued_cmdif (ata_should_dma_map (qc)) {if (qc->flags &ATA_QCFLAG_SG ) {if (ata_sg_setup (qc)) - goto sg_err; - }else if (qc->flags &ATA_QCFLAG_SINGLE ) { -if (ata_sg_setup_one (qc)) goto sg_err; } }else {
include/linux/libata.h +1 -3
@@ -216,8 +216,7 @@ enum {/* struct ata_queued_cmd flags */ ATA_QCFLAG_ACTIVE = (1 <<0 ),/* cmd not yet ack'd to scsi lyer */ ATA_QCFLAG_SG = (1 <<1 ),/* have s/g table? */ -ATA_QCFLAG_SINGLE = (1 <<2 ),/* no s/g, just a single buffer */ -ATA_QCFLAG_DMAMAP =ATA_QCFLAG_SG |ATA_QCFLAG_SINGLE , +ATA_QCFLAG_DMAMAP =ATA_QCFLAG_SG ,ATA_QCFLAG_IO = (1 <<3 ),/* standard IO command */ ATA_QCFLAG_RESULT_TF = (1 <<4 ),/* result TF requested */ ATA_QCFLAG_CLEAR_EXCL = (1 << 5), /* clear excl_link on completion */
@@ -459,7 +458,6 @@ struct ata_queued_cmd {struct scatterlist sgent;struct scatterlist pad_sgent; -void *buf_virt;/* DO NOT iterate over __sg manually, use ata_for_each_sg() */ struct scatterlist *__sg;
PATCH 7/7 sg_ring: convert core ATA code to sg_ring.
ATA relies so heavily on scsi that it needs to be converted at the same time.
ATA adds padding to scatterlists in scsi commands, but because there was no good way of appending to those scatterlists, it had to use boutique iterators to make sure the padding is included. With sg_ring, ATA can simply append an sg_ring entry with the padding, and normal iterators can be used.
I renamed qc->cursg to qc->cur_sg to catch all the users: they should now be referring to 'qc->cur_sg[qc->cursg_i]' wherever they were using 'qc->cursg'.
drivers/ata/ahci.c +5 -5
@@ -1479,9 +1479,9 @@ static void ahci_tf_read(struct ata_portstatic unsigned int ahci_fill_sg (struct ata_queued_cmd *qc,void *cmd_tbl) { -struct scatterlist *sg;struct ahci_sg *ahci_sg; -unsigned int n_sg =0 ; +struct sg_ring *sg; +unsigned int i, n_sg =0 ;VPRINTK ("ENTER\n" );
@@ -1489,9 +1489,9 @@ static unsigned int ahci_fill_sg(struct* Next, the S/G list. */ ahci_sg = cmd_tbl +AHCI_CMD_TBL_HDR_SZ ; -ata_for_each_sg (sg, qc) { -dma_addr_t addr =sg_dma_address (sg); -u32 sg_len =sg_dma_len (sg); +sg_ring_for_each (qc->sg, sg, i) { +dma_addr_t addr =sg_dma_address (&sg->sg[i]); +u32 sg_len =sg_dma_len (&sg->sg[i]); ahci_sg->addr =cpu_to_le32 (addr &0xffffffff ); ahci_sg->addr_hi =cpu_to_le32 ((addr >>16 ) >>16 );
drivers/ata/libata-core.c +130 -129
420 lines not on this page · raw message
drivers/ata/libata-scsi.c +27 -9
@@ -517,8 +517,7 @@ static struct ata_queued_cmd *ata_scsi_q qc->scsicmd = cmd; qc->scsidone = done; - qc->__sg =scsi_sglist (cmd); - qc->n_elem =scsi_sg_count (cmd); + qc->sg =scsi_sgring (cmd); }else { cmd->result = (DID_OK <<16 ) | (QUEUE_FULL <<1 );done (cmd);
@@ -1479,6 +1478,17 @@ static void ata_scsi_qc_complete(structata_qc_free (qc); } +static unsigned int sg_ring_len (struct sg_ring *sg_start) +{ +struct sg_ring *sg; +unsigned int i, len =0 ; + +sg_ring_for_each (sg_start, sg, i) + len += sg->sg[i].length; + +return len; +} +/** * ata_scsi_translate - Translate then issue SCSI command to ATA device * @dev: ATA device to which the command is addressed
@@ -1529,7 +1539,16 @@ static int ata_scsi_translate(struct ata goto err_did; } -ata_sg_init (qc,scsi_sglist (cmd),scsi_sg_count (cmd)); +ata_sg_init (qc,scsi_sgring (cmd)); +if (scsi_bufflen (cmd) !=sg_ring_len (scsi_sgring (cmd))) { +printk ("scsi_bufflen = %u, sg_ring num %u/%u len = %u, qc->nbytes = %u\n" , +scsi_bufflen (cmd), +scsi_sgring (cmd)->num,scsi_sgring (cmd)->max, +sg_ring_len (scsi_sgring (cmd)), + qc->nbytes); + } +BUG_ON (scsi_bufflen (cmd) !=sg_ring_len (scsi_sgring (cmd))); +BUG_ON (scsi_bufflen (cmd) != qc->nbytes); qc->dma_dir = cmd->sc_data_direction; }
@@ -1592,9 +1611,9 @@ static unsigned int ata_scsi_rbuf_get(stu8 *buf;unsigned int buflen; -struct scatterlist *sg =scsi_sglist (cmd); - -if (sg) { +if (scsi_sgring (cmd)) { +struct scatterlist *sg = &scsi_sgring (cmd)->sg[0 ]; +BUG_ON (scsi_sgring (cmd)->num !=1 ); buf =kmap_atomic (sg_page (sg),KM_IRQ0 ) + sg->offset; buflen = sg->length; }else {
@@ -1619,9 +1638,8 @@ static unsigned int ata_scsi_rbuf_get(ststatic inline void ata_scsi_rbuf_put (struct scsi_cmnd *cmd,u8 *buf) { -struct scatterlist *sg =scsi_sglist (cmd); -if (sg) -kunmap_atomic (buf - sg->offset,KM_IRQ0 ); +if (scsi_sgring (cmd)) +kunmap_atomic (buf -scsi_sgring (cmd)->sg[0 ].offset,KM_IRQ0 ); }/**
drivers/ata/libata.h +2 -2
@@ -73,8 +73,8 @@ extern unsigned ata_exec_internal(struct unsigned long timeout);extern unsigned ata_exec_internal_sg (struct ata_device *dev,struct ata_taskfile *tf,const u8 *cdb, -int dma_dir,struct scatterlist *sg, -unsigned int n_elem,unsigned long timeout); +int dma_dir,struct sg_ring *sg, +unsigned long timeout);extern unsigned int ata_do_simple_cmd (struct ata_device *dev,u8 cmd);extern int ata_dev_read_id (struct ata_device *dev,unsigned int *p_class,unsigned int flags,u16 *id);
drivers/ata/pdc_adma.c +5 -5
@@ -315,22 +315,22 @@ static void adma_error_handler(struct atstatic int adma_fill_sg (struct ata_queued_cmd *qc) { -struct scatterlist *sg; +struct sg_ring *sg;struct ata_port *ap = qc->ap;struct adma_port_priv *pp = ap->private_data;u8 *buf = pp->pkt, *last_buf =NULL ; -int i = (2 + buf[3 ]) *8 ; +int idx, i = (2 + buf[3 ]) *8 ;u8 pFLAGS = pORD | ((qc->tf.flags &ATA_TFLAG_WRITE ) ? pDIRO :0 ); -ata_for_each_sg (sg, qc) { +sg_ring_for_each (qc->sg, sg, idx) {u32 addr;u32 len; - addr = (u32 )sg_dma_address (sg); + addr = (u32 )sg_dma_address (&sg->sg[idx]); *(__le32 *)(buf + i) =cpu_to_le32 (addr); i +=4 ; - len =sg_dma_len (sg) >>3 ; + len =sg_dma_len (&sg->sg[idx]) >>3 ; *(__le32 *)(buf + i) =cpu_to_le32 (len); i +=4 ;
drivers/ata/sata_mv.c +5 -4
@@ -1134,13 +1134,14 @@ static void mv_fill_sg(struct ata_queuedstatic void mv_fill_sg (struct ata_queued_cmd *qc) {struct mv_port_priv *pp = qc->ap->private_data; -struct scatterlist *sg; +struct sg_ring *sg; +unsigned int i;struct mv_sg *mv_sg, *last_sg =NULL ; mv_sg = pp->sg_tbl; -ata_for_each_sg (sg, qc) { -dma_addr_t addr =sg_dma_address (sg); -u32 sg_len =sg_dma_len (sg); +sg_ring_for_each (qc->sg, sg, i) { +dma_addr_t addr =sg_dma_address (&sg->sg[i]); +u32 sg_len =sg_dma_len (&sg->sg[i]);while (sg_len) {u32 offset = addr &0xffff ;
drivers/ata/sata_nv.c +15 -16
@@ -1315,20 +1315,20 @@ static int nv_adma_host_init(struct ata_ }static void nv_adma_fill_aprd (struct ata_queued_cmd *qc, -struct scatterlist *sg, -int idx, +struct sg_ring *sg, +int i,int idx,struct nv_adma_prd *aprd) {u8 flags =0 ;if (qc->tf.flags &ATA_TFLAG_WRITE ) flags |=NV_APRD_WRITE ; -if (idx == qc->n_elem -1 ) +if (!sg_ring_next (sg, qc->sg) && i == sg->num -1 ) flags |=NV_APRD_END ;else if (idx !=4 ) flags |=NV_APRD_CONT ; - aprd->addr =cpu_to_le64 (((u64 )sg_dma_address (sg))); - aprd->len =cpu_to_le32 (((u32 )sg_dma_len (sg)));/* len in bytes */ + aprd->addr =cpu_to_le64 (((u64 )sg_dma_address (&sg->sg[i]))); + aprd->len =cpu_to_le32 (((u32 )sg_dma_len (&sg->sg[i])));/* len in bytes */ aprd->flags = flags; aprd->packet_len =0 ; }
@@ -1336,18 +1336,18 @@ static void nv_adma_fill_sg(struct ata_qstatic void nv_adma_fill_sg (struct ata_queued_cmd *qc,struct nv_adma_cpb *cpb) {struct nv_adma_port_priv *pp = qc->ap->private_data; -unsigned int idx; +unsigned int i, idx;struct nv_adma_prd *aprd; -struct scatterlist *sg; +struct sg_ring *sg;VPRINTK ("ENTER\n" ); idx =0 ; -ata_for_each_sg (sg, qc) { +sg_ring_for_each (qc->sg, sg, i) { aprd = (idx <5 ) ? &cpb->aprd[idx] : &pp->aprd[NV_ADMA_SGTBL_LEN * qc->tag + (idx-5 )]; -nv_adma_fill_aprd (qc, sg, idx, aprd); +nv_adma_fill_aprd (qc, sg, i, idx, aprd); idx++; }if (idx >5 )
@@ -1994,23 +1994,22 @@ static void nv_swncq_fill_sg(struct ata_static void nv_swncq_fill_sg (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap; -struct scatterlist *sg; -unsigned int idx; +struct sg_ring *sg; +unsigned int i, idx;struct nv_swncq_port_priv *pp = ap->private_data;struct ata_prd *prd; -WARN_ON (qc->__sg ==NULL ); -WARN_ON (qc->n_elem ==0 && qc->pad_len ==0 ); +WARN_ON (qc->sg ==NULL ); prd = pp->prd +ATA_MAX_PRD * qc->tag; idx =0 ; -ata_for_each_sg (sg, qc) { +sg_ring_for_each (qc->sg, sg, i) {u32 addr, offset;u32 sg_len, len; - addr = (u32 )sg_dma_address (sg); - sg_len =sg_dma_len (sg); + addr = (u32 )sg_dma_address (&sg->sg[i]); + sg_len =sg_dma_len (&sg->sg[i]);while (sg_len) { offset = addr &0xffff ;
drivers/ata/sata_promise.c +6 -7
@@ -540,18 +540,17 @@ static void pdc_fill_sg(struct ata_queuestatic void pdc_fill_sg (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap; -struct scatterlist *sg; -unsigned int idx; +struct sg_ring *sg; +unsigned int i, idx;const u32 SG_COUNT_ASIC_BUG =41 *4 ;if (!(qc->flags &ATA_QCFLAG_DMAMAP ))return ; -WARN_ON (qc->__sg ==NULL ); -WARN_ON (qc->n_elem ==0 && qc->pad_len ==0 ); +WARN_ON (qc->sg ==NULL ); idx =0 ; -ata_for_each_sg (sg, qc) { +sg_ring_for_each (qc->sg, sg, i) {u32 addr, offset;u32 sg_len, len;
@@ -559,8 +558,8 @@ static void pdc_fill_sg(struct ata_queue* Note h/w doesn't support 64-bit, so we unconditionally * truncate dma_addr_t to u32. */ - addr = (u32 )sg_dma_address (sg); - sg_len =sg_dma_len (sg); + addr = (u32 )sg_dma_address (&sg->sg[i]); + sg_len =sg_dma_len (&sg->sg[i]);while (sg_len) { offset = addr &0xffff ;
drivers/ata/sata_qstor.c +6 -7
@@ -284,25 +284,24 @@ static int qs_scr_write(struct ata_portstatic unsigned int qs_fill_sg (struct ata_queued_cmd *qc) { -struct scatterlist *sg; +struct sg_ring *sg;struct ata_port *ap = qc->ap;struct qs_port_priv *pp = ap->private_data; -unsigned int nelem; +unsigned int i, nelem;u8 *prd = pp->pkt +QS_CPB_BYTES ; -WARN_ON (qc->__sg ==NULL ); -WARN_ON (qc->n_elem ==0 && qc->pad_len ==0 ); +WARN_ON (qc->sg ==NULL ); nelem =0 ; -ata_for_each_sg (sg, qc) { +sg_ring_for_each (qc->sg, sg, i) {u64 addr;u32 len; - addr =sg_dma_address (sg); + addr =sg_dma_address (&sg->sg[i]); *(__le64 *)prd =cpu_to_le64 (addr); prd +=sizeof (u64); - len =sg_dma_len (sg); + len =sg_dma_len (&sg->sg[i]); *(__le32 *)prd =cpu_to_le32 (len); prd +=sizeof (u64);
drivers/ata/sata_sil24.c +5 -4
@@ -811,12 +811,13 @@ static inline void sil24_fill_sg(structstatic inline void sil24_fill_sg (struct ata_queued_cmd *qc,struct sil24_sge *sge) { -struct scatterlist *sg; +struct sg_ring *sg; +unsigned int i;struct sil24_sge *last_sge =NULL ; -ata_for_each_sg (sg, qc) { - sge->addr =cpu_to_le64 (sg_dma_address (sg)); - sge->cnt =cpu_to_le32 (sg_dma_len (sg)); +sg_ring_for_each (qc->sg, sg, i) { + sge->addr =cpu_to_le64 (sg_dma_address (&sg->sg[i])); + sge->cnt =cpu_to_le32 (sg_dma_len (&sg->sg[i])); sge->flags =0 ; last_sge = sge;
drivers/ata/sata_sx4.c +5 -5
@@ -467,7 +467,7 @@ static inline void pdc20621_host_pkt(strstatic void pdc20621_dma_prep (struct ata_queued_cmd *qc) { -struct scatterlist *sg; +struct sg_ring *sg;struct ata_port *ap = qc->ap;struct pdc_port_priv *pp = ap->private_data;void __iomem *mmio = ap->host->iomap[PDC_MMIO_BAR ];
@@ -487,10 +487,10 @@ static void pdc20621_dma_prep(struct ata* Build S/G table */ idx =0 ; -ata_for_each_sg (sg, qc) { - buf[idx++] =cpu_to_le32 (sg_dma_address (sg)); - buf[idx++] =cpu_to_le32 (sg_dma_len (sg)); - total_len +=sg_dma_len (sg); +sg_ring_for_each (qc->sg, sg, i) { + buf[idx++] =cpu_to_le32 (sg_dma_address (&sg->sg[i])); + buf[idx++] =cpu_to_le32 (sg_dma_len (&sg->sg[i])); + total_len +=sg_dma_len (&sg->sg[i]); } buf[idx -1 ] |=cpu_to_le32 (ATA_PRD_EOT ); sgt_len = idx *4 ;
include/linux/libata.h +11 -43
97 lines not on this page · raw message
Re: [PATCH 7/7] sg_ring: convert core ATA code to sg_ring.
Hello, Rusty Russell.
Quoted 9 lines · Rusty Russell
ATA relies so heavily on scsi that it needs to be converted at the same time.
ATA adds padding to scatterlists in scsi commands, but because there was no good way of appending to those scatterlists, it had to use boutique iterators to make sure the padding is included. With
sg_ring, ATA can simply append ansg_ringentry with the padding, and normal iterators can be used.
Quoted 5 lines
I renamed
qc->cursgtoqc->cur_sgto catch all the users: they should now be referring to 'qc->cur_sg[qc->cursg_i]' wherever they were using 'qc->cursg'.Signed-off-by Rusty Russell <[email protected]>
There's pending patchset to make libata use chained sg. Please search for "[PATCHSET] libata: improve ATAPI data transfer handling, take #3" on your favorite mailing archive (I'm writing this message offline so can't give you the url at the moment). The 11th patch in the patchset converts libata to use chained sg and and the following ones extend it to also chain drain sg entries.
I agree that the current chained sg isn't easy to use and it would be nice to have some abstraction on top of it but sg_ring seems to implement duplicate feature which is already available through sg chaining albeit cumbersome to use.
It would be better to build upon sg chaining as we already have it. I think it can be made much easier with a bit more safe guards, generalization and some helpers.
Thanks.
(PS, I haven't followed the sg chaining discussion. Why is sg chaining an optional feature? Performance overhead on low end machines?)
Signature · 1 line
tejun
James Bottomley
maintainerRe: [PATCH 7/7] sg_ring: convert core ATA code to sg_ring.
Quoted 2 lines · Tejun Heo
(PS, I haven't followed the sg chaining discussion. Why is sg chaining an optional feature? Performance overhead on low end machines?)
The idea of SG chaining is to allow drivers that wish to take advantage of it to increase their transfer lengths beyond MAX_HW_SEGMENTS*PAGE_SIZE by using chaining. However, drivers that stay below MAX_HW_SEGMENTS for the scatterlist length don't need to be altered.
The ultimate goal (well, perhaps more wish) is to have all drivers converted, so SCSI can use something small for the default scatterlist sizing and dump all the sglist mempool stuff (although this may never be reached).
James
Re: [PATCH 7/7] sg_ring: convert core ATA code to sg_ring.
In reply to Tejun HeoQuoted 3 lines · On Wednesday 26 December 2007 19:36:36 Tejun Heo wrote:
It would be better to build upon sg chaining as we already have it. I think it can be made much easier with a bit more safe guards, generalization and some helpers.
Hi Tejun,
I did this work to replace sg chaining. The current chaining code gives
us longer sgs, and (if used carefully) the ability to append in some circumstances. But it's clumsy, and we can do better. This is my attempt.
I understand that people are experiencing whiplash from the idea of
another change just after the sg chaining changes which have already gone in.
At the very least, I think I'll have to come up with a better transition
plan for SCSI drivers, rather than trying to convert them all at once... In theory, sg chaining can be done through the sg ring arrays, but that's pretty horrible.
Cheers,
Rusty.
Re: [PATCH 7/7] sg_ring: convert core ATA code to sg_ring.
Hello, Rusty.
Quoted 8 lines · Rusty Russell
Quoted 3 lines · On Wednesday 26 December 2007 19:36:36 Tejun Heo wrote:
It would be better to build upon sg chaining as we already have it. I think it can be made much easier with a bit more safe guards, generalization and some helpers.
I did this work to replace sg chaining. The current chaining code gives
us longer sgs, and (if used carefully) the ability to append in some circumstances. But it's clumsy, and we can do better. This is my attempt.
Ah.. okay. That makes sense then. I fully agree that sg chaining as it currently stands is pretty ugly to use. Appending extra entries requires reserving one extra space in the sg list to be appended to accommodate the last entry of the original list, which will be used for chaining now. Also, it's quite brittle in that information can get out of sync easily (there's no one API for sg list manipulation, things have to be done manually).
Quoted 7 lines
I understand that people are experiencing whiplash from the idea of
another change just after the sg chaining changes which have already gone in.At the very least, I think I'll have to come up with a better transition
plan for SCSI drivers, rather than trying to convert them all at once... In theory, sg chaining can be done through the sg ring arrays, but that's pretty horrible.
I don't necessarily think large one time conversion is bad. They're necessary at times and make sense when they can increase long term maintainability of the code. One thing I'm concerned about is the mixture of sg chaining and sg ring. It would be best if we can agree on conversion plan (or sg chaining extension plan).
Thanks.
Signature · 1 line
tejun
James Bottomley
maintainerQuoted 6 lines · Rusty Russell
This patch series is the start of my attempt to simplify and make explicit the chained scatterlist logic.
It's not complete: my SATA box boots and seems happy, but all the other users of SCSI need to be updated and checked. But I've gotten far enough to believe it's worth persuing.
Sorry for the delay in looking at this, I was busy with Holidays and things.
When I compare sg_ring with the current sg_chain (and later sg_table) implementations, I'm actually struck by how similar they are.
The other thing I note is that the problem you're claiming to solve with sg_ring (the ability to add extra scatterlists to the front or the back of an existing one) is already solved with sg_chain, so the only real advantage of sg_ring was that it contains explicit counts, which sg_table (in -mm) also introduces.
The other differences are that sg_ring only allows adding at the front or back of an existing sg_ring, it doesn't allow splicing at any point like sg_chain does, so I'd say it's less functional (not that I actually want anyone ever to do this, of course ...)
The final point is that sg_ring requires a two level traversal: ring list then scatterlist, whereas sg_chain only requires a single level traversal. I grant that we can abstract out the traversal into something that would make users think they're only doing a single level, but I don't see what the extra level really buys us.
The above analysis seems to suggest that sg_chain is simpler and has more functionality than sg_ring, unless I've missed anything?
The only thing missing from sg_chain perhaps is an accessor function that does the splicing, which I can easily construct if you want to try it out in virtio.
James
Quoted 10 lines · On Sunday 06 January 2008 02:31:12 James Bottomley wrote:
Quoted 6 lines · Rusty Russell
This patch series is the start of my attempt to simplify and make explicit the chained scatterlist logic.
It's not complete: my SATA box boots and seems happy, but all the other users of SCSI need to be updated and checked. But I've gotten far enough to believe it's worth persuing.
Sorry for the delay in looking at this, I was busy with Holidays and things.
Thankyou for your consideration.
Quoted 2 lines
When I compare
sg_ringwith the currentsg_chain(and latersg_table) implementations, I'm actually struck by how similar they are.
I agree, they're solving the same problem. It is possible that the pain of change is no longer worthwhile, but I hate to see us give up on this. We're adding complexity without making it harder to misuse.
Quoted 5 lines
The other thing I note is that the problem you're claiming to solve with
sg_ring(the ability to add extra scatterlists to the front or the back of an existing one) is already solved withsg_chain, so the only real advantage ofsg_ringwas that it contains explicit counts, whichsg_table(in -mm) also introduces.
I just converted virtio using latest Linus for fair comparison, and it's still pretty ugly. sg_ring needs more work to de-scsi it. sg_table is almost sg_ring except it retains all the chaining warts.
But we hit the same problems:
1) sg_chain loses information. The clever chain packaging makes reading easy, but manipulation is severely limited. You can append to your own chains by padding, but not someone elses. This works for SCSI, but what about the rest of us? And don't even think of joining mapped chains: it will almost work.
2) sg_chain's end marker is only for reading non-dma elements, not for mapped chains, nor for writing into chains. Plus it's a duplicate of the num arg, which is still passed around for efficiency. (virtio had to implement count_sg() because I didn't want those two redundant num args).
3) By overloading struct scatterlist, it's invisible what code has been converted to chains, and which hasn't. Too clever by half!
Look at sg_chain(): it claims to join two scatterlists, but it doesn't. It assumes that prv is an array, not a chain. Because you've overloaded an existing type, this happens everywhere. Try passing skb_to_sgvec a chained skb.
sg_ring would not have required any change to existing drivers, just those that want to use long sg lists. And then it's damn obvious which are which.
4) sg_chain missed a chance to combine all the relevent information (max, num, num_dma and the sg array). sg_table comes close, but you still can't join them (no max information, and even if there were, what if it's full?). Unlike sg_ring, it's unlikely to reduce bugs.
5) (A little moot now) sg_ring didn't require arch changes.
Quoted 4 lines
The other differences are that
sg_ringonly allows adding at the front or back of an existingsg_ring, it doesn't allow splicing at any point likesg_chaindoes, so I'd say it's less functional (not that I actually want anyone ever to do this, of course ...)
Well it's just as possible, but you might have to copy more elements (with sg chaining you need only copy 1 sg element to make room for the chain elem). Agreed that it's a little out there...
Quoted 5 lines
The final point is that
sg_ringrequires a two level traversal: ring list then scatterlist, whereassg_chainonly requires a single level traversal. I grant that we can abstract out the traversal into something that would make users think they're only doing a single level, but I don't see what the extra level really buys us.
We hide the real complexity from users and it makes it less safe for non-trivial cases.
Hence the introduction of YA datastructure: sg_table. This is getting close:
just hang the array off it and you'll have sg_ring and no requirement for dynamic alloc all the time. And once you have a header, no need for chaining tricks...
Quoted 3 lines
The only thing missing from
sg_chainperhaps is an accessor function that does the splicing, which I can easily construct if you want to try it out in virtio.
I don't need that (I prepend and append), but it'd be nice if sg_next took a const struct scatterlist *.
Cheers,
Rusty.
Hello,
Quoted 9 lines · Rusty Russell
Quoted 5 lines
The other thing I note is that the problem you're claiming to solve with
sg_ring(the ability to add extra scatterlists to the front or the back of an existing one) is already solved withsg_chain, so the only real advantage ofsg_ringwas that it contains explicit counts, whichsg_table(in -mm) also introduces.I just converted virtio using latest Linus for fair comparison, and it's still pretty ugly.
sg_ringneeds more work to de-scsi it.sg_tableis almostsg_ringexcept it retains all the chaining warts.
I agree it's ugly.
Quoted 6 lines
But we hit the same problems:
1)
sg_chainloses information. The clever chain packaging makes reading easy, but manipulation is severely limited. You can append to your own chains by padding, but not someone elses. This works for SCSI, but what about the rest of us? And don't even think of joining mapped chains: it will almost work.
You can append by allocating one more element on the chain to be appended and moving the last element of the first chain to it while using the last element for chaining. Join can be done by similar technique using three element extra chain in the middle. But, yeah, it's ugly as hell.
Signature · 1 line
tejun
Quoted 11 lines · On Monday 07 January 2008 16:01:40 Tejun Heo wrote:
Quoted 7 lines
But we hit the same problems:
1)
sg_chainloses information. The clever chain packaging makes reading easy, but manipulation is severely limited. You can append to your own chains by padding, but not someone elses. This works for SCSI, but what about the rest of us? And don't even think of joining mapped chains: it will almost work.You can append by allocating one more element on the chain to be appended and moving the last element of the first chain to it while using the last element for chaining.
Hi Tejun,
Nice try! Even ignoring the ugliness of undoing such an operation if the
caller doesn't expect you to mangle their chains, consider a one-element sg array. :(
Cheers,
Rusty.
Quoted 12 lines · Rusty Russell
Quoted 10 lines · On Monday 07 January 2008 16:01:40 Tejun Heo wrote:
Quoted 7 lines
But we hit the same problems:
1)
sg_chainloses information. The clever chain packaging makes reading easy, but manipulation is severely limited. You can append to your own chains by padding, but not someone elses. This works for SCSI, but what about the rest of us? And don't even think of joining mapped chains: it will almost work.You can append by allocating one more element on the chain to be appended and moving the last element of the first chain to it while using the last element for chaining.
Quoted 5 lines
Hi Tejun,
Nice try! Even ignoring the ugliness of undoing such an operation if the
caller doesn't expect you to mangle their chains, consider a one-element sg array. :(
Heh heh, that can be dealt with by skipping the first chain if the first chain is empty after chaining. Please take a look at ata_sg_setup_extra() in the following.
That said, yeah, it's seriously ugly. Restoring the original sg is ugly too. I definitely agree that we need some improvements here.
Signature · 1 line
tejun
Quoted 11 lines · On Monday 07 January 2008 17:37:41 Tejun Heo wrote:
Quoted 5 lines · Rusty Russell
Hi Tejun,
Nice try! Even ignoring the ugliness of undoing such an operation if
the caller doesn't expect you to mangle their chains, consider a one-element sg array. :(Heh heh, that can be dealt with by skipping the first chain if the first chain is empty after chaining. Please take a look at
ata_sg_setup_extra()in the following.
Quoted 6 lines
http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=blob;f=driver
s/ata/libata-core.c;h=32dde5bbc75ed53e89ac17040da2cd0621a37161;hb=c8847e473a4a2844244784226eb362be10d52ce9That said, yeah, it's seriously ugly. Restoring the original sg is ugly too. I definitely agree that we need some improvements here.
Erk, that's beyond ugly, into actual evil.
To make this general you need to find the last N 1-element chains (but SCSI doesn't do this of course). Oh the horror...
I'd be remiss if I didn't mention that the sg_ring ata patches were straightforward, and indescribably beautiful if compared to this!
Thanks,
Rusty.
Quoted 11 lines · Rusty Russell
Quoted 10 lines · On Monday 07 January 2008 17:37:41 Tejun Heo wrote:
Quoted 5 lines · Rusty Russell
Hi Tejun,
Nice try! Even ignoring the ugliness of undoing such an operation if
the caller doesn't expect you to mangle their chains, consider a one-element sg array. :(Heh heh, that can be dealt with by skipping the first chain if the first chain is empty after chaining. Please take a look at
ata_sg_setup_extra()in the following.
Quoted 8 lines
Quoted 6 lines
http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=blob;f=driver
s/ata/libata-core.c;h=32dde5bbc75ed53e89ac17040da2cd0621a37161;hb=c8847e473a4a2844244784226eb362be10d52ce9That said, yeah, it's seriously ugly. Restoring the original sg is ugly too. I definitely agree that we need some improvements here.
Erk, that's beyond ugly, into actual evil.
/me agrees.
Quoted 2 lines
To make this general you need to find the last N 1-element chains (but SCSI doesn't do this of course). Oh the horror...
/me agrees.
Quoted 2 lines
I'd be remiss if I didn't mention that the
sg_ringata patches were straightforward, and indescribably beautiful if compared to this!
/me agrees. As long as this can be made sane using one unified interface, I don't care whether it's sg_ring, table or dish.
Thanks.
Signature · 1 line
tejun
Quoted 3 lines · Tejun Heo
/me agrees. As long as this can be made sane using one unified interface, I don't care whether it's
sg_ring, table or dish.
Give us sg_annulus :)
On a more serious note, having played with SG chaining for a couple of years in the crypto layer, I must say Rusty's proposal is a great improvement for me. The reason is that if the crypto layer gets an SG list from its user, it is not at liberty to just modify it. As such chaining at the end is cumbersome with the current interface because we have to duplicate the whole list in order to add just a single block at the end.
While with sg_ring we could just put the original list along with the new one in a new list.
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtJames Bottomley
maintainerQuoted 12 lines · Rusty Russell
Quoted 10 lines · On Sunday 06 January 2008 02:31:12 James Bottomley wrote:
Quoted 6 lines · Rusty Russell
This patch series is the start of my attempt to simplify and make explicit the chained scatterlist logic.
It's not complete: my SATA box boots and seems happy, but all the other users of SCSI need to be updated and checked. But I've gotten far enough to believe it's worth persuing.
Sorry for the delay in looking at this, I was busy with Holidays and things.
Quoted 8 lines
Thankyou for your consideration.
Quoted 2 lines
When I compare
sg_ringwith the currentsg_chain(and latersg_table) implementations, I'm actually struck by how similar they are.I agree, they're solving the same problem. It is possible that the pain of change is no longer worthwhile, but I hate to see us give up on this. We're adding complexity without making it harder to misuse.
We're always open to new APIs (or more powerful and expanded old ones). The way we've been doing the sg_chain conversion is to slide API layers into the drivers so sg_chain becomes a simple API flip when we turn it on. Unfortunately sg_ring doesn't quite fit nicely into this.
Quoted 7 lines
Quoted 5 lines
The other thing I note is that the problem you're claiming to solve with
sg_ring(the ability to add extra scatterlists to the front or the back of an existing one) is already solved withsg_chain, so the only real advantage ofsg_ringwas that it contains explicit counts, whichsg_table(in -mm) also introduces.I just converted virtio using latest Linus for fair comparison
Erm, but that's not really a fair comparison; you need the sg_table code in
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git
branch sg as well.
Quoted 10 lines
, and it's still
pretty ugly.sg_ringneeds more work to de-scsi it.sg_tableis almostsg_ringexcept it retains all the chaining warts.But we hit the same problems:
1)
sg_chainloses information. The clever chain packaging makes reading easy, but manipulation is severely limited. You can append to your own chains by padding, but not someone elses. This works for SCSI, but what about the rest of us? And don't even think of joining mapped chains: it will almost work.
OK, but realistically some of your criticisms are way outside of the design intent. Scatterlists (and now sg_chains) are the way the block subsystem hands pages to its underlying block devices. We do this through two APIs: blk_rq_map_sg() to take a request and place all the pages into a scatterlist respecting the merging parameters and dma_map_sg() which takes the scatterlist and produces an arch specific DMA mapped scatterlist reusing the old list. The requirement is that dma_unmap_sg return the chain to its former state (so block device may map, and unmap between congestion waits to avoid running systems out of scarce IOMMU mappings), but there's not even a parallel blk_rq_unmap_sg() beacuse the scatterlist is assumed disposable by the block driver after the request completes.
By "someone elses" I'm assuming you mean a chain in a stacked block driver? You are actually allowed to transform these ... depending on who does the dma mapping, of course, if the lower driver (yours) does DMA mapping, you can do anything you want. If the upper driver does, then you have to preserve idempotence.
There have never until now been any requirements to join already dma_map_sg() converted scatterlists ... that would wreak havoc with the way we reuse the list plus damage the idempotence of map/unmap. What is the use case you have for this?
Quoted 7 lines
2) sg_chain's end marker is only for reading non-dma elements, not for mapped chains, nor for writing into chains. Plus it's a duplicate of the num arg, which is still passed around for efficiency. (virtio had to implement
count_sg()because I didn't want those two redundant num args).3) By overloading struct scatterlist, it's invisible what code has been converted to chains, and which hasn't. Too clever by half!
No it's not ... that's the whole point. Code not yet converted to use the API accessors is by definition unable to use chaining. Code converted to use the accessors by design doesn't care (and so is "converted to chains").
Quoted 7 lines
Look at
sg_chain(): it claims to join two scatterlists, but it doesn't. It assumes that prv is an array, not a chain. Because you've overloaded an existing type, this happens everywhere. Try passingskb_to_sgveca chained skb.
sg_ringwould not have required any change to existing drivers, just those that want to use long sg lists. And then it's damn obvious which are which.
Which, by definition, would have been pretty much all of them.
Another driving factor is that the mempool allocation of scatterlists is suboptimal in terms of their bucket size, so we were looking to tune that once the chaining code was in. The guess is that we'd probalby end up with a uniform size for scatterlist chunks, but if all drivers aren't converted we can't do this type of tuning.
Quoted 4 lines
4)
sg_chainmissed a chance to combine all the relevent information (max, num,num_dmaand the sg array).sg_tablecomes close, but you still can't join them (no max information, and even if there were, what if it's full?). Unlikesg_ring, it's unlikely to reduce bugs.
sg_table is sg_chain ... they're incremental extensions of the same body of work.
Quoted 25 lines
5) (A little moot now)
sg_ringdidn't require arch changes.Quoted 4 lines
The other differences are that
sg_ringonly allows adding at the front or back of an existingsg_ring, it doesn't allow splicing at any point likesg_chaindoes, so I'd say it's less functional (not that I actually want anyone ever to do this, of course ...)Well it's just as possible, but you might have to copy more elements (with sg chaining you need only copy 1 sg element to make room for the chain elem). Agreed that it's a little out there...
Quoted 5 lines
The final point is that
sg_ringrequires a two level traversal: ring list then scatterlist, whereassg_chainonly requires a single level traversal. I grant that we can abstract out the traversal into something that would make users think they're only doing a single level, but I don't see what the extra level really buys us.We hide the real complexity from users and it makes it less safe for non-trivial cases.
Hence the introduction of YA datastructure:
sg_table. This is getting close:
just hang the array off it and you'll havesg_ringand no requirement for dynamic alloc all the time. And once you have a header, no need for chaining tricks...
Quoted 6 lines
Quoted 3 lines
The only thing missing from
sg_chainperhaps is an accessor function that does the splicing, which I can easily construct if you want to try it out in virtio.I don't need that (I prepend and append), but it'd be nice if
sg_nexttook a const struct scatterlist *.
What all this is saying to me is that we have an undesigned, ad hoc interface for scatterlist manipulation in passthrough drivers. Perhaps we should start by defining that API and its associated requirements. i.e. what is it that pass through drivers want to be able to do to scatterlists? The only such example of a driver like that I know is the crypto API and now your virtio. Once we have the actual requirements and then the API, I think the natural data structures will probably drop out.
James
Quoted 4 lines · On Tuesday 08 January 2008 02:48:23 James Bottomley wrote:
We're always open to new APIs (or more powerful and expanded old ones). The way we've been doing the
sg_chainconversion is to slide API layers into the drivers sosg_chainbecomes a simple API flip when we turn it on. Unfortunatelysg_ringdoesn't quite fit nicely into this.
Hi James,
Well, it didn't touch any drivers. The only ones which needed altering
were those which wanted to use large scatter-gather lists. You think of the subtlety of sg-chain conversion as a feature; it's a bug :)
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
The other thing I note is that the problem you're claiming to solve with
sg_ring(the ability to add extra scatterlists to the front or the back of an existing one) is already solved withsg_chain, so the only real advantage ofsg_ringwas that it contains explicit counts, whichsg_table(in -mm) also introduces.
Quoted 8 lines
Quoted 1 line
I just converted virtio using latest Linus for fair comparison
Erm, but that's not really a fair comparison; you need the
sg_tablecode in
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.gitbranch sg as well.
Actually, I think it's a wash. Now callers need to set up an sg_table as well. It will save the count_sg() calls though.
Quoted 12 lines
Quoted 11 lines
, and it's still
pretty ugly.sg_ringneeds more work to de-scsi it.sg_tableis almostsg_ringexcept it retains all the chaining warts.But we hit the same problems:
1)
sg_chainloses information. The clever chain packaging makes reading easy, but manipulation is severely limited. You can append to your own chains by padding, but not someone elses. This works for SCSI, but what about the rest of us? And don't even think of joining mapped chains: it will almost work.
Quoted 3 lines
OK, but realistically some of your criticisms are way outside of the design intent. Scatterlists (and now
sg_chains) are the way the block subsystem hands pages to its underlying block devices.
James, scatterlists are used for more than the block subsystem. I know you're designing for that, but we can do better.
Because a single sg_ring is trivially convertable to and from a
scatterlist *, the compatibility story is nice. You can implement a subsystem (say, the block layer) with sg_ring, and still hand out struct scatterlist arrays for backwards compat: old code won't ask for v. long scatterlist arrays anyway.
Now we have sg_chaining, we can actually convert complex sg_rings into sg
chains when someone asks, as my most recent patch does. I think that's one abstraction too many, but it provides a transition path.
Quoted 4 lines
There have never until now been any requirements to join already
dma_map_sg()converted scatterlists ... that would wreak havoc with the way we reuse the list plus damage the idempotence of map/unmap. What is the use case you have for this?
(This was, admittedly, a made-up example).
Quoted 5 lines
Quoted 5 lines
2) sg_chain's end marker is only for reading non-dma elements, not for mapped chains, nor for writing into chains. Plus it's a duplicate of the num arg, which is still passed around for efficiency. (virtio had to implement
count_sg()because I didn't want those two redundant num args).
Quoted 7 lines
Quoted 2 lines
3) By overloading struct scatterlist, it's invisible what code has been converted to chains, and which hasn't. Too clever by half!
No it's not ... that's the whole point. Code not yet converted to use the API accessors is by definition unable to use chaining. Code converted to use the accessors by design doesn't care (and so is "converted to chains").
But you've overloaded the type: what's the difference between:
/* Before conversion */ int foo(struct scatterlist *, int); And: /* After conversion */ int foo(struct scatterlist *, int);
You have to wade through the implementation to see the difference: does this function take a "chained scatterlist" or an "array scatterlist"?
Then you add insult to injury by implementing sg_chain() *which doesn't handle chained scatterlists!*.
Quoted 5 lines
Quoted 3 lines
sg_ringwould not have required any change to existing drivers, just those that want to use long sg lists. And then it's damn obvious which are which.Which, by definition, would have been pretty much all of them.
But it would have started with none of them, and it would have been done over time. Eventually we might have had a flag day to remove raw scatterlist arrays.
Quoted 7 lines
Quoted 4 lines
4)
sg_chainmissed a chance to combine all the relevent information (max, num,num_dmaand the sg array).sg_tablecomes close, but you still can't join them (no max information, and even if there were, what if it's full?). Unlikesg_ring, it's unlikely to reduce bugs.
sg_tableissg_chain... they're incremental extensions of the same body of work.
Yes.
Quoted 4 lines
The only such example of a driver like that I know is the crypto API and now your virtio. Once we have the actual requirements and then the API, I think the natural data structures will probably drop out.
ATA wants to chain sgs (Tejun cc'd). It hasn't been practical to chain up until now, but I'd say it's going to be more widely used now it is.
Basically, sg_chain solves one problem: larger sg lists for scsi. That's fine, but I want to see better use of scatterlists everywhere in the kernel.
sg_chains suck for manipulation, and AFAICT that's inherent. Here, take a look at the sg_ring conversion of scsi_alloc_sgtable and scsi_free_sgtable and you can see why I'm unhappy with the sg_chain code:
@@ -737,21 +745,41 @@ static inline unsigned int scsi_sgtable_
return index;
}
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask) +static void free_sgring(struct sg_ring *head)
{
struct scsi_host_sg_pool *sgp;
- struct scatterlist *sgl, *prev, *ret;
+ struct sg_ring *sg, *n;
+
+ /* Free any other entries in the ring. */
+ list_for_each_entry_safe(sg, n, &head->list, list) {
+ list_del(&sg->list);
+ sgp = scsi_sg_pools + scsi_sgtable_index(sg->max);
+ mempool_free(sg, sgp->pool);
+ }
+
+ /* Now free the head of the ring. */
+ BUG_ON(!list_empty(&head->list));
+
+ sgp = scsi_sg_pools + scsi_sgtable_index(head->max);
+ mempool_free(head, sgp->pool);
+}
+
+struct sg_ring *scsi_alloc_sgring(struct scsi_cmnd *cmd, unsigned int num,
+ gfp_t gfp_mask)
+{
+ struct scsi_host_sg_pool *sgp;
+ struct sg_ring *sg, *ret;
unsigned int index;
int this, left;
- BUG_ON(!cmd->use_sg);
+ BUG_ON(!num);
- left = cmd->use_sg;
- ret = prev = NULL;
+ left = num;
+ ret = NULL;
do {
this = left;
if (this > SCSI_MAX_SG_SEGMENTS) {
- this = SCSI_MAX_SG_SEGMENTS - 1;
+ this = SCSI_MAX_SG_SEGMENTS;
index = SG_MEMPOOL_NR - 1;
} else
index = scsi_sgtable_index(this);
@@ -760,32 +788,20 @@ struct scatterlist *scsi_alloc_sgtable(s
sgp = scsi_sg_pools + index;
- sgl = mempool_alloc(sgp->pool, gfp_mask);
- if (unlikely(!sgl))
+ sg = mempool_alloc(sgp->pool, gfp_mask);
+ if (unlikely(!sg))
goto enomem;
- sg_init_table(sgl, sgp->size);
+ sg_ring_init(sg, sgp->size - SCSI_SG_PAD);
/*
- * first loop through, set initial index and return value
+ * first loop through, set return value, otherwise
+ * attach this to the tail.
*/
if (!ret)
- ret = sgl;
-
- /*
- * chain previous sglist, if any. we know the previous
- * sglist must be the biggest one, or we would not have
- * ended up doing another loop.
- */
- if (prev)
- sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);
-
- /*
- * if we have nothing left, mark the last segment as
- * end-of-list
- */
- if (!left)
- sg_mark_end(&sgl[this - 1]);
+ ret = sg;
+ else
+ list_add_tail(&sg->list, &ret->list);
/*
* don't allow subsequent mempool allocs to sleep, it would
@@ -793,85 +809,21 @@ struct scatterlist *scsi_alloc_sgtable(s
*/
gfp_mask &= ~__GFP_WAIT;
gfp_mask |= __GFP_HIGH;
- prev = sgl;
} while (left);
- /*
- * ->use_sg may get modified after dma mapping has potentially
- * shrunk the number of segments, so keep a copy of it for free.
- */
- cmd->__use_sg = cmd->use_sg;
return ret;
enomem:
- if (ret) {
- /*
- * Free entries chained off ret. Since we were trying to
- * allocate another sglist, we know that all entries are of
- * the max size.
- */
- sgp = scsi_sg_pools + SG_MEMPOOL_NR - 1;
- prev = ret;
- ret = &ret[SCSI_MAX_SG_SEGMENTS - 1];
-
- while ((sgl = sg_chain_ptr(ret)) != NULL) {
- ret = &sgl[SCSI_MAX_SG_SEGMENTS - 1];
- mempool_free(sgl, sgp->pool);
- }
-
- mempool_free(prev, sgp->pool);
- }
+ if (ret)
+ free_sgring(ret);
return NULL;
}
+EXPORT_SYMBOL(scsi_alloc_sgring);
-EXPORT_SYMBOL(scsi_alloc_sgtable);
-
-void scsi_free_sgtable(struct scsi_cmnd *cmd)
+void scsi_free_sgring(struct scsi_cmnd *cmd)
{
- struct scatterlist *sgl = cmd->request_buffer;
- struct scsi_host_sg_pool *sgp;
-
- /*
- * if this is the biggest size sglist, check if we have
- * chained parts we need to free
- */
- if (cmd->__use_sg > SCSI_MAX_SG_SEGMENTS) {
- unsigned short this, left;
- struct scatterlist *next;
- unsigned int index;
-
- left = cmd->__use_sg - (SCSI_MAX_SG_SEGMENTS - 1);
- next = sg_chain_ptr(&sgl[SCSI_MAX_SG_SEGMENTS - 1]);
- while (left && next) {
- sgl = next;
- this = left;
- if (this > SCSI_MAX_SG_SEGMENTS) {
- this = SCSI_MAX_SG_SEGMENTS - 1;
- index = SG_MEMPOOL_NR - 1;
- } else
- index = scsi_sgtable_index(this);
-
- left -= this;
-
- sgp = scsi_sg_pools + index;
-
- if (left)
- next = sg_chain_ptr(&sgl[sgp->size - 1]);
-
- mempool_free(sgl, sgp->pool);
- }
-
- /*
- * Restore original, will be freed below
- */
- sgl = cmd->request_buffer;
- sgp = scsi_sg_pools + SG_MEMPOOL_NR - 1;
- } else
- sgp = scsi_sg_pools + scsi_sgtable_index(cmd->__use_sg);
-
- mempool_free(sgl, sgp->pool);
+ free_sgring(cmd->sg);
}
-
-EXPORT_SYMBOL(scsi_free_sgtable);
+EXPORT_SYMBOL(scsi_free_sgring);
/*
* Function: scsi_release_buffers()
Hope that clarifies,
Rusty.
James Bottomley
maintainerQuoted 11 lines · Rusty Russell
Quoted 4 lines · On Tuesday 08 January 2008 02:48:23 James Bottomley wrote:
We're always open to new APIs (or more powerful and expanded old ones). The way we've been doing the
sg_chainconversion is to slide API layers into the drivers sosg_chainbecomes a simple API flip when we turn it on. Unfortunatelysg_ringdoesn't quite fit nicely into this.Hi James,
Well, it didn't touch any drivers. The only ones which needed altering
were those which wanted to use large scatter-gather lists. You think of the subtlety of sg-chain conversion as a feature; it's a bug :)
No, I think of the side effect of hiding scatterlist manipulations inside accessors as a feature because it insulates the drivers from the details of the implementation.
Quoted 100 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
The other thing I note is that the problem you're claiming to solve with
sg_ring(the ability to add extra scatterlists to the front or the back of an existing one) is already solved withsg_chain, so the only real advantage ofsg_ringwas that it contains explicit counts, whichsg_table(in -mm) also introduces.Quoted 8 lines
Quoted 1 line
I just converted virtio using latest Linus for fair comparison
Erm, but that's not really a fair comparison; you need the
sg_tablecode in
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.gitbranch sg as well.
Actually, I think it's a wash. Now callers need to set up an
sg_tableas well. It will save thecount_sg()calls though.Quoted 12 lines
Quoted 11 lines
, and it's still
pretty ugly.sg_ringneeds more work to de-scsi it.sg_tableis almostsg_ringexcept it retains all the chaining warts.But we hit the same problems:
1)
sg_chainloses information. The clever chain packaging makes reading easy, but manipulation is severely limited. You can append to your own chains by padding, but not someone elses. This works for SCSI, but what about the rest of us? And don't even think of joining mapped chains: it will almost work.Quoted 3 lines
OK, but realistically some of your criticisms are way outside of the design intent. Scatterlists (and now
sg_chains) are the way the block subsystem hands pages to its underlying block devices.James, scatterlists are used for more than the block subsystem. I know you're designing for that, but we can do better.
Because a single
sg_ringis trivially convertable to and from a
scatterlist *, the compatibility story is nice. You can implement a subsystem (say, the block layer) withsg_ring, and still hand out struct scatterlist arrays for backwards compat: old code won't ask for v. long scatterlist arrays anyway.Now we have
sg_chaining, we can actually convert complexsg_ringsinto sg
chains when someone asks, as my most recent patch does. I think that's one abstraction too many, but it provides a transition path.Quoted 4 lines
There have never until now been any requirements to join already
dma_map_sg()converted scatterlists ... that would wreak havoc with the way we reuse the list plus damage the idempotence of map/unmap. What is the use case you have for this?(This was, admittedly, a made-up example).
Quoted 5 lines
Quoted 5 lines
2) sg_chain's end marker is only for reading non-dma elements, not for mapped chains, nor for writing into chains. Plus it's a duplicate of the num arg, which is still passed around for efficiency. (virtio had to implement
count_sg()because I didn't want those two redundant num args).Quoted 7 lines
Quoted 2 lines
3) By overloading struct scatterlist, it's invisible what code has been converted to chains, and which hasn't. Too clever by half!
No it's not ... that's the whole point. Code not yet converted to use the API accessors is by definition unable to use chaining. Code converted to use the accessors by design doesn't care (and so is "converted to chains").
But you've overloaded the type: what's the difference between:
/* Before conversion */ int foo(struct scatterlist *, int); And: /* After conversion */ int foo(struct scatterlist *, int);You have to wade through the implementation to see the difference: does this function take a "chained scatterlist" or an "array scatterlist"?
Then you add insult to injury by implementing
sg_chain()*which doesn't handle chained scatterlists!*.Quoted 5 lines
Quoted 3 lines
sg_ringwould not have required any change to existing drivers, just those that want to use long sg lists. And then it's damn obvious which are which.Which, by definition, would have been pretty much all of them.
But it would have started with none of them, and it would have been done over time. Eventually we might have had a flag day to remove raw scatterlist arrays.
Quoted 7 lines
Quoted 4 lines
4)
sg_chainmissed a chance to combine all the relevent information (max, num,num_dmaand the sg array).sg_tablecomes close, but you still can't join them (no max information, and even if there were, what if it's full?). Unlikesg_ring, it's unlikely to reduce bugs.
sg_tableissg_chain... they're incremental extensions of the same body of work.Yes.
Quoted 7 lines
Quoted 4 lines
The only such example of a driver like that I know is the crypto API and now your virtio. Once we have the actual requirements and then the API, I think the natural data structures will probably drop out.
ATA wants to chain sgs (Tejun cc'd). It hasn't been practical to chain up until now, but I'd say it's going to be more widely used now it is.
Actually, I think we might be able to solve ATA's problem without ever having it manipulate the sg lists, but that's really a different issue.
Quoted 7 lines
Basically,
sg_chainsolves one problem: larger sg lists for scsi. That's fine, but I want to see better use of scatterlists everywhere in the kernel.
sg_chainssuck for manipulation, and AFAICT that's inherent. Here, take a look at thesg_ringconversion ofscsi_alloc_sgtableandscsi_free_sgtableand you can see why I'm unhappy with thesg_chaincode:
[...]
Quoted 1 line
Hope that clarifies,
Actually, not really. If I want to continue the competition, I can just point out that your sg_ring code is bigger than those corresponding segments are after the sg_table conversion of scsi_lib.c ...
However, this is pointless. We seem to be talking past each other. Your sg_ring is an implementation, like sg_table. What I want to do is actually insulate the device drivers (even the drivers that want to manipulate sg lists) from the implementation. The idea being that if this is done correctly, we can alter the implementation from sg_table to sg_ring to sg_annulus and not have to change any of the driver code.
Since your code introduces new ring handling primitives, it's running counter to that grain. What I'm primarily interested in is what's broken about the new accessor API ... why doesn't it work to hide the internals of sg_ring? It seems to me, if I try to do the conversion, all that's missing is this problematic two level traversal of primitives that sg_ring requires.
However, a lot of the churn seems to be because you want to change the fundamental element from a scatterlist entry to a sg_ring. Worse still, we now get exposure of locations of scatterlist elements. Like this conversion for instance:
Review
Quoted 11 lines
(quoted diff) +5
-4- for (k = 0; k < scp->use_sg; ++k, sg = sg_next(sg)) { - kaddr = (unsigned char *)kmap_atomic(sg_page(sg), KM_USER0); + sg_ring_for_each(scp->sg, sg, k) { + kaddr = (unsigned char *)kmap_atomic(sg_page(&sg->sg[k]), + KM_USER0); if (NULL == kaddr) return -1; - kaddr_off = (unsigned char *)kaddr + sg->offset; - len = sg->length; + kaddr_off = (unsigned char *)kaddr + sg->sg[k].offset; + len = sg->sg[k].length;
Here, you're re-exposing the internals of how we traverse the lists. This is explicitly something the sg_next() and friends designedly get rid of. All drivers want to know is what the next scatterlist entry is, not where it comes from.
Quoted 3 lines
if ((req_len + len) > max_arr_len) {
len = max_arr_len - req_len;
fin = 1;I suspect if you re-roll sg_ring to conform to the current accessors, you'll find it pretty much fits, but probably looks a lot like sg_table. But if it doesn't; if there's something in the accessors that's missing, that's what I want to know about.
James
Quoted 9 lines · On Thursday 10 January 2008 09:10:37 James Bottomley wrote:
Quoted 8 lines · Rusty Russell
Quoted 4 lines · On Tuesday 08 January 2008 02:48:23 James Bottomley wrote:
We're always open to new APIs (or more powerful and expanded old ones). The way we've been doing the
sg_chainconversion is to slide API layers into the drivers sosg_chainbecomes a simple API flip when we turn it on. Unfortunatelysg_ringdoesn't quite fit nicely into this.Hi James,
Quoted 7 lines
Quoted 3 lines
Well, it didn't touch any drivers. The only ones which needed
altering were those which wanted to use large scatter-gather lists. You think of the subtlety of sg-chain conversion as a feature; it's a bug :)No, I think of the side effect of hiding scatterlist manipulations inside accessors as a feature because it insulates the drivers from the details of the implementation.
I completely disagree. Abstractions add complexity, and that costs. They steal information from their users, and as they build up like sediment they make debugging and understanding harder.
For example, an array is simple and well understood. An abstraction would
just buy confusion and YA thing to learn.
When a single array was no longer sufficient, I figured a linked-list of arrays was the simplest replacement. Easy to understand and accepted practice (tho rings are a bit exotic). Because the implementation is the interface, authors can see what is legal.
More concretely, you're already regarding your abstractions as too expensive, which is why sg_chain() doesn't handle chained sgs.
Result: you've got a complex implementation and a complex interface with a complex abstraction.
Quoted 5 lines
Quoted 3 lines
sg_chainssuck for manipulation, and AFAICT that's inherent. Here, take a look at thesg_ringconversion ofscsi_alloc_sgtableandscsi_free_sgtableand you can see why I'm unhappy with thesg_chaincode:[...]
Quoted 7 lines
Quoted 1 line
Hope that clarifies,
Actually, not really. If I want to continue the competition, I can just point out that your
sg_ringcode is bigger than those corresponding segments are after thesg_tableconversion ofscsi_lib.c...However, this is pointless.
No, it's exactly the point. These details *matter*. The implementation *matters*. sg_table moves this code out of scsi_lib (good!), but still demonstrates how much of a PITA they are to manipulate.
As for being able to make arbitrary changes in future without hitting drivers:
this is the Kernel ABI pipe dream writ small.
OK, I give in with -ETIMEDOUT. I'll go away now and do something productive :)
Cheers,
Rusty.
James Bottomley
maintainerQuoted 14 lines · Rusty Russell
Quoted 13 lines · On Thursday 10 January 2008 09:10:37 James Bottomley wrote:
Quoted 11 lines · Rusty Russell
Quoted 4 lines · On Tuesday 08 January 2008 02:48:23 James Bottomley wrote:
We're always open to new APIs (or more powerful and expanded old ones). The way we've been doing the
sg_chainconversion is to slide API layers into the drivers sosg_chainbecomes a simple API flip when we turn it on. Unfortunatelysg_ringdoesn't quite fit nicely into this.Hi James,
Well, it didn't touch any drivers. The only ones which needed
altering were those which wanted to use large scatter-gather lists. You think of the subtlety of sg-chain conversion as a feature; it's a bug :)
Quoted 7 lines
Quoted 3 lines
No, I think of the side effect of hiding scatterlist manipulations inside accessors as a feature because it insulates the drivers from the details of the implementation.
I completely disagree. Abstractions add complexity, and that costs. They steal information from their users, and as they build up like sediment they make debugging and understanding harder.
Don't be silly ... abstractions are the core of how we program something as complex as platform hardware. By design, abstractions hide information. Good abstractions hide information you don't need to know. Take the DMA API for instance: dma_map_sg() hides all of the platform specifics of whether or not you have an IOMMU and how it is programmed. That's a good abstraction ... drivers would be a complete mess if they had to do it themselves.
Quoted 7 lines
For example, an array is simple and well understood. An abstraction would
just buy confusion and YA thing to learn.When a single array was no longer sufficient, I figured a linked-list of arrays was the simplest replacement. Easy to understand and accepted practice (tho rings are a bit exotic). Because the implementation is the interface, authors can see what is legal.
Really, I don't buy this at all. The first thing you complained about when you came up with sg_ring was how painful it was to update the 30 odd SCSI drivers. That was also my reaction when the scatterlist chaining idea came along. My second was supposing this isn't quite right and we modify the way chaining is done ... do I have to update all the drivers all over again? Hence one of the input requirements to the update was a consumer abstraction to insulate drivers from the implementation.
Thinking about information that the drivers don't need to know, the obvious thing is how the actual scatterlist is implemented, whether its a list based ring, a pointer/mark based chain or an as yet to be invented annulus. All I think most consumers need is:
1. A way to get the first scatterlist element
2. A way to get the next scatterlist element
3. An iterator over the entire scatterlist
4. A way to tell if they've reached the last scatterlist element.
Actually 1,2,4 satisfy 3 but it's such a common operation in most drivers that it was a good simplification. Other things are sizing and element count.
Quoted 2 lines
More concretely, you're already regarding your abstractions as too expensive, which is why
sg_chain()doesn't handle chained sgs.
sg_chain isn't part of the consumer interface it's part of the producer/manipulator interface. I care a lot less about getting that one right first time because it only occurs in one place in the SCSI mid-layer and is easy to update and fix.
My principal concern at the moment is the consumer interface ... if there's a fault in that we need to know now.
Quoted 14 lines
Result: you've got a complex implementation and a complex interface with a complex abstraction.
Quoted 11 lines
Quoted 3 lines
sg_chainssuck for manipulation, and AFAICT that's inherent. Here, take a look at thesg_ringconversion ofscsi_alloc_sgtableandscsi_free_sgtableand you can see why I'm unhappy with thesg_chaincode:[...]
Quoted 1 line
Hope that clarifies,
Actually, not really. If I want to continue the competition, I can just point out that your
sg_ringcode is bigger than those corresponding segments are after thesg_tableconversion ofscsi_lib.c...
Quoted 8 lines
Quoted 1 line
However, this is pointless.
No, it's exactly the point. These details *matter*. The implementation *matters*.
sg_tablemoves this code out ofscsi_lib(good!), but still demonstrates how much of a PITA they are to manipulate.As for being able to make arbitrary changes in future without hitting drivers:
this is the Kernel ABI pipe dream writ small.
No, the kernel ABI is about preserving binary semantics for proprietary drivers. We're talking about an API here which is a totally different thing. I don't believe I said anywhere that it should be a fixed API for all time ... that would also be silly. However, that doesn't mean you don't take the best care you can to get an API as right as you can. If it's wrong, it will be altered, but it is much nicer to get it right first time so it doesn't have to be.
James
Quoted 5 lines
OK, I give in with
-ETIMEDOUT. I'll go away now and do something productive :)Cheers,
Rusty.