[PATCH v3] use dynamically allocated sense buffer

[PATCH v3] use dynamically allocated sense buffer

[FUJITA Tomonori]

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) */
-- 
1.5.3.4

Re: [PATCH v3] use dynamically allocated sense buffer

[Boaz Harrosh]

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

Re: [PATCH v3] use dynamically allocated sense buffer

[James Bottomley]

On Wed, 2008-01-16 at 13:32 +0900, FUJITA Tomonori wrote:
> This is the third version of:
> 
> http://marc.info/?l=linux-scsi&m=120038907123706&w=2
[...]
> 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
> @@ -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);

This is going to cause the enterprise some angst because ZONE_DMA can be
very small, and the enterprise requrements for commands in flight very
large.  I also think its unnecessary if we know the host isn't requiring
ISA DMA.  How about the below to fix this, it's based on the existing
infrastructure for solving the very same problem.

James

---

>From e7ffbd81684779f5eb41d66d5f499b82af40e12b Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Sun, 20 Jan 2008 09:28:24 -0600
Subject: [SCSI] don't use __GFP_DMA for sense buffers if not required

Only hosts which actually have ISA DMA requirements need sense buffers
coming out of ZONE_DMA, so only use the __GFP_DMA flag for that case
to avoid allocating this scarce resource if it's not necessary.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/hosts.c     |    9 +----
 drivers/scsi/scsi.c      |  106 +++++++++++++++++++--------------------------
 drivers/scsi/scsi_priv.h |    2 -
 3 files changed, 46 insertions(+), 71 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index f5d3fbb..9a10b43 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -268,7 +268,6 @@ 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);
 
@@ -373,13 +372,9 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	else
 		shost->dma_boundary = 0xffffffff;
 
-	rval = scsi_setup_command_sense_buffer(shost);
-	if (rval)
-		goto fail_kfree;
-
 	rval = scsi_setup_command_freelist(shost);
 	if (rval)
-		goto fail_destroy_sense;
+		goto fail_kfree;
 
 	device_initialize(&shost->shost_gendev);
 	snprintf(shost->shost_gendev.bus_id, BUS_ID_SIZE, "host%d",
@@ -404,8 +399,6 @@ 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 0a4a5b8..045e69d 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -141,29 +141,30 @@ const char * scsi_device_type(unsigned type)
 EXPORT_SYMBOL(scsi_device_type);
 
 struct scsi_host_cmd_pool {
-	struct kmem_cache	*slab;
-	unsigned int	users;
-	char		*name;
-	unsigned int	slab_flags;
-	gfp_t		gfp_mask;
+	struct kmem_cache	*cmd_slab;
+	struct kmem_cache	*sense_slab;
+	unsigned int		users;
+	char			*cmd_name;
+	char			*sense_name;
+	unsigned int		slab_flags;
+	gfp_t			gfp_mask;
 };
 
 static struct scsi_host_cmd_pool scsi_cmd_pool = {
-	.name		= "scsi_cmd_cache",
+	.cmd_name	= "scsi_cmd_cache",
+	.sense_name	= "scsi_sense_cache",
 	.slab_flags	= SLAB_HWCACHE_ALIGN,
 };
 
 static struct scsi_host_cmd_pool scsi_cmd_dma_pool = {
-	.name		= "scsi_cmd_cache(DMA)",
+	.cmd_name	= "scsi_cmd_cache(DMA)",
+	.sense_name	= "scsi_sense_cache(DMA)",
 	.slab_flags	= SLAB_HWCACHE_ALIGN|SLAB_CACHE_DMA,
 	.gfp_mask	= __GFP_DMA,
 };
 
 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
@@ -177,8 +178,8 @@ 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);
+	cmd = kmem_cache_alloc(shost->cmd_pool->cmd_slab,
+			       gfp_mask | shost->cmd_pool->gfp_mask);
 
 	if (unlikely(!cmd)) {
 		unsigned long flags;
@@ -197,12 +198,13 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 			cmd->sense_buffer = buf;
 		}
 	} else {
-		buf = kmem_cache_alloc(sense_buffer_slab, __GFP_DMA|gfp_mask);
+		buf = kmem_cache_alloc(shost->cmd_pool->sense_slab,
+				       gfp_mask | shost->cmd_pool->gfp_mask);
 		if (likely(buf)) {
 			memset(cmd, 0, sizeof(*cmd));
 			cmd->sense_buffer = buf;
 		} else {
-			kmem_cache_free(shost->cmd_pool->slab, cmd);
+			kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
 			cmd = NULL;
 		}
 	}
