Re: [PATCH 11/13] SCSI: support for allocating large scatterlists

Jens Axboe
2007-05-10T10:52:59Z

On Thu, May 10 2007, Andrew Morton wrote:
> On Thu, 10 May 2007 12:21:53 +0200 Jens Axboe <jens.axboe@oracle.com> wrote:
>
> > This is what enables large commands. If we need to allocate an
> > sgtable that doesn't fit in a single page, allocate several
> > SCSI_MAX_SG_SEGMENTS sized tables and chain them together.
> >
> > We default to the safe setup of NOT chaining, for now.
> >
> > ...
> >
> > +/*
> > + * Should fit within a single page, and must be a power-of-2.
> > + */
> > +#define SCSI_MAX_SG_SEGMENTS 128
>
> But what units is it in? Bytes? sizeof(void*)?

sg elements. It's not new, just juggled around a bit. The comment is a
bit stale now though, it doesn't have to be a pow-of-2. Ideally we just
want to make sure that sizeof(struct scatterlist) * SCSI_MAX_SG_SEGMENTS
<= PAGE_SIZE to avoid 2^1 allocations.

> > +struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
> > +{
> > + struct scsi_host_sg_pool *sgp;
> > + struct scatterlist *sgl, *prev, *ret;
> > + unsigned int index;
> > + int this, left;
> > +
> > + BUG_ON(!cmd->use_sg);
> > +
> > + left = cmd->use_sg;
> > + ret = prev = NULL;
> > + do {
> > + this = left;
> > + if (this > SCSI_MAX_SG_SEGMENTS) {
> > + this = SCSI_MAX_SG_SEGMENTS;
> > + index = SG_MEMPOOL_NR - 1;
> > + } else
> > + index = scsi_sgtable_index(this);
> > +
> > + left -= this;
> > +
> > + /*
> > + * if we have more entries after this round, reserve a slot
> > + * for the chain pointer.
> > + */
> > + if (left)
> > + left++;
> > +
> > + sgp = scsi_sg_pools + index;
> > +
> > + sgl = mempool_alloc(sgp->pool, gfp_mask);
> > + if (unlikely(!sgl))
> > + goto enomem;
> > +
> > + memset(sgl, 0, sizeof(*sgl) * sgp->size);
> > +
> > + /*
> > + * first loop through, set initial index and return value
> > + */
> > + if (!ret) {
> > + cmd->sglist_len = index;
> > + ret = sgl;
> > + }
> > +
> > + /*
> > + * chain previous sglist, if any. we know the previous
> > + * sglist must be the biggest one, or we would not have
> > + * ended up doing another loop.
> > + */
> > + if (prev)
> > + sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);
> > +
> > + /*
> > + * don't allow subsequent mempool allocs to sleep, it would
> > + * violate the mempool principle.
> > + */
> > + gfp_mask &= ~__GFP_WAIT;
>
> hrm.
>
> Might want to set __GFP_HIGH here too.

Agree, I did consider that (clear wait, set ATOMIC to get the __HIGH).

--
Jens Axboe