[PATCH 0/2] Splitting sg chain allocation support from scsi

[PATCH 0/2] Splitting sg chain allocation support from scsi

[Jens Axboe]

Hi,

Currently scsi_lib.c does its own sg chain allocation and freeing, as it
was proof of concept. But other drivers may want to take advantage of
that feature as well. These two patches add a lib/scatterlist.c and
moves most of the linux/scatterlist.h big include into that, and adds
helpers for allocating and initializing an sg list.

This adds a new kernel define SG_MAX_SINGLE_ALLOC that sets the boundary
for when sg_alloc_table() will return a chained list, that used to be a
scsi private thing.

The patch still needs a bit of cleaning up and documenting, but it's a
start. Boots and works here.

-- 
Jens Axboe

[PATCH 1/2] SG: Move functions to lib/scatterlist.c and add sg chaining allocator helpers

[Jens Axboe]

>From 26dcdd893e845180cc822640d36521d7cb18ce38 Mon Sep 17 00:00:00 2001
From: Jens Axboe <jens.axboe@oracle.com>
Date: Wed, 14 Nov 2007 12:35:10 +0100
Subject: [PATCH] SG: Move functions to lib/scatterlist.c and add sg chaining allocator helpers

Manually doing chained sg lists is not trivial, so add some helpers
to make sure that drivers get it right.

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
 include/linux/scatterlist.h |  126 +++++------------------
 lib/Makefile                |    2 +-
 lib/scatterlist.c           |  240 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 267 insertions(+), 101 deletions(-)
 create mode 100644 lib/scatterlist.c

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 2597350..0fad621 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -7,6 +7,12 @@
 #include <linux/string.h>
 #include <asm/io.h>
 
+struct sg_table {
+	struct scatterlist *sgl;	/* the list */
+	unsigned int nents;		/* number of mapped entries */
+	unsigned int orig_nents;	/* original size of list */
+};
+
 /*
  * Notes on SG table design.
  *
@@ -98,31 +104,6 @@ static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
 #define sg_chain_ptr(sg)	\
 	((struct scatterlist *) ((sg)->page_link & ~0x03))
 
-/**
- * sg_next - return the next scatterlist entry in a list
- * @sg:		The current sg entry
- *
- * Description:
- *   Usually the next entry will be @sg@ + 1, but if this sg element is part
- *   of a chained scatterlist, it could jump to the start of a new
- *   scatterlist array.
- *
- **/
-static inline struct scatterlist *sg_next(struct scatterlist *sg)
-{
-#ifdef CONFIG_DEBUG_SG
-	BUG_ON(sg->sg_magic != SG_MAGIC);
-#endif
-	if (sg_is_last(sg))
-		return NULL;
-
-	sg++;
-	if (unlikely(sg_is_chain(sg)))
-		sg = sg_chain_ptr(sg);
-
-	return sg;
-}
-
 /*
  * Loop over each sg element, following the pointer to a new list if necessary
  */