@@ -265,8 +267,9 @@ void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
 	spin_unlock_irqrestore(&shost->free_list_lock, flags);
 
 	if (likely(cmd != NULL)) {
-		kmem_cache_free(sense_buffer_slab, cmd->sense_buffer);
-		kmem_cache_free(shost->cmd_pool->slab, cmd);
+		kmem_cache_free(shost->cmd_pool->sense_slab,
+				cmd->sense_buffer);
+		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
 	}
 
 	put_device(dev);
@@ -310,7 +313,6 @@ 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);
@@ -322,10 +324,13 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 	mutex_lock(&host_cmd_pool_mutex);
 	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
 	if (!pool->users) {
-		pool->slab = kmem_cache_create(pool->name,
-				sizeof(struct scsi_cmnd), 0,
-				pool->slab_flags, NULL);
-		if (!pool->slab)
+		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
+						   sizeof(struct scsi_cmnd), 0,
+						   pool->slab_flags, NULL);
+		pool->sense_slab = kmem_cache_create(pool->sense_name,
+						     SCSI_SENSE_BUFFERSIZE, 0,
+						     pool->slab_flags, NULL);
+		if (!pool->cmd_slab || !pool->sense_slab)
 			goto fail;
 	}
 
@@ -336,26 +341,28 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 	/*
 	 * Get one backup command for this host.
 	 */
-	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
-			GFP_KERNEL | shost->cmd_pool->gfp_mask);
+	cmd = kmem_cache_alloc(shost->cmd_pool->cmd_slab,
+			       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 = kmem_cache_alloc(shost->cmd_pool->sense_slab,
+					     GFP_KERNEL |
+					     shost->cmd_pool->gfp_mask);
+	if (!cmd->sense_buffer)
+		goto fail2;
 
-	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:
+	if (cmd)
+		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
 	mutex_lock(&host_cmd_pool_mutex);
-	if (!--pool->users)
-		kmem_cache_destroy(pool->slab);
+	if (!--pool->users) {
+		kmem_cache_destroy(pool->cmd_slab);
+		kmem_cache_destroy(pool->sense_slab);
+	}
  fail:
 	mutex_unlock(&host_cmd_pool_mutex);
 	return -ENOMEM;
@@ -372,39 +379,16 @@ 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);
+		kmem_cache_free(shost->cmd_pool->sense_slab,
+				cmd->sense_buffer);
+		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
 	}
 
 	mutex_lock(&host_cmd_pool_mutex);
-	if (!--shost->cmd_pool->users)
-		kmem_cache_destroy(shost->cmd_pool->slab);
-	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;
-		}
+	if (!--shost->cmd_pool->users) {
+		kmem_cache_destroy(shost->cmd_pool->cmd_slab);
+		kmem_cache_destroy(shost->cmd_pool->sense_slab);
 	}
-	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);
 }
 
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 55c6f71..3f34e93 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -27,8 +27,6 @@ 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);
-- 
1.5.3.7

Re: [PATCH v3] use dynamically allocated sense buffer

[Matthew Wilcox]

On Wed, Jan 16, 2008 at 01:32:17PM +0900, FUJITA Tomonori wrote:
> 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.

I think this is fine for the moment.

