| Message ID | 20200421124952.297448-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: put hot fields of scsi_host_template into one cacheline | expand |
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 */ >
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
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.
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 */
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(-)