Re: [PATCH 03/13] scsi: unify allocation of scsi command and sense buffer

[Boaz Harrosh <bharrosh@panasas.com>]

On 05/25/2009 10:30 AM, Jens Axboe wrote:
> Fold the sense buffer into the command, thereby eliminating a slab
> allocation and free per command.
> 
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

Jens Hi.

I'm "TO:" this to Tomo.

This is the way it used to be for a long time. It was only recently changed by
Tomo because of a bug on none-cache-coherent arches that need to dma-access the
sense_buffer and also on the other hand change scsi_cmnd members by CPU.

In my opinion all you need is an __aligned(SMP_CACHE_BYTES) declaration at
sense_buffer[] and let there be a hole at the end before the array. But Tomo
did not like that, so he separated the two.

Ideally there should be a MACRO that is defined to WORD_SIZE on cache-coherent
ARCHs and to SMP_CACHE_BYTES on none-cache-coherent systems and use that size
at the __align() attribute. (So only stupid ARCHES get hurt)

(see below)
> ---
>  drivers/scsi/scsi.c      |   44 ++++++++++----------------------------------
>  include/scsi/scsi_cmnd.h |   12 ++++++------
>  2 files changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 166417a..6a993af 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -133,7 +133,6 @@ EXPORT_SYMBOL(scsi_device_type);
>  
>  struct scsi_host_cmd_pool {
>  	struct kmem_cache	*cmd_slab;
> -	struct kmem_cache	*sense_slab;
>  	unsigned int		users;
>  	char			*cmd_name;
>  	char			*sense_name;
> @@ -167,20 +166,9 @@ static DEFINE_MUTEX(host_cmd_pool_mutex);
>  static struct scsi_cmnd *
>  scsi_pool_alloc_command(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
>  {
> -	struct scsi_cmnd *cmd;
> -
> -	cmd = kmem_cache_zalloc(pool->cmd_slab, gfp_mask | pool->gfp_mask);
> -	if (!cmd)
> -		return NULL;
> +	gfp_t gfp = gfp_mask | pool->gfp_mask;
>  
> -	cmd->sense_buffer = kmem_cache_alloc(pool->sense_slab,
> -					     gfp_mask | pool->gfp_mask);
> -	if (!cmd->sense_buffer) {
> -		kmem_cache_free(pool->cmd_slab, cmd);
> -		return NULL;
> -	}
> -
> -	return cmd;
> +	return kmem_cache_zalloc(pool->cmd_slab, gfp);
>  }
>  
>  /**
> @@ -198,7 +186,6 @@ scsi_pool_free_command(struct scsi_host_cmd_pool *pool,
>  	if (cmd->prot_sdb)
>  		kmem_cache_free(scsi_sdb_cache, cmd->prot_sdb);
>  
> -	kmem_cache_free(pool->sense_slab, cmd->sense_buffer);
>  	kmem_cache_free(pool->cmd_slab, cmd);
>  }
>  
> @@ -242,7 +229,6 @@ scsi_host_alloc_command(struct Scsi_Host *shost, gfp_t gfp_mask)
>  struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
>  {
>  	struct scsi_cmnd *cmd;
> -	unsigned char *buf;
>  
>  	cmd = scsi_host_alloc_command(shost, gfp_mask);
>  
> @@ -257,11 +243,8 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
>  		}
>  		spin_unlock_irqrestore(&shost->free_list_lock, flags);
>  
> -		if (cmd) {
> -			buf = cmd->sense_buffer;
> +		if (cmd)
>  			memset(cmd, 0, sizeof(*cmd));
> -			cmd->sense_buffer = buf;
> -		}
>  	}
>  
>  	return cmd;
> @@ -361,19 +344,13 @@ static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
>  	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
>  		&scsi_cmd_pool;
>  	if (!pool->users) {
> -		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
> -						   sizeof(struct scsi_cmnd), 0,
> -						   pool->slab_flags, NULL);
> -		if (!pool->cmd_slab)
> -			goto fail;
> +		unsigned int slab_size;
>  
> -		pool->sense_slab = kmem_cache_create(pool->sense_name,
> -						     SCSI_SENSE_BUFFERSIZE, 0,
> -						     pool->slab_flags, NULL);
> -		if (!pool->sense_slab) {
> -			kmem_cache_destroy(pool->cmd_slab);
> +		slab_size = sizeof(struct scsi_cmnd) + SCSI_SENSE_BUFFERSIZE;

You might as well just define sense array as unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE]
and save the manual calculation.

> +		pool->cmd_slab = kmem_cache_create(pool->cmd_name, slab_size,
> +						   0, pool->slab_flags, NULL);
> +		if (!pool->cmd_slab)
>  			goto fail;
> -		}
>  	}
>  
>  	pool->users++;
> @@ -397,10 +374,9 @@ static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
>  	 */
>  	BUG_ON(pool->users == 0);
>  
> -	if (!--pool->users) {
> +	if (!--pool->users)
>  		kmem_cache_destroy(pool->cmd_slab);
> -		kmem_cache_destroy(pool->sense_slab);
> -	}
> +
>  	mutex_unlock(&host_cmd_pool_mutex);
>  }
>  
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 43b50d3..649ad36 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -102,12 +102,6 @@ struct scsi_cmnd {
>  	struct request *request;	/* The command we are
>  				   	   working on */
>  
> -#define SCSI_SENSE_BUFFERSIZE 	96
> -	unsigned char *sense_buffer;
> -				/* obtained by REQUEST SENSE when
> -				 * CHECK CONDITION is received on original
> -				 * command (auto-sense) */
> -
>  	/* Low-level done function - can be used by low-level driver to point
>  	 *        to completion function.  Not used by mid/upper level code. */
>  	void (*scsi_done) (struct scsi_cmnd *);
> @@ -129,6 +123,12 @@ struct scsi_cmnd {
>  	int result;		/* Status code from lower level driver */
>  
>  	unsigned char tag;	/* SCSI-II queued command tag */
> +
> +#define SCSI_SENSE_BUFFERSIZE 	96
> +	unsigned char sense_buffer[0];

+	unsigned char sense_buffer[BUFFERSIZE]; __aligned(CACHE_COHERENT_BYTES)

> +				/* obtained by REQUEST SENSE when
> +				 * CHECK CONDITION is received on original
> +				 * command (auto-sense) */
>  };
>  
>  extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);

Thanks
Boaz