Longer-term, I want to allow low-level drivers to allocate the
sense_buffer themselves so they can DMA directly into it (ie grown-up dma
mapping, rather than this quaint x86 __GFP_DMA).  This patch doesn't get
us any closer to that, but it doesn't get us further away from it either.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH v3] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Sun, 20 Jan 2008 10:36:56 -0600
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> 
> On Wed, 2008-01-16 at 13:32 +0900, FUJITA Tomonori wrote:
> > This is the third version of:
> > 
> > http://marc.info/?l=linux-scsi&m=120038907123706&w=2
> [...]
> > 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
> > @@ -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);
> 
> This is going to cause the enterprise some angst because ZONE_DMA can be
> very small, and the enterprise requrements for commands in flight very
> large.  I also think its unnecessary if we know the host isn't requiring
> ISA DMA.

Yes, I should have done properly.


>  How about the below to fix this, it's based on the existing
> infrastructure for solving the very same problem.

Looks nice. Integrating sense_buffer_pool into struct
scsi_host_cmd_pool looks much better.


> James
> 
> ---
> 
> >From e7ffbd81684779f5eb41d66d5f499b82af40e12b Mon Sep 17 00:00:00 2001
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date: Sun, 20 Jan 2008 09:28:24 -0600
> Subject: [SCSI] don't use __GFP_DMA for sense buffers if not required
> 
> Only hosts which actually have ISA DMA requirements need sense buffers
> coming out of ZONE_DMA, so only use the __GFP_DMA flag for that case
> to avoid allocating this scarce resource if it's not necessary.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/scsi/hosts.c     |    9 +----
>  drivers/scsi/scsi.c      |  106 +++++++++++++++++++--------------------------
>  drivers/scsi/scsi_priv.h |    2 -
>  3 files changed, 46 insertions(+), 71 deletions(-)
> 

(snip)

> @@ -310,7 +313,6 @@ 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);
> @@ -322,10 +324,13 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>  	mutex_lock(&host_cmd_pool_mutex);
>  	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
>  	if (!pool->users) {
> -		pool->slab = kmem_cache_create(pool->name,
> -				sizeof(struct scsi_cmnd), 0,
> -				pool->slab_flags, NULL);
> -		if (!pool->slab)
> +		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
> +						   sizeof(struct scsi_cmnd), 0,
> +						   pool->slab_flags, NULL);
> +		pool->sense_slab = kmem_cache_create(pool->sense_name,
> +						     SCSI_SENSE_BUFFERSIZE, 0,
> +						     pool->slab_flags, NULL);
> +		if (!pool->cmd_slab || !pool->sense_slab)
>  			goto fail;


If one of the above allocations fails, the allocated slab leaks?


diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 045e69d..1a9fba6 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -327,11 +327,16 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
 						   sizeof(struct scsi_cmnd), 0,
 						   pool->slab_flags, NULL);
+		if (!pool->cmd_slab)
+			goto fail;
+
 		pool->sense_slab = kmem_cache_create(pool->sense_name,
 						     SCSI_SENSE_BUFFERSIZE, 0,
 						     pool->slab_flags, NULL);
-		if (!pool->cmd_slab || !pool->sense_slab)
+		if (!pool->sense_slab) {
+			kmem_cache_destroy(pool->cmd_slab);
 			goto fail;
+		}
 	}
 
 	pool->users++;

Re: [PATCH v3] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Sun, 20 Jan 2008 09:40:11 -0700
Matthew Wilcox <matthew@wil.cx> wrote:

> On Wed, Jan 16, 2008 at 01:32:17PM +0900, FUJITA Tomonori wrote:
> > 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.
> 
> I think this is fine for the moment.
> 
> Longer-term, I want to allow low-level drivers to allocate the
> sense_buffer themselves so they can DMA directly into it (ie grown-up dma
> mapping, rather than this quaint x86 __GFP_DMA).  This patch doesn't get

Yeah, I think that the approach is one of candidates.

If we go with it, I think that the major issue is that LLDs don't know
when they can reclaim sense_buffer from scsi-ml; scsi-ml uses
sense_buffer after scmd->scsi_done.


> us any closer to that, but it doesn't get us further away from it either.

Re: [PATCH v3] use dynamically allocated sense buffer

[Matthew Wilcox]

