[PATCH] scsi: core: Micro-optimize the hot path

[PATCH] scsi: core: Micro-optimize the hot path

[Bart Van Assche]

Every code path that inserts scmd->eh_entry into a list uses
list_add_tail() (see also scsi_abort_command() and scsi_eh_scmd_add()).
list_add_tail() overwrites the next and prev pointers. Hence, the only
code that depends on the initialization of scmd->eh_entry is the
BUG_ON(!list_empty(&scmd->eh_entry)) statement in scsi_abort_command().
Remove this statement because scsi_timeout() already prevents
scsi_abort_command() from being called twice for the same command by
setting SCMD_STATE_COMPLETE. Remove the scmd->eh_entry initialization
because the only statement that relied on this initialization has been
removed.

Cc: John Garry <john.garry@linux.dev>
Cc: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_error.c | 1 -
 drivers/scsi/scsi_lib.c   | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 2e0b1909dc69..75442d60c30e 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -270,7 +270,6 @@ scsi_abort_command(struct scsi_cmnd *scmd)
 	spin_lock_irqsave(&shost->host_lock, flags);
 	if (shost->eh_deadline != -1 && !shost->last_reset)
 		shost->last_reset = jiffies;
-	BUG_ON(!list_empty(&scmd->eh_entry));
 	list_add_tail(&scmd->eh_entry, &shost->eh_abort_list);
 	spin_unlock_irqrestore(&shost->host_lock, flags);
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index db64470f5229..f695a6605815 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1322,7 +1322,6 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 	}
 
 	cmd->device = dev;
-	INIT_LIST_HEAD(&cmd->eh_entry);
 	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
 }
 
@@ -1590,8 +1589,6 @@ static void scsi_complete(struct request *rq)
 		return;
 	}
 
-	INIT_LIST_HEAD(&cmd->eh_entry);
-
 	atomic_inc(&cmd->device->iodone_cnt);
 	if (cmd->result)
 		atomic_inc(&cmd->device->ioerr_cnt);

Re: [PATCH] scsi: core: Micro-optimize the hot path

[John Garry]

On 9/25/26 19:09, Bart Van Assche wrote:

nit: I think that the subject can be more specific, like "don't 
unnecessarily init scmd->eh_entry"

> Every code path that inserts scmd->eh_entry into a list uses
> list_add_tail() (see also scsi_abort_command() and scsi_eh_scmd_add()).
> list_add_tail() overwrites the next and prev pointers. Hence, the only
> code that depends on the initialization of scmd->eh_entry is the
> BUG_ON(!list_empty(&scmd->eh_entry)) statement in scsi_abort_command().
> Remove this statement because scsi_timeout() already prevents
> scsi_abort_command() from being called twice for the same command by
> setting SCMD_STATE_COMPLETE. Remove the scmd->eh_entry initialization
> because the only statement that relied on this initialization has been
> removed.
> 
> Cc: John Garry <john.garry@linux.dev>
> Cc: Hannes Reinecke <hare@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Seems fine, so:
Reviewed-by: John Garry <john.garry@linux.dev>

> ---
>   drivers/scsi/scsi_error.c | 1 -
>   drivers/scsi/scsi_lib.c   | 3 ---
>   2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 2e0b1909dc69..75442d60c30e 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -270,7 +270,6 @@ scsi_abort_command(struct scsi_cmnd *scmd)
>   	spin_lock_irqsave(&shost->host_lock, flags);
>   	if (shost->eh_deadline != -1 && !shost->last_reset)
>   		shost->last_reset = jiffies;
> -	BUG_ON(!list_empty(&scmd->eh_entry));
>   	list_add_tail(&scmd->eh_entry, &shost->eh_abort_list);
>   	spin_unlock_irqrestore(&shost->host_lock, flags);
>   
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index db64470f5229..f695a6605815 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1322,7 +1322,6 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
>   	}
>   
>   	cmd->device = dev;
> -	INIT_LIST_HEAD(&cmd->eh_entry);
>   	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
>   }
>   
> @@ -1590,8 +1589,6 @@ static void scsi_complete(struct request *rq)
>   		return;
>   	}
>   
> -	INIT_LIST_HEAD(&cmd->eh_entry);
> -
>   	atomic_inc(&cmd->device->iodone_cnt);
>   	if (cmd->result)
>   		atomic_inc(&cmd->device->ioerr_cnt);

Re: [PATCH] scsi: core: Micro-optimize the hot path

[Hannes Reinecke]

On 9/25/26 8:09 PM, Bart Van Assche wrote:
> Every code path that inserts scmd->eh_entry into a list uses
> list_add_tail() (see also scsi_abort_command() and scsi_eh_scmd_add()).
> list_add_tail() overwrites the next and prev pointers. Hence, the only
> code that depends on the initialization of scmd->eh_entry is the
> BUG_ON(!list_empty(&scmd->eh_entry)) statement in scsi_abort_command().
> Remove this statement because scsi_timeout() already prevents
> scsi_abort_command() from being called twice for the same command by
> setting SCMD_STATE_COMPLETE. Remove the scmd->eh_entry initialization
> because the only statement that relied on this initialization has been
> removed.
> 
> Cc: John Garry <john.garry@linux.dev>
> Cc: Hannes Reinecke <hare@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_error.c | 1 -
>   drivers/scsi/scsi_lib.c   | 3 ---
>   2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 2e0b1909dc69..75442d60c30e 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -270,7 +270,6 @@ scsi_abort_command(struct scsi_cmnd *scmd)
>   	spin_lock_irqsave(&shost->host_lock, flags);
>   	if (shost->eh_deadline != -1 && !shost->last_reset)
>   		shost->last_reset = jiffies;
> -	BUG_ON(!list_empty(&scmd->eh_entry));
>   	list_add_tail(&scmd->eh_entry, &shost->eh_abort_list);
>   	spin_unlock_irqrestore(&shost->host_lock, flags);
>   
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index db64470f5229..f695a6605815 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1322,7 +1322,6 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
>   	}
>   
>   	cmd->device = dev;
> -	INIT_LIST_HEAD(&cmd->eh_entry);
>   	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
>   }
>   
> @@ -1590,8 +1589,6 @@ static void scsi_complete(struct request *rq)
>   		return;
>   	}
>   
> -	INIT_LIST_HEAD(&cmd->eh_entry);
> -
>   	atomic_inc(&cmd->device->iodone_cnt);
>   	if (cmd->result)
>   		atomic_inc(&cmd->device->ioerr_cnt);

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] scsi: core: Micro-optimize the hot path

[Martin K. Petersen (Oracle)]

Bart,

> Every code path that inserts scmd->eh_entry into a list uses
> list_add_tail() (see also scsi_abort_command() and scsi_eh_scmd_add()).
> list_add_tail() overwrites the next and prev pointers. Hence, the only
> code that depends on the initialization of scmd->eh_entry is the
> BUG_ON(!list_empty(&scmd->eh_entry)) statement in scsi_abort_command().

Applied to 7.4/scsi-staging, thanks!

-- 
Martin K. Petersen