@@ -130,40 +111,6 @@ static inline struct scatterlist *sg_next(struct scatterlist *sg)
 	for (__i = 0, sg = (sglist); __i < (nr); __i++, sg = sg_next(sg))
 
 /**
- * sg_last - return the last scatterlist entry in a list
- * @sgl:	First entry in the scatterlist
- * @nents:	Number of entries in the scatterlist
- *
- * Description:
- *   Should only be used casually, it (currently) scan the entire list
- *   to get the last entry.
- *
- *   Note that the @sgl@ pointer passed in need not be the first one,
- *   the important bit is that @nents@ denotes the number of entries that
- *   exist from @sgl@.
- *
- **/
-static inline struct scatterlist *sg_last(struct scatterlist *sgl,
-					  unsigned int nents)
-{
-#ifndef ARCH_HAS_SG_CHAIN
-	struct scatterlist *ret = &sgl[nents - 1];
-#else
-	struct scatterlist *sg, *ret = NULL;
-	unsigned int i;
-
-	for_each_sg(sgl, sg, nents, i)
-		ret = sg;
-
-#endif
-#ifdef CONFIG_DEBUG_SG
-	BUG_ON(sgl[0].sg_magic != SG_MAGIC);
-	BUG_ON(!sg_is_last(ret));
-#endif
-	return ret;
-}
-
-/**
  * sg_chain - Chain two sglists together
  * @prv:	First scatterlist
  * @prv_nents:	Number of entries in prv
@@ -208,47 +155,6 @@ static inline void sg_mark_end(struct scatterlist *sg)
 }
 
 /**
- * sg_init_table - Initialize SG table
- * @sgl:	   The SG table
- * @nents:	   Number of entries in table
- *
- * Notes:
- *   If this is part of a chained sg table, sg_mark_end() should be
- *   used only on the last table part.
- *
- **/
-static inline void sg_init_table(struct scatterlist *sgl, unsigned int nents)
-{
-	memset(sgl, 0, sizeof(*sgl) * nents);
-#ifdef CONFIG_DEBUG_SG
-	{
-		unsigned int i;
-		for (i = 0; i < nents; i++)
-			sgl[i].sg_magic = SG_MAGIC;
-	}
-#endif
-	sg_mark_end(&sgl[nents - 1]);
-}
-
-/**
- * sg_init_one - Initialize a single entry sg list
- * @sg:		 SG entry
- * @buf:	 Virtual address for IO
- * @buflen:	 IO length
- *
- * Notes:
- *   This should not be used on a single entry that is part of a larger
- *   table. Use sg_init_table() for that.
- *
- **/
-static inline void sg_init_one(struct scatterlist *sg, const void *buf,
-			       unsigned int buflen)
-{
-	sg_init_table(sg, 1);
-	sg_set_buf(sg, buf, buflen);
-}
-
-/**
  * sg_phys - Return physical address of an sg entry
  * @sg:	     SG entry
  *
@@ -278,4 +184,24 @@ static inline void *sg_virt(struct scatterlist *sg)
 	return page_address(sg_page(sg)) + sg->offset;
 }
 
+struct scatterlist *sg_next(struct scatterlist *);
+struct scatterlist *sg_last(struct scatterlist *s, unsigned int);
+void sg_init_table(struct scatterlist *, unsigned int);
+void sg_init_one(struct scatterlist *, const void *, unsigned int);
+
+typedef struct scatterlist *(sg_alloc_fn)(unsigned int, gfp_t);
+typedef void (sg_free_fn)(struct scatterlist *, unsigned int);
+
+void __sg_free_table(struct sg_table *, sg_free_fn *);
+void sg_free_table(struct sg_table *);
+int __sg_alloc_table(struct sg_table *, unsigned int, gfp_t, sg_alloc_fn *,
+			sg_free_fn *);
+int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
+
+/*
+ * Maximum number of entries that will be allocated in one piece, if
+ * a list larger than this is required then chaining will be utilized.
+ */
+#define SG_MAX_SINGLE_ALLOC		128
+
 #endif /* _LINUX_SCATTERLIST_H */
diff --git a/lib/Makefile b/lib/Makefile
index 3a0983b..a3e2947 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -6,7 +6,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 rbtree.o radix-tree.o dump_stack.o \
 	 idr.o int_sqrt.o bitmap.o extable.o prio_tree.o \
 	 sha1.o irq_regs.o reciprocal_div.o argv_split.o \