On Mon, Jan 21, 2008 at 01:08:58PM +0900, FUJITA Tomonori wrote:
> On Sun, 20 Jan 2008 09:40:11 -0700
> Matthew Wilcox <matthew@wil.cx> wrote:
> > Longer-term, I want to allow low-level drivers to allocate the
> > sense_buffer themselves so they can DMA directly into it (ie grown-up dma
> > mapping, rather than this quaint x86 __GFP_DMA).  This patch doesn't get
> 
> Yeah, I think that the approach is one of candidates.
> 
> If we go with it, I think that the major issue is that LLDs don't know
> when they can reclaim sense_buffer from scsi-ml; scsi-ml uses
> sense_buffer after scmd->scsi_done.

The midlayer would call a function in the scsi_host_template to free the
command.  The sense_buffer would be freed at the same time.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH v3] use dynamically allocated sense buffer

[FUJITA Tomonori]

On Sun, 20 Jan 2008 21:37:41 -0700
Matthew Wilcox <matthew@wil.cx> wrote:

> On Mon, Jan 21, 2008 at 01:08:58PM +0900, FUJITA Tomonori wrote:
> > On Sun, 20 Jan 2008 09:40:11 -0700
> > Matthew Wilcox <matthew@wil.cx> wrote:
> > > Longer-term, I want to allow low-level drivers to allocate the
> > > sense_buffer themselves so they can DMA directly into it (ie grown-up dma
> > > mapping, rather than this quaint x86 __GFP_DMA).  This patch doesn't get
> > 
> > Yeah, I think that the approach is one of candidates.
> > 
> > If we go with it, I think that the major issue is that LLDs don't know
> > when they can reclaim sense_buffer from scsi-ml; scsi-ml uses
> > sense_buffer after scmd->scsi_done.
> 
> The midlayer would call a function in the scsi_host_template to free the
> command.  The sense_buffer would be freed at the same time.

Yeah, it would work though I'm a bit concerned about adding another
phase to the scsi_cmnd interaction between the midlayer and LLDs.

Another possible option is that putting some sense_buffer information
to the host template and the midlayer allocates sense buffers for
LLDs. LLDs can ask for them when it's necessary. One disadvantage of
this is the many LLDs have sense buffer in their own data structure so
it's not fit well for these LLDs, I think.

Re: [PATCH v3] use dynamically allocated sense buffer

[Boaz Harrosh]

