In a review comment to this post:
[PATCH v2 12/18] sg: sense buffer rework
Hannes said:
> Maybe it's worthwhile using a mempool here for sense buffers; frequest
> kmalloc()/kfree() really should be avoided.
>
> Also the allocation at end_io time is slightly dodgy; I'd rather have
> the sense allocated before setting up the command. Thing is, the end_io
> callback might be called at any time and in about every context
> (including from an interrupt handler), so I really would avoid having to
> do kmalloc() there.
The problem, seen from the POV of the sg driver, is to get a sense_buffer
either before, or at the time of, sg_rq_end_io(). It needs to be by the end
of that call because the inherited code for the sg driver comments that
the associated request and scsi_request object(s) will be "freed" toward
the end of sg_rq_end_io() to maximize re-use of those objects.
To get a sense_buffer _before_ sg_rq_end_io() seems an unnecessary imposition
as I would guesstimate that the sense buffer is actually needed between
1% and 0.00001% of the time. Then that leaves fetching a sense_buffer within
sg_rq_end_io() _only_ when it is needed, copying the contents from either
scsi_request::sense [or scsi_cmnd::sense_buffer, which is damn confusing]
into an sg owned sense buffer before the request and associated
scsi_request (and associated scsi_cmnd) object is "freed" (actually put back
into the pool for re-use, I suspect).
Now addressing Hannes' comment: scsi_lib.c already has a sense buffer pool,
actually two of them:
static struct kmem_cache *scsi_sense_cache;
static struct kmem_cache *scsi_sense_isadma_cache;
So it would be good to re-use that code, as I assume it works and is
efficient. But the two needed functions:
scsi_alloc_sense_buffer() and scsi_free_sense_buffer()
are declared static and not exported.
So is that cache appropriate for the sg driver (and perhaps st and
ses drivers) and if so can those scsi_lib functions be exported?
They are a little over-constrained for what the sg driver needs.
As for the "dodgy" comment, I believe that is only in cases where
kernel janitors come along and unwisely change a gfp_mask from
GFP_ATOMIC to GFP_KERNEL. Hopefully a comment like
... , GFP_ATOMIC /* <-- leave */);
will catch the attention of a janitor, a reviewer, or someone debugging
broken code.
Doug Gilbert
PS: The split personalities of scsi_request and scsi_cmnd:
Here is a quick survey of ULDs in the scsi subsystem:
ULD Uses
==========================
sd scsi_cmnd (only)
sr scsi_cmnd (only)
st scsi_request (only)
sg scsi_request (only)
ch neither
ses neither
Rationale ?
