Re: [PATCH v3] use dynamically allocated sense buffer

[Boaz Harrosh <bharrosh@panasas.com>]

On Wed, Jan 16 2008 at 6:32 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> This is the third version of:
> 
> http://marc.info/?l=linux-scsi&m=120038907123706&w=2
> 
> The changes from the second version are:
> 
> - Fixed the memory leak bug that Boaz pointed out.
> 
> shots->backup_sense_buffer has gone. One sense buffer is allocated per
> host and it's always attached to the scsi_cmnd linked with freelist
> (backup command).
> 
> - Calling scsi_setup_command_sense_buffer in scsi_host_alloc instead
> of scsi_add_host.
> 
> The first version tries to allocates as many buffers as
> shost->can_queue so scsi_setup_command_sense_buffer is called in
> scsi_add_host. But, it's not the case any more, so this calls
> scsi_setup_command_sense_buffer in scsi_host_alloc like
> scsi_setup_command_freelist.
> 
> 
> I did the same tests against this patch (though I knew there were not
> the performnace differences):
> 
> dynamic sense buf (slub) | 483.5 MB/s  IOPS 123772.7/s
> 
> For convenience, here are the previous results:
> 
> scsi-misc (slub)         | 486.9 MB/s  IOPS 124652.9/s
> dynamic sense buf (slub) | 483.2 MB/s  IOPS 123704.1/s
> 
> 
> I put the results and the kernel configuration:
> 
> http://www.kernel.org/pub/linux/kernel/people/tomo/sense/
> 
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] use dynamically allocated sense buffer
> 
> This removes static array sense_buffer in scsi_cmnd and uses
> dynamically allocated sense_buffer (with GFP_DMA).
> 
> The reason for doing this is that some architectures need cacheline
> aligned buffer for DMA:
> 
> http://lkml.org/lkml/2007/11/19/2
> 
> The problems are that scsi_eh_prep_cmnd puts scsi_cmnd::sense_buffer
> to sglist and some LLDs directly DMA to scsi_cmnd::sense_buffer. It's
> necessary to DMA to scsi_cmnd::sense_buffer safely. This patch solves
> these issues.
> 
> __scsi_get_command allocates sense_buffer via kmem_cache_alloc and
> attaches it to a scsi_cmnd so everything just work as before.
> 
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/scsi/hosts.c     |    9 ++++++-
>  drivers/scsi/scsi.c      |   61 ++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/scsi/scsi_priv.h |    2 +
>  include/scsi/scsi_cmnd.h |    2 +-
>  4 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 9a10b43..f5d3fbb 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -268,6 +268,7 @@ static void scsi_host_dev_release(struct device *dev)
>  	}
>  
>  	scsi_destroy_command_freelist(shost);
> +	scsi_destroy_command_sense_buffer(shost);
>  	if (shost->bqt)
>  		blk_free_tags(shost->bqt);
>  
> @@ -372,10 +373,14 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>  	else
>  		shost->dma_boundary = 0xffffffff;
>  
> -	rval = scsi_setup_command_freelist(shost);
> +	rval = scsi_setup_command_sense_buffer(shost);
>  	if (rval)
>  		goto fail_kfree;
>  
> +	rval = scsi_setup_command_freelist(shost);
> +	if (rval)
> +		goto fail_destroy_sense;
> +
>  	device_initialize(&shost->shost_gendev);
>  	snprintf(shost->shost_gendev.bus_id, BUS_ID_SIZE, "host%d",
>  		shost->host_no);
> @@ -399,6 +404,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>  
>   fail_destroy_freelist:
>  	scsi_destroy_command_freelist(shost);
> + fail_destroy_sense:
> +	scsi_destroy_command_sense_buffer(shost);
>   fail_kfree:
>  	kfree(shost);
>  	return NULL;
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 54ff611..0a4a5b8 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -161,6 +161,9 @@ static struct scsi_host_cmd_pool scsi_cmd_dma_pool = {
>  
>  static DEFINE_MUTEX(host_cmd_pool_mutex);
>  
> +static struct kmem_cache *sense_buffer_slab;
> +static int sense_buffer_slab_users;
> +
>  /**
>   * __scsi_get_command - Allocate a struct scsi_cmnd
>   * @shost: host to transmit command
> @@ -172,6 +175,7 @@ static DEFINE_MUTEX(host_cmd_pool_mutex);
>  struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
>  {
>  	struct scsi_cmnd *cmd;
> +	unsigned char *buf;
>  
>  	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
>  			gfp_mask | shost->cmd_pool->gfp_mask);
> @@ -186,6 +190,21 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
>  			list_del_init(&cmd->list);
>  		}
>  		spin_unlock_irqrestore(&shost->free_list_lock, flags);
> +
> +		if (cmd) {
> +			buf = cmd->sense_buffer;
> +			memset(cmd, 0, sizeof(*cmd));
> +			cmd->sense_buffer = buf;
> +		}
> +	} else {
> +		buf = kmem_cache_alloc(sense_buffer_slab, __GFP_DMA|gfp_mask);
> +		if (likely(buf)) {
> +			memset(cmd, 0, sizeof(*cmd));
> +			cmd->sense_buffer = buf;
> +		} else {
> +			kmem_cache_free(shost->cmd_pool->slab, cmd);
> +			cmd = NULL;
> +		}
>  	}
>  
>  	return cmd;
> @@ -212,7 +231,6 @@ struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, gfp_t gfp_mask)
>  	if (likely(cmd != NULL)) {
>  		unsigned long flags;
>  
> -		memset(cmd, 0, sizeof(*cmd));
>  		cmd->device = dev;
>  		init_timer(&cmd->eh_timeout);
>  		INIT_LIST_HEAD(&cmd->list);
> @@ -246,8 +264,10 @@ void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
>  	}
>  	spin_unlock_irqrestore(&shost->free_list_lock, flags);
>  
> -	if (likely(cmd != NULL))
> +	if (likely(cmd != NULL)) {
> +		kmem_cache_free(sense_buffer_slab, cmd->sense_buffer);
>  		kmem_cache_free(shost->cmd_pool->slab, cmd);
> +	}
>  
>  	put_device(dev);
>  }
> @@ -290,6 +310,7 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>  {
>  	struct scsi_host_cmd_pool *pool;
>  	struct scsi_cmnd *cmd;
> +	unsigned char *sense_buffer;
>  
>  	spin_lock_init(&shost->free_list_lock);
>  	INIT_LIST_HEAD(&shost->free_list);
> @@ -319,9 +340,18 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>  			GFP_KERNEL | shost->cmd_pool->gfp_mask);
>  	if (!cmd)
>  		goto fail2;
> +
> +	sense_buffer = kmem_cache_alloc(sense_buffer_slab,
> +					GFP_KERNEL | __GFP_DMA);
> +	if (!sense_buffer)
> +		goto destroy_backup;
> +
> +	cmd->sense_buffer = sense_buffer;
>  	list_add(&cmd->list, &shost->free_list);
>  	return 0;
>  
> +destroy_backup:
> +	kmem_cache_free(shost->cmd_pool->slab, cmd);
>   fail2:
>  	mutex_lock(&host_cmd_pool_mutex);
>  	if (!--pool->users)
> @@ -342,6 +372,7 @@ void scsi_destroy_command_freelist(struct Scsi_Host *shost)
>  
>  		cmd = list_entry(shost->free_list.next, struct scsi_cmnd, list);
>  		list_del_init(&cmd->list);
> +		kmem_cache_free(sense_buffer_slab, cmd->sense_buffer);
>  		kmem_cache_free(shost->cmd_pool->slab, cmd);
>  	}
>  
> @@ -351,6 +382,32 @@ void scsi_destroy_command_freelist(struct Scsi_Host *shost)
>  	mutex_unlock(&host_cmd_pool_mutex);
>  }
>  
> +int scsi_setup_command_sense_buffer(struct Scsi_Host *shost)
> +{
> +	mutex_lock(&host_cmd_pool_mutex);
> +	if (!sense_buffer_slab_users) {
> +		sense_buffer_slab = kmem_cache_create("scsi_sense_buffer",
> +						      SCSI_SENSE_BUFFERSIZE,
> +						      0, SLAB_CACHE_DMA, NULL);
> +		if (!sense_buffer_slab) {
> +			mutex_unlock(&host_cmd_pool_mutex);
> +			return -ENOMEM;
> +		}
> +	}
> +	sense_buffer_slab_users++;
> +	mutex_unlock(&host_cmd_pool_mutex);
> +
> +	return 0;
> +}
> +
> +void scsi_destroy_command_sense_buffer(struct Scsi_Host *shost)
> +{
> +	mutex_lock(&host_cmd_pool_mutex);
> +	if (!--sense_buffer_slab_users)
> +		kmem_cache_destroy(sense_buffer_slab);
> +	mutex_unlock(&host_cmd_pool_mutex);
> +}
> +
>  #ifdef CONFIG_SCSI_LOGGING
>  void scsi_log_send(struct scsi_cmnd *cmd)
>  {
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 3f34e93..55c6f71 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -27,6 +27,8 @@ extern void scsi_exit_hosts(void);
>  extern int scsi_dispatch_cmd(struct scsi_cmnd *cmd);
>  extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
>  extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
> +extern int scsi_setup_command_sense_buffer(struct Scsi_Host *shost);
> +extern void scsi_destroy_command_sense_buffer(struct Scsi_Host *shost);
>  extern void __scsi_done(struct scsi_cmnd *cmd);
>  #ifdef CONFIG_SCSI_LOGGING
>  void scsi_log_send(struct scsi_cmnd *cmd);
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 3f47e52..abd7479 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -88,7 +88,7 @@ struct scsi_cmnd {
>  				   	   working on */
>  
>  #define SCSI_SENSE_BUFFERSIZE 	96
> -	unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
> +	unsigned char *sense_buffer;
>  				/* obtained by REQUEST SENSE when
>  				 * CHECK CONDITION is received on original
>  				 * command (auto-sense) */

Hi
I like this patch better then the option of using a mempools. (See the discussion
in the previous thread). Because here we use the same mechanism set up for commands
and just couple a sense with it's command. If one changes then it will be easier to
keep in sync. (and it is a few cycles less)

Tomo one more nitpicking. If you just call scsi_setup_command_sense_buffer() from
scsi_setup_command_freelist() (Same with free/destroy) then they can be static and
the changes to scsi_priv.h & hosts.c will not be needed. Hence it will not add more
complexity to the error handling in scsi_host_alloc().

I'm still working on an on-demand-sense-allocation, but it is more complicated then
what we need right now for 2.6.25. I recommend to include this patch for the next kernel.

I will RFC my proposal later, once I'm happy with its outcome. And the hacks are down
to none.

Boaz