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

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

[Ming Lei]

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>
---
 include/scsi/scsi_host.h | 67 +++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 822e8cda8d9b..959dc5160f72 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -33,37 +33,12 @@ 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
-	 */
-	const char *(* info)(struct Scsi_Host *);
+	/* Put hot fields together in same cacheline */
 
 	/*
-	 * 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,38 @@ struct scsi_host_template {
 	 */
 	void (*commit_rqs)(struct Scsi_Host *, u16);
 
+	/*
+	 * 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 +475,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 */
-- 
2.25.2

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

[John Garry]

On 21/04/2020 13:49, 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>
> ---
>   include/scsi/scsi_host.h | 67 +++++++++++++++++++++-------------------
>   1 file changed, 35 insertions(+), 32 deletions(-)
> 
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 822e8cda8d9b..959dc5160f72 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -33,37 +33,12 @@ 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
> -	 */
> -	const char *(* info)(struct Scsi_Host *);
> +	/* Put hot fields together in same cacheline */
>   
>   	/*
> -	 * 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);

Should new member .init_cmd_priv be included also in the "hot" group? 
Even if NULL generally, we still have to load that from memory to know 
it (is NULL) in scsi_mq_init_request() and scsi_init_command() [which 
are fastpath, right?]

Cheers,
John


> +	unsigned int cmd_size;
>   
>   	/*
>   	 * The queuecommand function is used to queue up a scsi
> @@ -111,6 +86,38 @@ struct scsi_host_template {
>   	 */
>   	void (*commit_rqs)(struct Scsi_Host *, u16);
>   
> +	/*
> +	 * 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 +475,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 */
> 

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

[Ming Lei]

On Tue, Apr 21, 2020 at 04:16:52PM +0100, John Garry wrote:
> On 21/04/2020 13:49, 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>
> > ---
> >   include/scsi/scsi_host.h | 67 +++++++++++++++++++++-------------------
> >   1 file changed, 35 insertions(+), 32 deletions(-)
> > 
> > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> > index 822e8cda8d9b..959dc5160f72 100644
> > --- a/include/scsi/scsi_host.h
> > +++ b/include/scsi/scsi_host.h
> > @@ -33,37 +33,12 @@ 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
> > -	 */
> > -	const char *(* info)(struct Scsi_Host *);
> > +	/* Put hot fields together in same cacheline */
> >   	/*
> > -	 * 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);
> 
> Should new member .init_cmd_priv be included also in the "hot" group? Even
> if NULL generally, we still have to load that from memory to know it (is
> NULL) in scsi_mq_init_request() and scsi_init_command() [which are fastpath,
> right?]

scsi_mq_init_request() is called via ->init_request(), which is oneshot
initialization.

scsi_init_command() is in fast path, but it is called from
scsi_mq_prep_fn() directly, so it isn't related with ->init_request().


Thanks, 
Ming

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

[Christoph Hellwig]

On Tue, Apr 21, 2020 at 08:49:52PM +0800, 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

Please add comment in the code on why they are grouped as they are.
It probably also makes sense to have them at the very beginning of the
structure.

Otherwise this looks good.