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

[Jens Axboe <jens.axboe@oracle.com>]

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 <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);

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