-	 proportions.o prio_heap.o
+	 proportions.o prio_heap.o scatterlist.o
 
 lib-$(CONFIG_MMU) += ioremap.o
 lib-$(CONFIG_SMP) += cpumask.o
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
new file mode 100644
index 0000000..2175b3c
--- /dev/null
+++ b/lib/scatterlist.c
@@ -0,0 +1,240 @@
+#include <linux/module.h>
+#include <linux/scatterlist.h>
+
+/**
+ * sg_next - return the next scatterlist entry in a list
+ * @sg:		The current sg entry
+ *
+ * Description:
+ *   Usually the next entry will be @sg + 1, but if this sg element is part
+ *   of a chained scatterlist, it could jump to the start of a new
+ *   scatterlist array.
+ *
+ **/
+struct scatterlist *sg_next(struct scatterlist *sg)
+{
+#ifdef CONFIG_DEBUG_SG
+	BUG_ON(sg->sg_magic != SG_MAGIC);
+#endif
+	if (sg_is_last(sg))
+		return NULL;
+
+	sg++;
+	if (unlikely(sg_is_chain(sg)))
+		sg = sg_chain_ptr(sg);
+
+	return sg;
+}
+EXPORT_SYMBOL(sg_next);
+
+/**
+ * sg_last - return the last scatterlist entry in a list
+ * @sgl:	First entry in the scatterlist
+ * @nents:	Number of entries in the scatterlist
+ *
+ * Description:
+ *   Should only be used casually, it (currently) scan the entire list
+ *   to get the last entry.
+ *
+ *   Note that the @sgl@ pointer passed in need not be the first one,
+ *   the important bit is that @nents@ denotes the number of entries that
+ *   exist from @sgl@.
+ *
+ **/
+struct scatterlist *sg_last(struct scatterlist *sgl, unsigned int nents)
+{
+#ifndef ARCH_HAS_SG_CHAIN
+	struct scatterlist *ret = &sgl[nents - 1];
+#else
+	struct scatterlist *sg, *ret = NULL;
+	unsigned int i;
+
+	for_each_sg(sgl, sg, nents, i)
+		ret = sg;
+
+#endif
+#ifdef CONFIG_DEBUG_SG
+	BUG_ON(sgl[0].sg_magic != SG_MAGIC);
+	BUG_ON(!sg_is_last(ret));
+#endif
+	return ret;
+}
+EXPORT_SYMBOL(sg_last);
+
+/**
+ * sg_init_table - Initialize SG table
+ * @sgl:	   The SG table
+ * @nents:	   Number of entries in table
+ *
+ * Notes:
+ *   If this is part of a chained sg table, sg_mark_end() should be
+ *   used only on the last table part.
+ *
+ **/
+void sg_init_table(struct scatterlist *sgl, unsigned int nents)
+{
+	memset(sgl, 0, sizeof(*sgl) * nents);
+#ifdef CONFIG_DEBUG_SG
+	{
+		unsigned int i;
+		for (i = 0; i < nents; i++)
+			sgl[i].sg_magic = SG_MAGIC;
+	}
+#endif
+	sg_mark_end(&sgl[nents - 1]);
+}
+EXPORT_SYMBOL(sg_init_table);
+
+/**
+ * sg_init_one - Initialize a single entry sg list
+ * @sg:		 SG entry
+ * @buf:	 Virtual address for IO
+ * @buflen:	 IO length
+ *
+ * Notes:
+ *   This should not be used on a single entry that is part of a larger
+ *   table. Use sg_init_table() for that.
+ *
+ **/
+void sg_init_one(struct scatterlist *sg, const void *buf, unsigned int buflen)
+{
+	sg_init_table(sg, 1);
+	sg_set_buf(sg, buf, buflen);
+}
+EXPORT_SYMBOL(sg_init_one);
+
+static struct scatterlist *sg_kmalloc(unsigned int nents, gfp_t gfp_mask)
+{
+	return kmalloc(nents * sizeof(struct scatterlist), gfp_mask);
+}
+
+static void sg_kfree(struct scatterlist *sg, unsigned int nents)
+{
+	kfree(sg);
+}
+
+/**
+ * __sg_free_table - Free a previously mapped sg table
+ * @table:	The sg table header to use
+ * @free_fn:	Free function
+ *
+ **/
+void __sg_free_table(struct sg_table *table, sg_free_fn *free_fn)
+{
+	struct scatterlist *sgl = table->sgl;
+
+	if (table->orig_nents > SG_MAX_SINGLE_ALLOC) {
+		unsigned int alloc_size, sg_size, total;
+		struct scatterlist *next;
+
+		total = table->orig_nents - SG_MAX_SINGLE_ALLOC;
+		next = sg_chain_ptr(&sgl[SG_MAX_SINGLE_ALLOC - 1]);
+		while (total && next) {
+			sgl = next;
+			alloc_size = total;
+			if (alloc_size > SG_MAX_SINGLE_ALLOC) {
+				alloc_size = SG_MAX_SINGLE_ALLOC;
+				sg_size = alloc_size - 1;
+			} else
+				sg_size = alloc_size;
+
+			total -= sg_size;
+			table->orig_nents -= sg_size;
+			if (total)
+				next = sg_chain_ptr(&sgl[alloc_size - 1]);
+
+			free_fn(sgl, alloc_size);
+		}
+	}
+
+	free_fn(table->sgl, table->orig_nents);
+	table->sgl = NULL;
+}
+
+/**
+ * sg_free_table - Free a previously allocated sg table
+ * @table:	The mapped sg table header
+ *
+ **/
+void sg_free_table(struct sg_table *table)
+{
+	__sg_free_table(table, sg_kfree);
+}
+EXPORT_SYMBOL(sg_free_table);
+
+/**
+ * __sg_alloc_table - Allocate and initialize an sg table with given allocator
+ * @table:	The sg table header to use
+ * @nents:	Number of entries in sg list
+ * @gfp_mask:	GFP allocation mask
+ * @alloc_fn:	Allocator to use
+ * @free_fn:	Free function
+ *
+ **/
+int __sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask,
+		     sg_alloc_fn *alloc_fn, sg_free_fn *free_fn)
+{
+	struct scatterlist *sg, *prv;
+	unsigned int left;
+
+	memset(table, 0, sizeof(*table));
+
+	left = nents;
+	prv = NULL;
+	do {
+		unsigned int sg_size, alloc_size = left;
+
+		if (alloc_size > SG_MAX_SINGLE_ALLOC) {
+			alloc_size = SG_MAX_SINGLE_ALLOC;
+			sg_size = alloc_size - 1;
+		} else
+			sg_size = alloc_size;
+
+		left -= sg_size;
+
+		sg = alloc_fn(alloc_size, gfp_mask);
+		if (unlikely(!sg))
+			goto enomem;
+
+		sg_init_table(sg, alloc_size);
+		table->nents = table->orig_nents += sg_size;
+
+		/*
+		 * If this is the first mapping, assign the sg table header.
+		 * If this is not the first mapping, chain previous part.
+		 */
+		if (prv)
+			sg_chain(prv, SG_MAX_SINGLE_ALLOC, sg);
+		else
+			table->sgl = sg;
+
+		/*
+		 * If no more entries after this one, mark the end
+		 */
+		if (!left)
+			sg_mark_end(&sg[sg_size - 1]);
+
+		gfp_mask &= ~__GFP_WAIT;
+		gfp_mask |= __GFP_HIGH;
+		prv = sg;
+	} while (left);
+
+	return 0;
+enomem:
+	__sg_free_table(table, free_fn);
+	return -ENOMEM;
+}
+EXPORT_SYMBOL(__sg_alloc_table);
+
+/**
+ * sg_alloc_table - Allocate and initialize an sg table
+ * @table:	The sg table header to use
+ * @nents:	Number of entries in sg list
+ * @gfp_mask:	GFP allocation mask
+ *
+ **/
+int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
+{
+	return __sg_alloc_table(table, nents, gfp_mask, sg_kmalloc, sg_kfree);
+}
+EXPORT_SYMBOL(sg_alloc_table);
-- 
1.5.3.GIT

