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 [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]
