Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <08d85db7-679b-b099-485b-744b8bb22839@suse.de>
Date:   Tue, 4 Oct 2022 07:55:44 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH v2 6/6] scsi: mvsas: Use sas_task_find_rq() for tagging
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, jinpu.wang@cloud.ionos.com,
        damien.lemoal@wdc.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxarm@huawei.com, ipylypiv@google.com, changyuanl@google.com,
        hch@lst.de, yanaijie@huawei.com
References: <1664528184-162714-1-git-send-email-john.garry@huawei.com>
 <1664528184-162714-7-git-send-email-john.garry@huawei.com>
From:   Hannes Reinecke <hare@suse.de>
In-Reply-To: <1664528184-162714-7-git-send-email-john.garry@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 9/30/22 10:56, John Garry wrote:
> The request associated with a scsi command coming from the block layer
> has a unique tag, so use that when possible for getting a slot.
> 
> Unfortunately we don't support reserved commands in the SCSI midlayer yet.
> As such, SMP tasks - as an example - will not have a request associated, so
> in the interim continue to manage those tags for that type of sas_task
> internally.
> 
> We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
> decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
> MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
> ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
> but what those 2 slots are used for is not obvious.
> 
> Also make the tag management functions static, where possible.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/mvsas/mv_defs.h |  1 +
>   drivers/scsi/mvsas/mv_init.c |  9 +++++----
>   drivers/scsi/mvsas/mv_sas.c  | 38 ++++++++++++++++++++++++------------
>   drivers/scsi/mvsas/mv_sas.h  |  7 +------
>   4 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
> index 7123a2efbf58..8ef174cd4d37 100644
> --- a/drivers/scsi/mvsas/mv_defs.h
> +++ b/drivers/scsi/mvsas/mv_defs.h
> @@ -40,6 +40,7 @@ enum driver_configuration {
>   	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
>   	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
>   	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
> +	MVS_RSVD_SLOTS		= 4,
>   	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
>   };
>   
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index c85fb812ad43..cfe84473a515 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -142,7 +142,7 @@ static void mvs_free(struct mvs_info *mvi)
>   		scsi_host_put(mvi->shost);
>   	list_for_each_entry(mwq, &mvi->wq_list, entry)
>   		cancel_delayed_work(&mwq->work_q);
> -	kfree(mvi->tags);
> +	kfree(mvi->rsvd_tags);
>   	kfree(mvi);
>   }
>   
> @@ -284,7 +284,6 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
>   			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
>   			goto err_out;
>   	}
> -	mvi->tags_num = slot_nr;
>   
>   	return 0;
>   err_out:
> @@ -367,8 +366,8 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
>   	mvi->sas = sha;
>   	mvi->shost = shost;
>   
> -	mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
> -	if (!mvi->tags)
> +	mvi->rsvd_tags = bitmap_zalloc(MVS_RSVD_SLOTS, GFP_KERNEL);
> +	if (!mvi->rsvd_tags)
>   		goto err_out;
>   
>   	if (MVS_CHIP_DISP->chip_ioremap(mvi))
> @@ -469,6 +468,8 @@ static void  mvs_post_sas_ha_init(struct Scsi_Host *shost,
>   	else
>   		can_queue = MVS_CHIP_SLOT_SZ;
>   
> +	can_queue -= MVS_RSVD_SLOTS;
> +
>   	shost->sg_tablesize = min_t(u16, SG_ALL, MVS_MAX_SG);
>   	shost->can_queue = can_queue;
>   	mvi->shost->cmd_per_lun = MVS_QUEUE_SIZE;
> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
> index 0810e6c930e1..00b3a2781d21 100644
> --- a/drivers/scsi/mvsas/mv_sas.c
> +++ b/drivers/scsi/mvsas/mv_sas.c
> @@ -20,33 +20,39 @@ static int mvs_find_tag(struct mvs_info *mvi, struct sas_task *task, u32 *tag)
>   	return 0;
>   }
>   
> -void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
> +static void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
>   {
> -	void *bitmap = mvi->tags;
> +	void *bitmap = mvi->rsvd_tags;
>   	clear_bit(tag, bitmap);
>   }
>   
> -void mvs_tag_free(struct mvs_info *mvi, u32 tag)
> +static void mvs_tag_free(struct mvs_info *mvi, u32 tag)
>   {
> +	if (tag < mvi->shost->can_queue)
> +		return;
> +
> +	tag -= mvi->shost->can_queue;
> +
>   	mvs_tag_clear(mvi, tag);
>   }
>   
> -void mvs_tag_set(struct mvs_info *mvi, unsigned int tag)
> +static void mvs_tag_set(struct mvs_info *mvi, unsigned int tag)
>   {
> -	void *bitmap = mvi->tags;
> +	void *bitmap = mvi->rsvd_tags;
>   	set_bit(tag, bitmap);
>   }
>   
> -inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
> +static int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
>   {
>   	unsigned int index, tag;
> -	void *bitmap = mvi->tags;
> +	void *bitmap = mvi->rsvd_tags;
>   
> -	index = find_first_zero_bit(bitmap, mvi->tags_num);
> +	index = find_first_zero_bit(bitmap, MVS_RSVD_SLOTS);
>   	tag = index;
> -	if (tag >= mvi->tags_num)
> +	if (tag >= MVS_RSVD_SLOTS)
>   		return -SAS_QUEUE_FULL;
>   	mvs_tag_set(mvi, tag);
> +	tag += mvi->shost->can_queue;
>   	*tag_out = tag;
>   	return 0;
>   }

Also here: please move the reserved tags to the front, such that the tag 
allocation scheme matches with the blk-mq tag allocation scheme.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman