[PATCH 1/2] consolidate command allocation in a single place

[PATCH 1/2] consolidate command allocation in a single place

[James Bottomley]

Since the way we allocate commands with a separate sense buffer is
getting complicated, we should isolate setup and teardown to a single
routine so that if it gets even more complex, there's only one place
in the code that needs to be altered.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/scsi.c |   76 +++++++++++++++++++++++++++++++++------------------
 1 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index e5c6f6a..d70e608 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -166,6 +166,51 @@ static struct scsi_host_cmd_pool scsi_cmd_dma_pool = {
 static DEFINE_MUTEX(host_cmd_pool_mutex);
 
 /**
+ * scsi_get_command_from_pool - internal function to get a fully allocated command
+ * @pool:	slab pool to allocate the command from
+ * @gfp_mask:	mask for the allocation
+ *
+ * Returns a fully allocated command (with the allied sense buffer) or
+ * NULL on failure
+ */
+static struct scsi_cmnd *
+scsi_get_command_from_pool(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
+{
+	struct scsi_cmnd *cmd;
+
+	cmd = kmem_cache_alloc(pool->cmd_slab, gfp_mask | pool->gfp_mask);
+	if (!cmd)
+		return NULL;
+
+	memset(cmd, 0, sizeof(*cmd));
+
+	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;
+}
+
+/**
+ * scsi_put_command_to_pool - internal function to release a command
+ * @pool:	slab pool to allocate the command from
+ * @cmd:	command to release
+ *
+ * the command must previously have been allocated by
+ * scsi_get_command_from_pool.
+ */
+static void
+scsi_put_command_to_pool(struct scsi_host_cmd_pool *pool,
+			 struct scsi_cmnd *cmd)
+{
+	kmem_cache_free(pool->sense_slab, cmd->sense_buffer);
+	kmem_cache_free(pool->cmd_slab, cmd);
+}
+
+/**
  * __scsi_get_command - Allocate a struct scsi_cmnd
  * @shost: host to transmit command
  * @gfp_mask: allocation mask
@@ -178,8 +223,7 @@ 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->cmd_slab,
-			       gfp_mask | shost->cmd_pool->gfp_mask);
+	cmd = scsi_get_command_from_pool(shost->cmd_pool, gfp_mask);
 
 	if (unlikely(!cmd)) {
 		unsigned long flags;
@@ -197,16 +241,6 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 			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;
@@ -266,11 +300,8 @@ 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->sense_slab,
-				cmd->sense_buffer);
-		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
-	}
+	if (likely(cmd != NULL))
+		scsi_put_command_to_pool(shost->cmd_pool, cmd);
 
 	put_device(dev);
 }
@@ -346,23 +377,14 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 	/*
 	 * Get one backup command for this host.
 	 */
-	cmd = kmem_cache_alloc(shost->cmd_pool->cmd_slab,
-			       GFP_KERNEL | shost->cmd_pool->gfp_mask);
+	cmd = scsi_get_command_from_pool(shost->cmd_pool, GFP_KERNEL);
 	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->cmd_slab);
-- 
1.5.4.1

Re: [PATCH 1/2] consolidate command allocation in a single place

[Matthew Wilcox]

On Thu, Mar 13, 2008 at 11:53:11AM -0500, James Bottomley wrote:
> Since the way we allocate commands with a separate sense buffer is
> getting complicated, we should isolate setup and teardown to a single
> routine so that if it gets even more complex, there's only one place
> in the code that needs to be altered.

> +static struct scsi_cmnd *
> +scsi_get_command_from_pool(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
> +static void
> +scsi_put_command_to_pool(struct scsi_host_cmd_pool *pool,
> +			 struct scsi_cmnd *cmd)

The names are a bit clunky ... 'put command to pool'?  How about
alloc/free instead of get/put?  or:

scsi_pool_alloc_command(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
scsi_pool_free_command(struct scsi_host_cmd_pool *pool, struct scsi_cmnd *cmd)

or even:

scsi_command_pool_alloc(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
scsi_command_pool_free(struct scsi_host_cmd_pool *pool, struct scsi_cmnd *cmd)

(i also wouldn't object to use 'cmnd' or 'cmd' in place of 'command').

-- 
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 1/2] consolidate command allocation in a single place

[James Bottomley]

On Thu, 2008-03-13 at 11:09 -0600, Matthew Wilcox wrote:
> On Thu, Mar 13, 2008 at 11:53:11AM -0500, James Bottomley wrote:
> > Since the way we allocate commands with a separate sense buffer is
> > getting complicated, we should isolate setup and teardown to a single
> > routine so that if it gets even more complex, there's only one place
> > in the code that needs to be altered.
> 
> > +static struct scsi_cmnd *
> > +scsi_get_command_from_pool(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
> > +static void
> > +scsi_put_command_to_pool(struct scsi_host_cmd_pool *pool,
> > +			 struct scsi_cmnd *cmd)
> 
> The names are a bit clunky ... 'put command to pool'?  How about
> alloc/free instead of get/put?  or:
> 
> scsi_pool_alloc_command(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
> scsi_pool_free_command(struct scsi_host_cmd_pool *pool, struct scsi_cmnd *cmd)
> 
> or even:
> 
> scsi_command_pool_alloc(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
> scsi_command_pool_free(struct scsi_host_cmd_pool *pool, struct scsi_cmnd *cmd)
> 
> (i also wouldn't object to use 'cmnd' or 'cmd' in place of 'command').

Yes, but it's following the naming convention in the file.  OK, I don't
like it as well, that's why the new functions in the following patch
have alloc/free.

James

Re: [PATCH 1/2] consolidate command allocation in a single place

[James Bottomley]

On Thu, 2008-03-13 at 12:15 -0500, James Bottomley wrote:
> On Thu, 2008-03-13 at 11:09 -0600, Matthew Wilcox wrote:
> > On Thu, Mar 13, 2008 at 11:53:11AM -0500, James Bottomley wrote:
> > > Since the way we allocate commands with a separate sense buffer is
> > > getting complicated, we should isolate setup and teardown to a single
> > > routine so that if it gets even more complex, there's only one place
> > > in the code that needs to be altered.
> > 
> > > +static struct scsi_cmnd *
> > > +scsi_get_command_from_pool(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
> > > +static void
> > > +scsi_put_command_to_pool(struct scsi_host_cmd_pool *pool,
> > > +			 struct scsi_cmnd *cmd)
> > 
> > The names are a bit clunky ... 'put command to pool'?  How about
> > alloc/free instead of get/put?  or:
> > 
> > scsi_pool_alloc_command(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
> > scsi_pool_free_command(struct scsi_host_cmd_pool *pool, struct scsi_cmnd *cmd)
> > 
> > or even:
> > 
> > scsi_command_pool_alloc(struct scsi_host_cmd_pool *pool, gfp_t gfp_mask)
> > scsi_command_pool_free(struct scsi_host_cmd_pool *pool, struct scsi_cmnd *cmd)
> > 
> > (i also wouldn't object to use 'cmnd' or 'cmd' in place of 'command').
> 
> Yes, but it's following the naming convention in the file.  OK, I don't
> like it as well, that's why the new functions in the following patch
> have alloc/free.

Actually, I reconsidered this.  Since I don't use the get/put
terminology in the second patch, it really doesn't make sense to stick
with it in the first one.  The naming convention still seems to be
scsi_<action>_command, so I picked

scsi_pool_alloc_command
scsi_pool_free_command

I also swept up one last place where the kmem_cache_allocs were being
used in the destroy freelist.

James

---

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Thu, 13 Mar 2008 11:16:33 -0500
Subject: consolidate command allocation in a single place

Since the way we allocate commands with a separate sense buffer is
getting complicated, we should isolate setup and teardown to a single
routine so that if it gets even more complex, there's only one place
in the code that needs to be altered.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/scsi.c |   80 ++++++++++++++++++++++++++++++++-------------------
 1 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index e5c6f6a..2cf9a62 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -166,6 +166,51 @@ static struct scsi_host_cmd_pool scsi_cmd_dma_pool = {
 static DEFINE_MUTEX(host_cmd_pool_mutex);
 
 /**
+ * scsi_pool_alloc_command - internal function to get a fully allocated command
+ * @pool:	slab pool to allocate the command from
+ * @gfp_mask:	mask for the allocation
+ *
+ * Returns a fully allocated command (with the allied sense buffer) or
+ * NULL on failure
+ */
+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_alloc(pool->cmd_slab, gfp_mask | pool->gfp_mask);
+	if (!cmd)
+		return NULL;
+
+	memset(cmd, 0, sizeof(*cmd));
+
+	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;
+}
+
+/**
+ * scsi_pool_free_command - internal function to release a command
+ * @pool:	slab pool to allocate the command from
+ * @cmd:	command to release
+ *
+ * the command must previously have been allocated by
+ * scsi_pool_alloc_command.
+ */
+static void
+scsi_pool_free_command(struct scsi_host_cmd_pool *pool,
+			 struct scsi_cmnd *cmd)
+{
+	kmem_cache_free(pool->sense_slab, cmd->sense_buffer);
+	kmem_cache_free(pool->cmd_slab, cmd);
+}
+
+/**
  * __scsi_get_command - Allocate a struct scsi_cmnd
  * @shost: host to transmit command
  * @gfp_mask: allocation mask
@@ -178,8 +223,7 @@ 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->cmd_slab,
-			       gfp_mask | shost->cmd_pool->gfp_mask);
+	cmd = scsi_pool_alloc_command(shost->cmd_pool, gfp_mask);
 
 	if (unlikely(!cmd)) {
 		unsigned long flags;
@@ -197,16 +241,6 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 			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;
@@ -266,11 +300,8 @@ 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->sense_slab,
-				cmd->sense_buffer);
-		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
-	}
+	if (likely(cmd != NULL))
+		scsi_pool_free_command(shost->cmd_pool, cmd);
 
 	put_device(dev);
 }
@@ -346,23 +377,14 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 	/*
 	 * Get one backup command for this host.
 	 */
-	cmd = kmem_cache_alloc(shost->cmd_pool->cmd_slab,
-			       GFP_KERNEL | shost->cmd_pool->gfp_mask);
+	cmd = scsi_pool_alloc_command(shost->cmd_pool, GFP_KERNEL);
 	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->cmd_slab);
@@ -384,9 +406,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(shost->cmd_pool->sense_slab,
-				cmd->sense_buffer);
-		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
+		scsi_pool_free_command(shost->cmd_pool, cmd);
 	}
 
 	mutex_lock(&host_cmd_pool_mutex);
-- 
1.5.4.3