-- 
Jens Axboe

[PATCH 2/2] SG: Convert SCSI to use scatterlist helpers for sg chaining

[Jens Axboe]

>From 7b2421e1c075d3c262e28d0598608141add2e7da Mon Sep 17 00:00:00 2001
From: Jens Axboe <jens.axboe@oracle.com>
Date: Wed, 14 Nov 2007 12:36:47 +0100
Subject: [PATCH] SG: Convert SCSI to use scatterlist helpers for sg chaining

Also change scsi_alloc_sgtable() to just return 0/failure, since it
maps to the command passed in. ->request_buffer is now no longer needed,
once drivers are adapted to use scsi_sglist() it can be killed.

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
 drivers/scsi/scsi_lib.c     |  137 ++++++-------------------------------------
 drivers/scsi/scsi_tgt_lib.c |    3 +-
 include/scsi/scsi_cmnd.h    |    7 +-
 3 files changed, 23 insertions(+), 124 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0e81e4c..1fb3c2e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -737,138 +737,40 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
 	return index;
 }
 
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static void scsi_sg_free(struct scatterlist *sgl, unsigned int nents)
 {
 	struct scsi_host_sg_pool *sgp;
-	struct scatterlist *sgl, *prev, *ret;
-	unsigned int index;
-	int this, left;
-
-	BUG_ON(!cmd->use_sg);
-
-	left = cmd->use_sg;
-	ret = prev = NULL;
-	do {
-		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;
-
-		sgl = mempool_alloc(sgp->pool, gfp_mask);
-		if (unlikely(!sgl))
-			goto enomem;
+	sgp = scsi_sg_pools + scsi_sgtable_index(nents);
+	mempool_free(sgl, sgp->pool);
+}
 
-		sg_init_table(sgl, sgp->size);
+static struct scatterlist *scsi_sg_alloc(unsigned int nents, gfp_t gfp_mask)
+{
+	struct scsi_host_sg_pool *sgp;
 
-		/*
-		 * first loop through, set initial index and return value
-		 */
-		if (!ret)
-			ret = sgl;
+	sgp = scsi_sg_pools + scsi_sgtable_index(nents);
+	return mempool_alloc(sgp->pool, gfp_mask);
+}
 
-		/*
-		 * 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);
+int scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+{
+	int ret;
 
-		/*
-		 * if we have nothing left, mark the last segment as
-		 * end-of-list
-		 */
-		if (!left)
-			sg_mark_end(&sgl[this - 1]);
+	BUG_ON(!cmd->use_sg);
 
-		/*
-		 * don't allow subsequent mempool allocs to sleep, it would
-		 * violate the mempool principle.
-		 */
-		gfp_mask &= ~__GFP_WAIT;
-		gfp_mask |= __GFP_HIGH;
-		prev = sgl;
-	} while (left);
+	ret = __sg_alloc_table(&cmd->sg_table, cmd->use_sg, gfp_mask,
+					scsi_sg_alloc, scsi_sg_free);
 