[-- Attachment #1: Type: text/plain, Size: 4464 bytes --]

On Mon, Jan 21 2008 at 5:59 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> On Sun, 20 Jan 2008 10:36:56 -0600
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
>> On Wed, 2008-01-16 at 13:32 +0900, FUJITA Tomonori wrote:
>>> This is the third version of:
>>>
>>> http://marc.info/?l=linux-scsi&m=120038907123706&w=2
>> [...]
>>> 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
>>> @@ -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);
>> This is going to cause the enterprise some angst because ZONE_DMA can be
>> very small, and the enterprise requrements for commands in flight very
>> large.  I also think its unnecessary if we know the host isn't requiring
>> ISA DMA.
> 
> Yes, I should have done properly.
> 
> 
>>  How about the below to fix this, it's based on the existing
>> infrastructure for solving the very same problem.
> 
> Looks nice. Integrating sense_buffer_pool into struct
> scsi_host_cmd_pool looks much better.
> 
> 
>> James
>>
>> ---
>>
>> >From e7ffbd81684779f5eb41d66d5f499b82af40e12b Mon Sep 17 00:00:00 2001
>> From: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Date: Sun, 20 Jan 2008 09:28:24 -0600
>> Subject: [SCSI] don't use __GFP_DMA for sense buffers if not required
>>
>> Only hosts which actually have ISA DMA requirements need sense buffers
>> coming out of ZONE_DMA, so only use the __GFP_DMA flag for that case
>> to avoid allocating this scarce resource if it's not necessary.
>>
>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> ---
>>  drivers/scsi/hosts.c     |    9 +----
>>  drivers/scsi/scsi.c      |  106 +++++++++++++++++++--------------------------
>>  drivers/scsi/scsi_priv.h |    2 -
>>  3 files changed, 46 insertions(+), 71 deletions(-)
>>
> 
> (snip)
> 
>> @@ -310,7 +313,6 @@ 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);
>> @@ -322,10 +324,13 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>>  	mutex_lock(&host_cmd_pool_mutex);
>>  	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
>>  	if (!pool->users) {
>> -		pool->slab = kmem_cache_create(pool->name,
>> -				sizeof(struct scsi_cmnd), 0,
>> -				pool->slab_flags, NULL);
>> -		if (!pool->slab)
>> +		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
>> +						   sizeof(struct scsi_cmnd), 0,
>> +						   pool->slab_flags, NULL);
>> +		pool->sense_slab = kmem_cache_create(pool->sense_name,
>> +						     SCSI_SENSE_BUFFERSIZE, 0,
>> +						     pool->slab_flags, NULL);
>> +		if (!pool->cmd_slab || !pool->sense_slab)
>>  			goto fail;
> 
> 
> If one of the above allocations fails, the allocated slab leaks?
> 
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 045e69d..1a9fba6 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -327,11 +327,16 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>  		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
>  						   sizeof(struct scsi_cmnd), 0,
>  						   pool->slab_flags, NULL);
> +		if (!pool->cmd_slab)
> +			goto fail;
> +
>  		pool->sense_slab = kmem_cache_create(pool->sense_name,
>  						     SCSI_SENSE_BUFFERSIZE, 0,
>  						     pool->slab_flags, NULL);
> -		if (!pool->cmd_slab || !pool->sense_slab)
> +		if (!pool->sense_slab) {
> +			kmem_cache_destroy(pool->cmd_slab);
>  			goto fail;
> +		}
>  	}
>  
>  	pool->users++;
> -
James Tomo Hi.

Has it been decided if this work will be accepted for 2.6.25 merge window?
If so will it be through the first stage scsi-misc or the second stage scsi-bidi?

And lastly will it be in the squashed form, attached.
The reason I ask is because I want to rebase some work on top of that.

Just my $0.02, I think it should go even into the backport versions of 2.6.24.x
after proper testing.

Thanks
Boaz


[-- Attachment #2: 0001-use-dynamically-allocated-sense-buffer.patch --]
[-- Type: text/plain, Size: 6739 bytes --]

>From 047640aa77747a6ab49b3b38866a8146cf7fcbf4 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Wed, 23 Jan 2008 19:56:04 +0200
Subject: [PATCH] use dynamically allocated sense buffer

  This removes static array sense_buffer in scsi_cmnd and uses
  dynamically allocated DMA able sense_buffer.

  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>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi.c      |   92 ++++++++++++++++++++++++++++++++++-----------
 include/scsi/scsi_cmnd.h |    2 +-
 2 files changed, 70 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 834d329..b35d194 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -141,20 +141,24 @@ const char * scsi_device_type(unsigned type)
 EXPORT_SYMBOL(scsi_device_type);
 
 struct scsi_host_cmd_pool {
-	struct kmem_cache	*slab;
-	unsigned int	users;
-	char		*name;
-	unsigned int	slab_flags;
-	gfp_t		gfp_mask;
+	struct kmem_cache	*cmd_slab;
+	struct kmem_cache	*sense_slab;
+	unsigned int		users;
+	char			*cmd_name;
+	char			*sense_name;
+	unsigned int		slab_flags;
+	gfp_t			gfp_mask;
 };
 
 static struct scsi_host_cmd_pool scsi_cmd_pool = {
-	.name		= "scsi_cmd_cache",
+	.cmd_name	= "scsi_cmd_cache",
+	.sense_name	= "scsi_sense_cache",
 	.slab_flags	= SLAB_HWCACHE_ALIGN,
 };
 
 static struct scsi_host_cmd_pool scsi_cmd_dma_pool = {
-	.name		= "scsi_cmd_cache(DMA)",
+	.cmd_name	= "scsi_cmd_cache(DMA)",
+	.sense_name	= "scsi_sense_cache(DMA)",
 	.slab_flags	= SLAB_HWCACHE_ALIGN|SLAB_CACHE_DMA,
 	.gfp_mask	= __GFP_DMA,
 };
@@ -172,9 +176,10 @@ 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);
+	cmd = kmem_cache_alloc(shost->cmd_pool->cmd_slab,
+			       gfp_mask | shost->cmd_pool->gfp_mask);
 
 	if (unlikely(!cmd)) {
 		unsigned long flags;
@@ -186,6 +191,22 @@ 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(shost->cmd_pool->sense_slab,
+				       gfp_mask | shost->cmd_pool->gfp_mask);
+		if (likely(buf)) {
+			memset(cmd, 0, sizeof(*cmd));
+			cmd->sense_buffer = buf;
+		} else {
+			kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
+			cmd = NULL;
+		}
 	}
 
 	return cmd;
