Messages in this thread Patch in this message | | | From | Rusty Russell <> | Subject | Re: [PATCH 09/10] Change table chaining layout | Date | Thu, 25 Oct 2007 21:54:44 +1000 |
| |
On Thursday 25 October 2007 19:11:40 Jens Axboe wrote: > On Thu, Oct 25 2007, Rusty Russell wrote: > > 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.
Well currently sg_chain() only joins "incomplete" (ie. unterminated) sg chains. That works great for you, but it feels more like a special purpose to me.
> > 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).
It was suggested by analogy earlier in this thread, to use a two-level structure.
In this case I would have first renamed struct scatterlist to struct scatterelem. Then struct scatterlist looks like:
struct scatterlist { unsigned int num; struct scatterelem elems[0]; };
We'd want a nice macro to declare them for the stack case:
#define DEFINE_SCATTERLIST(name, elems) \ struct { \ struct scatterlist sg; \ struct scatterelem elems[num]; \ } name
Now we've tied the number and array together, we can introduce:
struct sg_multilist { unsigned int num_scatterlists; struct scatterlist *sg_array[0]; };
And, of course, a common way to represent a one-sglist array:
#define DEFINE_SG_MULTI(name, num) \ struct { \ struct sg_multilist ml; \ struct scatterlist *sg_array; \ struct scatterlist sg; \ struct scatterelem elems[num]; \ } name = { .ml = { 1 }, .sg_array = &name.sg }
Now simply replace all the places which expect a "struct scatterlist" with "struct sg_multilist" and we're done.
Using dangling structures is not as neat as using pointers, but it's very efficient.
> > @@ -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?
I changed the sg_chain() function not to take one off the argument. It made more sense when I wrote the virtblk code (here it's natural, since the num elements used + 1 == size of array).
> > - 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.
Agreed, and it was the use of "prv_nents - 2" in that code which made me think the arg should be "num used" not "one past the num used".
Cheers, Rusty. - 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/
| |