-	/*
-	 * ->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;
+	cmd->request_buffer = cmd->sg_table.sgl;
 	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);
-	}
-	return NULL;
 }
 
 EXPORT_SYMBOL(scsi_alloc_sgtable);
 
 void scsi_free_sgtable(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);
+	__sg_free_table(&cmd->sg_table, scsi_sg_free);
 }
 
 EXPORT_SYMBOL(scsi_free_sgtable);
@@ -1119,8 +1021,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
 	/*
 	 * If sg table allocation fails, requeue request later.
 	 */
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
-	if (unlikely(!cmd->request_buffer)) {
+	if (unlikely(scsi_alloc_sgtable(cmd, GFP_ATOMIC))) {
 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index a91761c..742e1e3 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -359,8 +359,7 @@ static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 	int count;
 
 	cmd->use_sg = rq->nr_phys_segments;
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
-	if (!cmd->request_buffer)
+	if (scsi_alloc_sgtable(cmd, gfp_mask))
 		return -ENOMEM;
 
 	cmd->request_bufflen = rq->data_len;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 3f47e52..40f7c7c 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -8,7 +8,6 @@
 #include <linux/scatterlist.h>
 
 struct request;
-struct scatterlist;
 struct Scsi_Host;
 struct scsi_device;
 
@@ -68,8 +67,8 @@ struct scsi_cmnd {
 	void *request_buffer;		/* Actual requested buffer */
 
 	/* These elements define the operation we ultimately want to perform */
+	struct sg_table sg_table;
 	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 +127,14 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 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 int scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
 extern void scsi_free_sgtable(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_sglist(cmd) ((cmd)->sg_table.sgl)
 #define scsi_bufflen(cmd) ((cmd)->request_bufflen)
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
-- 
1.5.3.GIT

-- 
Jens Axboe

Re: [PATCH 2/2] SG: Convert SCSI to use scatterlist helpers for sg chaining

[Benny Halevy]

On Nov. 14, 2007, 13:47 +0200, Jens Axboe <jens.axboe@oracle.com> wrote:
>>From 7b2421e1c075d3c262e28d0598608141add2e7da Mon Sep 17 00:00:00 2001
> From: Jens Axboe <jens.axboe@oracle.com>
> Date: Wed, 14 Nov 2007 12:36:47 +0100
> Subject: [PATCH] SG: Convert SCSI to use scatterlist helpers for sg chaining
> 
> Also change scsi_alloc_sgtable() to just return 0/failure, since it
> maps to the command passed in. ->request_buffer is now no longer needed,
> once drivers are adapted to use scsi_sglist() it can be killed.
> 
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> ---
>  drivers/scsi/scsi_lib.c     |  137 ++++++-------------------------------------
>  drivers/scsi/scsi_tgt_lib.c |    3 +-
>  include/scsi/scsi_cmnd.h    |    7 +-
>  3 files changed, 23 insertions(+), 124 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 0e81e4c..1fb3c2e 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -737,138 +737,40 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
>  	return index;
>  }
>  
> -struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
> +static void scsi_sg_free(struct scatterlist *sgl, unsigned int nents)
>  {
>  	struct scsi_host_sg_pool *sgp;
> -	struct scatterlist *sgl, *prev, *ret;
> -	unsigned int index;
> -	int this, left;
> -
> -	BUG_ON(!cmd->use_sg);
> -
> -	left = cmd->use_sg;
> -	ret = prev = NULL;
> -	do {
> -		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;
> -
> -		sgl = mempool_alloc(sgp->pool, gfp_mask);
> -		if (unlikely(!sgl))
> -			goto enomem;
> +	sgp = scsi_sg_pools + scsi_sgtable_index(nents);
> +	mempool_free(sgl, sgp->pool);
> +}
>  
> -		sg_init_table(sgl, sgp->size);
> +static struct scatterlist *scsi_sg_alloc(unsigned int nents, gfp_t gfp_mask)
> +{
> +	struct scsi_host_sg_pool *sgp;
>  
> -		/*
> -		 * first loop through, set initial index and return value
> -		 */
> -		if (!ret)
> -			ret = sgl;
> +	sgp = scsi_sg_pools + scsi_sgtable_index(nents);
> +	return mempool_alloc(sgp->pool, gfp_mask);
> +}
>  
> -		/*
> -		 * 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);
> +int scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
> +{
> +	int ret;
>  
> -		/*
> -		 * if we have nothing left, mark the last segment as
> -		 * end-of-list
> -		 */
> -		if (!left)
> -			sg_mark_end(&sgl[this - 1]);
> +	BUG_ON(!cmd->use_sg);
>  
> -		/*
> -		 * don't allow subsequent mempool allocs to sleep, it would
> -		 * violate the mempool principle.
> -		 */
> -		gfp_mask &= ~__GFP_WAIT;
> -		gfp_mask |= __GFP_HIGH;
> -		prev = sgl;
> -	} while (left);
> +	ret = __sg_alloc_table(&cmd->sg_table, cmd->use_sg, gfp_mask,
> +					scsi_sg_alloc, scsi_sg_free);
>  
> -	/*
> -	 * ->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;
> +	cmd->request_buffer = cmd->sg_table.sgl;
>  	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);
> -	}
> -	return NULL;
>  }
>  
>  EXPORT_SYMBOL(scsi_alloc_sgtable);

<snip>

> @@ -128,14 +127,14 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
>  				 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 int scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);


This becomes even nicer on top of the last sdb patches that Boaz sent:
http://www.spinics.net/lists/linux-scsi/msg20947.html
and
http://www.spinics.net/lists/linux-scsi/msg20948.html

that unexport this scsi_alloc_sgtable and make it look like this:
+static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb,
+					unsigned short sg_count, gfp_t gfp_mask)

Re: [PATCH 2/2] SG: Convert SCSI to use scatterlist helpers for sg chaining

[Jens Axboe]

On Wed, Nov 14 2007, Benny Halevy wrote:
> On Nov. 14, 2007, 13:47 +0200, Jens Axboe <jens.axboe@oracle.com> wrote:
> >>From 7b2421e1c075d3c262e28d0598608141add2e7da Mon Sep 17 00:00:00 2001
> > From: Jens Axboe <jens.axboe@oracle.com>
> > Date: Wed, 14 Nov 2007 12:36:47 +0100
> > Subject: [PATCH] SG: Convert SCSI to use scatterlist helpers for sg chaining
> > 
> > Also change scsi_alloc_sgtable() to just return 0/failure, since it
> > maps to the command passed in. ->request_buffer is now no longer needed,
> > once drivers are adapted to use scsi_sglist() it can be killed.
> > 
> > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > ---
> >  drivers/scsi/scsi_lib.c     |  137 ++++++-------------------------------------
> >  drivers/scsi/scsi_tgt_lib.c |    3 +-
> >  include/scsi/scsi_cmnd.h    |    7 +-
> >  3 files changed, 23 insertions(+), 124 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 0e81e4c..1fb3c2e 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -737,138 +737,40 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
> >  	return index;
> >  }
> >  
> > -struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
> > +static void scsi_sg_free(struct scatterlist *sgl, unsigned int nents)
> >  {
> >  	struct scsi_host_sg_pool *sgp;
> > -	struct scatterlist *sgl, *prev, *ret;
> > -	unsigned int index;
> > -	int this, left;
> > -
> > -	BUG_ON(!cmd->use_sg);
> > -
> > -	left = cmd->use_sg;
> > -	ret = prev = NULL;
> > -	do {
> > -		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;
> > -
> > -		sgl = mempool_alloc(sgp->pool, gfp_mask);
> > -		if (unlikely(!sgl))
> > -			goto enomem;
> > +	sgp = scsi_sg_pools + scsi_sgtable_index(nents);
> > +	mempool_free(sgl, sgp->pool);
> > +}
> >  
> > -		sg_init_table(sgl, sgp->size);
> > +static struct scatterlist *scsi_sg_alloc(unsigned int nents, gfp_t gfp_mask)
> > +{
> > +	struct scsi_host_sg_pool *sgp;
> >  
> > -		/*
> > -		 * first loop through, set initial index and return value
> > -		 */
> > -		if (!ret)
> > -			ret = sgl;
> > +	sgp = scsi_sg_pools + scsi_sgtable_index(nents);
> > +	return mempool_alloc(sgp->pool, gfp_mask);
> > +}
> >  
> > -		/*
> > -		 * 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);
> > +int scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
> > +{
> > +	int ret;
> >  
> > -		/*
> > -		 * if we have nothing left, mark the last segment as
> > -		 * end-of-list
> > -		 */
> > -		if (!left)
> > -			sg_mark_end(&sgl[this - 1]);
> > +	BUG_ON(!cmd->use_sg);
> >  
> > -		/*
> > -		 * don't allow subsequent mempool allocs to sleep, it would
> > -		 * violate the mempool principle.
> > -		 */
> > -		gfp_mask &= ~__GFP_WAIT;
> > -		gfp_mask |= __GFP_HIGH;
> > -		prev = sgl;
> > -	} while (left);
> > +	ret = __sg_alloc_table(&cmd->sg_table, cmd->use_sg, gfp_mask,
> > +					scsi_sg_alloc, scsi_sg_free);
> >  
> > -	/*
> > -	 * ->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;
> > +	cmd->request_buffer = cmd->sg_table.sgl;
> >  	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);
> > -	}
> > -	return NULL;
> >  }
> >  
> >  EXPORT_SYMBOL(scsi_alloc_sgtable);
> 
> <snip>
> 
> > @@ -128,14 +127,14 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
> >  				 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 int scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
> 
> 
> This becomes even nicer on top of the last sdb patches that Boaz sent:
> http://www.spinics.net/lists/linux-scsi/msg20947.html
> and
> http://www.spinics.net/lists/linux-scsi/msg20948.html
> 
> that unexport this scsi_alloc_sgtable and make it look like this:
> +static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb,
> +					unsigned short sg_count, gfp_t gfp_mask)

Perfect, that ties in nicely!

-- 
Jens Axboe