Re: [PATCH V2] scsi: put hot fields of scsi_host_template into one cacheline

[John Garry <john.garry@huawei.com>]

On 22/04/2020 10:54, Ming Lei wrote:
> The following three fields of scsi_host_template are referenced in
> scsi IO submission path, so put them together into one cacheline:
> 
> - cmd_size
> - queuecommand
> - commit_rqs
> 
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: John Garry <john.garry@huawei.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

FWIW,
Reviewed-by: John Garry <john.garry@huawei.com>

> ---
> V2:
> 	- move the 3 fields at the beginning of scsi_host_template
> 	- comment why we do this way
> 
>   include/scsi/scsi_host.h | 72 ++++++++++++++++++++++------------------
>   1 file changed, 39 insertions(+), 33 deletions(-)
> 
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 822e8cda8d9b..eae5b9f52dfe 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -30,40 +30,15 @@ struct scsi_transport_template;
>   #define MODE_TARGET 0x02
>   
>   struct scsi_host_template {
> -	struct module *module;
> -	const char *name;
> -
>   	/*
> -	 * The info function will return whatever useful information the
> -	 * developer sees fit.  If not provided, then the name field will
> -	 * be used instead.
> -	 *
> -	 * Status: OPTIONAL
> +	 * Put fields referenced in IO submission path together in
> +	 * same cacheline
>   	 */
> -	const char *(* info)(struct Scsi_Host *);
>   
>   	/*
> -	 * Ioctl interface
> -	 *
> -	 * Status: OPTIONAL
> -	 */
> -	int (*ioctl)(struct scsi_device *dev, unsigned int cmd,
> -		     void __user *arg);
> -
> -
> -#ifdef CONFIG_COMPAT
> -	/*
> -	 * Compat handler. Handle 32bit ABI.
> -	 * When unknown ioctl is passed return -ENOIOCTLCMD.
> -	 *
> -	 * Status: OPTIONAL
> +	 * Additional per-command data allocated for the driver.
>   	 */
> -	int (*compat_ioctl)(struct scsi_device *dev, unsigned int cmd,
> -			    void __user *arg);
> -#endif
> -
> -	int (*init_cmd_priv)(struct Scsi_Host *shost, struct scsi_cmnd *cmd);
> -	int (*exit_cmd_priv)(struct Scsi_Host *shost, struct scsi_cmnd *cmd);
> +	unsigned int cmd_size;
>   
>   	/*
>   	 * The queuecommand function is used to queue up a scsi
> @@ -111,6 +86,41 @@ struct scsi_host_template {
>   	 */
>   	void (*commit_rqs)(struct Scsi_Host *, u16);
>   
> +	struct module *module;
> +	const char *name;
> +
> +	/*
> +	 * The info function will return whatever useful information the
> +	 * developer sees fit.  If not provided, then the name field will
> +	 * be used instead.
> +	 *
> +	 * Status: OPTIONAL
> +	 */
> +	const char *(* info)(struct Scsi_Host *);
> +
> +	/*
> +	 * Ioctl interface
> +	 *
> +	 * Status: OPTIONAL
> +	 */
> +	int (*ioctl)(struct scsi_device *dev, unsigned int cmd,
> +		     void __user *arg);
> +
> +
> +#ifdef CONFIG_COMPAT
> +	/*
> +	 * Compat handler. Handle 32bit ABI.
> +	 * When unknown ioctl is passed return -ENOIOCTLCMD.
> +	 *
> +	 * Status: OPTIONAL
> +	 */
> +	int (*compat_ioctl)(struct scsi_device *dev, unsigned int cmd,
> +			    void __user *arg);
> +#endif
> +
> +	int (*init_cmd_priv)(struct Scsi_Host *shost, struct scsi_cmnd *cmd);
> +	int (*exit_cmd_priv)(struct Scsi_Host *shost, struct scsi_cmnd *cmd);
> +
>   	/*
>   	 * This is an error handling strategy routine.  You don't need to
>   	 * define one of these if you don't want to - there is a default
> @@ -468,10 +478,6 @@ struct scsi_host_template {
>   	 */
>   	u64 vendor_id;
>   
> -	/*
> -	 * Additional per-command data allocated for the driver.
> -	 */
> -	unsigned int cmd_size;
>   	struct scsi_host_cmd_pool *cmd_pool;
>   
>   	/* Delay for runtime autosuspend */
>