| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 25 May 2009 12:28:01 +0300
From: Boaz Harrosh <bharrosh@...asas.com>
To: Jens Axboe <jens.axboe@...cle.com>,
FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
CC: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
chris.mason@...cle.com, david@...morbit.com, hch@...radead.org,
akpm@...ux-foundation.org, jack@...e.cz,
yanmin_zhang@...ux.intel.com
Subject: Re: [PATCH 03/13] scsi: unify allocation of scsi command and sense
buffer
On 05/25/2009 10:30 AM, Jens Axboe wrote:
> Fold the sense buffer into the command, thereby eliminating a slab
> allocation and free per command.
>
> Signed-off-by: Jens Axboe <jens.axboe@...cle.com>
Jens Hi.
I'm "TO:" this to Tomo.
This is the way it used to be for a long time. It was only recently changed by
Tomo because of a bug on none-cache-coherent arches that need to dma-access the
sense_buffer and also on the other hand change scsi_cmnd members by CPU.
In my opinion all you need is an __aligned(SMP_CACHE_BYTES) declaration at
sense_buffer[] and let there be a hole at the end before the array. But Tomo
did not like that, so he separated the two.
Ideally there should be a MACRO that is defined to WORD_SIZE on cache-coherent
ARCHs and to SMP_CACHE_BYTES on none-cache-coherent systems and use that size
at the __align() attribute. (So only stupid ARCHES get hurt)
(see below)
> ---
> drivers/scsi/scsi.c | 44 ++++++++++----------------------------------
> include/scsi/scsi_cmnd.h | 12 ++++++------
> 2 files changed, 16 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 166417a..6a993af 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -133,7 +133,6 @@ EXPORT_SYMBOL(scsi_device_type);
>
> struct scsi_host_cmd_pool {
> struct kmem_cache *cmd_slab;
> - struct kmem_cache *sense_slab;
> unsigned int users;
> char *cmd_name;
> char *sense_name;
> @@ -167,20 +166,9 @@ static DEFINE_MUTEX(host_cmd_pool_mutex);
> 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_zalloc(pool->cmd_slab, gfp_mask | pool->gfp_mask);
> - if (!cmd)
> - return NULL;
> + gfp_t gfp = gfp_mask | pool->gfp_mask;
>
> - 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;
> + return kmem_cache_zalloc(pool->cmd_slab, gfp);
> }
>
> /**
> @@ -198,7 +186,6 @@ scsi_pool_free_command(struct scsi_host_cmd_pool *pool,
> if (cmd->prot_sdb)
> kmem_cache_free(scsi_sdb_cache, cmd->prot_sdb);
>
> - kmem_cache_free(pool->sense_slab, cmd->sense_buffer);
> kmem_cache_free(pool->cmd_slab, cmd);
> }
>
> @@ -242,7 +229,6 @@ scsi_host_alloc_command(struct Scsi_Host *shost, gfp_t gfp_mask)
> struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
> {
> struct scsi_cmnd *cmd;
> - unsigned char *buf;
>
> cmd = scsi_host_alloc_command(shost, gfp_mask);
>
> @@ -257,11 +243,8 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
> }
> spin_unlock_irqrestore(&shost->free_list_lock, flags);
>
> - if (cmd) {
> - buf = cmd->sense_buffer;
> + if (cmd)
> memset(cmd, 0, sizeof(*cmd));
> - cmd->sense_buffer = buf;
> - }
> }
>
> return cmd;
> @@ -361,19 +344,13 @@ static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
> pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
> &scsi_cmd_pool;
> if (!pool->users) {
> - pool->cmd_slab = kmem_cache_create(pool->cmd_name,
> - sizeof(struct scsi_cmnd), 0,
> - pool->slab_flags, NULL);
> - if (!pool->cmd_slab)
> - goto fail;
> + unsigned int slab_size;
>
> - 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);
> + slab_size = sizeof(struct scsi_cmnd) + SCSI_SENSE_BUFFERSIZE;
You might as well just define sense array as unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE]
and save the manual calculation.
> + pool->cmd_slab = kmem_cache_create(pool->cmd_name, slab_size,
> + 0, pool->slab_flags, NULL);
> + if (!pool->cmd_slab)
> goto fail;
> - }
> }
>
> pool->users++;
> @@ -397,10 +374,9 @@ static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
> */
> BUG_ON(pool->users == 0);
>
> - if (!--pool->users) {
> + if (!--pool->users)
> kmem_cache_destroy(pool->cmd_slab);
> - kmem_cache_destroy(pool->sense_slab);
> - }
> +
> mutex_unlock(&host_cmd_pool_mutex);
> }
>
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 43b50d3..649ad36 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -102,12 +102,6 @@ struct scsi_cmnd {
> struct request *request; /* The command we are
> working on */
>
> -#define SCSI_SENSE_BUFFERSIZE 96
> - unsigned char *sense_buffer;
> - /* obtained by REQUEST SENSE when
> - * CHECK CONDITION is received on original
> - * command (auto-sense) */
> -
> /* Low-level done function - can be used by low-level driver to point
> * to completion function. Not used by mid/upper level code. */
> void (*scsi_done) (struct scsi_cmnd *);
> @@ -129,6 +123,12 @@ struct scsi_cmnd {
> int result; /* Status code from lower level driver */
>
> unsigned char tag; /* SCSI-II queued command tag */
> +
> +#define SCSI_SENSE_BUFFERSIZE 96
> + unsigned char sense_buffer[0];
+ unsigned char sense_buffer[BUFFERSIZE]; __aligned(CACHE_COHERENT_BYTES)
> + /* obtained by REQUEST SENSE when
> + * CHECK CONDITION is received on original
> + * command (auto-sense) */
> };
>
> extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists