On Thu, Oct 25 2007, Rusty Russell wrote: > 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.
That's a minor nit for your special purpose, we/you can change that. > 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). What is the bio idea? A bio works in essentially the same way, the only difference is having a specific next pointer. It's still just a linked lists of arbitrarily sized sg tables (the bio_vec arrays). > > 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 <[EMAIL PROTECTED]> > > 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); What's this bit for? > 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. > - * > + * @[EMAIL PROTECTED] 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; > } We definitely should clear any other markers, that makes sense. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/