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.
> - *
> + * @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;
> }
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