@@ -212,7 +233,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 +266,11 @@ void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
 	}
 	spin_unlock_irqrestore(&shost->free_list_lock, flags);
 
-	if (likely(cmd != NULL))
-		kmem_cache_free(shost->cmd_pool->slab, cmd);
+	if (likely(cmd != NULL)) {
+		kmem_cache_free(shost->cmd_pool->sense_slab,
+				cmd->sense_buffer);
+		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
+	}
 
 	put_device(dev);
 }
@@ -301,11 +324,19 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 	mutex_lock(&host_cmd_pool_mutex);
 	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
 	if (!pool->users) {
-		pool->slab = kmem_cache_create(pool->name,
-				sizeof(struct scsi_cmnd), 0,
-				pool->slab_flags, NULL);
-		if (!pool->slab)
+		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
+						   sizeof(struct scsi_cmnd), 0,
+						   pool->slab_flags, NULL);
+		if (!pool->cmd_slab)
+			goto fail;
+
+		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);
 			goto fail;
+		}
 	}
 
 	pool->users++;
@@ -315,17 +346,28 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 	/*
 	 * Get one backup command for this host.
 	 */
-	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
-			GFP_KERNEL | shost->cmd_pool->gfp_mask);
+	cmd = kmem_cache_alloc(shost->cmd_pool->cmd_slab,
+			       GFP_KERNEL | shost->cmd_pool->gfp_mask);
 	if (!cmd)
 		goto fail2;
+
+	cmd->sense_buffer = kmem_cache_alloc(shost->cmd_pool->sense_slab,
+					     GFP_KERNEL |
+					     shost->cmd_pool->gfp_mask);
+	if (!cmd->sense_buffer)
+		goto fail2;
+
 	list_add(&cmd->list, &shost->free_list);
 	return 0;
 
  fail2:
+	if (cmd)
+		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
 	mutex_lock(&host_cmd_pool_mutex);
-	if (!--pool->users)
-		kmem_cache_destroy(pool->slab);
+	if (!--pool->users) {
+		kmem_cache_destroy(pool->cmd_slab);
+		kmem_cache_destroy(pool->sense_slab);
+	}
  fail:
 	mutex_unlock(&host_cmd_pool_mutex);
 	return -ENOMEM;
@@ -342,12 +384,16 @@ 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(shost->cmd_pool->slab, cmd);
+		kmem_cache_free(shost->cmd_pool->sense_slab,
+				cmd->sense_buffer);
+		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
 	}
 
 	mutex_lock(&host_cmd_pool_mutex);
-	if (!--shost->cmd_pool->users)
-		kmem_cache_destroy(shost->cmd_pool->slab);
+	if (!--shost->cmd_pool->users) {
+		kmem_cache_destroy(shost->cmd_pool->cmd_slab);
+		kmem_cache_destroy(shost->cmd_pool->sense_slab);
+	}
 	mutex_unlock(&host_cmd_pool_mutex);
 }
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 9811666..de28aab 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -84,7 +84,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) */
-- 
1.5.3.3