consolidate command allocation in a single place

PATCH In review James Bottomley 2 participants 4 messages Mar 13, 2008 – Mar 17, 2008 SCSI

Share & tools

James Bottomley

author maintainer

2008-03-13 16:53 UTC #0 lore raw

PATCH 1/2 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 <[email protected]>

---
 drivers/scsi/scsi.c |   76 +++++++++++++++++++++++++++++++++------------------
 1 files changed, 49 insertions(+), 27 deletions(-)

drivers/scsi/scsi.c +49 ~~-27~~

index e5c6f6a..d70e608 100644
@@ -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);

Signature · 1 line

1.5.4.1

Matthew Wilcox

2008-03-13 17:09 UTC #1 lore raw

drivers/scsi/scsi.c static struct scsi_host_cmd_pool scsi_cmd_dma_pool = { · line 207

Quoted 10 lines · 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.
drivers/scsi/scsi.c +5 -0
+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').

Signature · 4 lines

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."

James Bottomley

author maintainer

2008-03-13 17:15 UTC #2 lore raw

Quoted 18 lines · Matthew Wilcox

drivers/scsi/scsi.c static struct scsi_host_cmd_pool scsi_cmd_dma_pool = { · line 207
Quoted 10 lines · 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.
drivers/scsi/scsi.c +5 -0
+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)

Quoted 6 lines

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

James Bottomley

author maintainer

2008-03-17 18:25 UTC #3 lore raw

Quoted 21 lines · James Bottomley

Quoted 20 lines · Matthew Wilcox
Review
Quoted 10 lines · 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.
(quoted diff) +5 -0
+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:

Quoted 8 lines

Quoted 4 lines
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 <[email protected]>
Date: Thu, 13 Mar 2008 11:16:33 -0500
Subject: consolidate command allocation in a single place

Signed-off-by James Bottomley <[email protected]>

---
 drivers/scsi/scsi.c |   80 ++++++++++++++++++++++++++++++++-------------------
 1 files changed, 50 insertions(+), 30 deletions(-)

drivers/scsi/scsi.c +50 ~~-30~~

index e5c6f6a..2cf9a62 100644
@@ -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);

Signature · 1 line

1.5.4.3