Re: [PATCH 09/10] Change table chaining layout

[Rusty Russell <rusty@rustcorp.com.au>]

On Wednesday 24 October 2007 01:22:55 Linus Torvalds wrote:
> On Tue, 23 Oct 2007, Boaz Harrosh wrote:
> > But since we do not do that, and every single API in the kernel that
> > receives a scatterlist pointer also receives an sg_count parameter,
> > than I do not see what is so hacky about giving that sg_count parameter
> > to the one that needs it the most. sg_next();
>
> Well, I'd personally actually prefer to *not* have the count be passed
> down explicitly, because it's just too error prone.

Well, the duplication is bad, but walking lists to find the length is
inefficient so you pass around the length as well.

What irritates me more is that scatterlists aren't quite generically useful.
The virtio code wants to join a scatterlist created by blk_rq_map_sg() with
two others, yet it won't work because sg_chain() doesn't remove the end
marker from the first entry.

If this patch weren't already included, I'd be strongly arguing for the bio
idea: I find the chained sg code tricksy and ugly (sorry Jens).

To be constructive, how's this (BTW, why @arg@, I thought it was simply
"@arg"?)

===
Make sg_chain() a little more generic

Allow chaining when the first chain already has an end marker, and
change it to a slightly clearer semantic (the number of used entries
in the array, not that number plus one).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 61fdaf0..24bbc92 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -778,7 +778,7 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 		 * ended up doing another loop.
 		 */
 		if (prev)
-			sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);
+			sg_chain(prev, SCSI_MAX_SG_SEGMENTS-1, sgl);
 
 		/*
 		 * if we have nothing left, mark the last segment as
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index df7ddce..c1ef145 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -147,12 +147,11 @@ static inline struct scatterlist *sg_last(struct scatterlist *sgl,
 /**
  * sg_chain - Chain two sglists together
  * @prv:	First scatterlist
- * @prv_nents:	Number of entries in prv
+ * @prv_nents:	Number of entries used in prv
  * @sgl:	Second scatterlist
  *
  * Description:
- *   Links @prv@ and @sgl@ together, to form a longer scatterlist.
- *
+ *   @prv[@prv_nents] is used as a link to join @prv to @sgl.
  **/
 static inline void sg_chain(struct scatterlist *prv, unsigned int prv_nents,
 			    struct scatterlist *sgl)
@@ -160,7 +159,9 @@ static inline void sg_chain(struct scatterlist *prv, unsigned int prv_nents,
 #ifndef ARCH_HAS_SG_CHAIN
 	BUG();
 #endif
-	prv[prv_nents - 1].page_link = (unsigned long) sgl | 0x01;
+	if (prv_nents > 0)
+		prv[prv_nents - 1].page_link &= ~0x02UL;
+	prv[prv_nents].page_link = (unsigned long) sgl | 0x01;
 }
 
 /**
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/