* [PATCH 1/2] consolidate command allocation in a single place @ 2008-03-13 16:53 James Bottomley 2008-03-13 17:09 ` Matthew Wilcox 0 siblings, 1 reply; 4+ messages in thread From: James Bottomley @ 2008-03-13 16:53 UTC (permalink / raw) To: linux-scsi; +Cc: FUJITA Tomonori, Boaz Harrosh 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] consolidate command allocation in a single place 2008-03-13 16:53 [PATCH 1/2] consolidate command allocation in a single place James Bottomley @ 2008-03-13 17:09 ` Matthew Wilcox 2008-03-13 17:15 ` James Bottomley 0 siblings, 1 reply; 4+ messages in thread From: Matthew Wilcox @ 2008-03-13 17:09 UTC (permalink / raw) To: James Bottomley; +Cc: linux-scsi, FUJITA Tomonori, Boaz Harrosh 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." ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] consolidate command allocation in a single place 2008-03-13 17:09 ` Matthew Wilcox @ 2008-03-13 17:15 ` James Bottomley 2008-03-17 18:25 ` James Bottomley 0 siblings, 1 reply; 4+ messages in thread From: James Bottomley @ 2008-03-13 17:15 UTC (permalink / raw) To: Matthew Wilcox; +Cc: linux-scsi, FUJITA Tomonori, Boaz Harrosh 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] consolidate command allocation in a single place 2008-03-13 17:15 ` James Bottomley @ 2008-03-17 18:25 ` James Bottomley 0 siblings, 0 replies; 4+ messages in thread From: James Bottomley @ 2008-03-17 18:25 UTC (permalink / raw) To: Matthew Wilcox; +Cc: linux-scsi, FUJITA Tomonori, Boaz Harrosh 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-03-17 18:25 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-03-13 16:53 [PATCH 1/2] consolidate command allocation in a single place James Bottomley 2008-03-13 17:09 ` Matthew Wilcox 2008-03-13 17:15 ` James Bottomley 2008-03-17 18:25 ` James Bottomley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox