On 3/9/23 04:13, Benjamin Block wrote:
On Tue, Mar 07, 2023 at 01:51:51PM -0800, Bart Van Assche wrote:scsi_dec_host_busy() is called from the hot path and hence must not obtain the host lock if no commands have failed. scsi_dec_host_busy() tests three different variables of which at least two are set if a command failed. Commit 3bd6f43f5cb3 ("scsi: core: Ensure that the SCSI error handler gets woken up") introduced a call_rcu() call to ensure that all tasks observe the host state change before the host_failed change. Simplify the approach for guaranteeing that the host state and host_failed/host_eh_scheduled changes are observed in order by using smp_store_release() to update host_failed or host_eh_scheduled after having update the host state and smp_load_acquire() before reading the host state.It's probably just me, but "simplify" is a bit of a misnomer when you replace RCU by plain memory barriers. And I'm kind of wondering what we improve here? It seems to me that at least as far as the hot path is concerned, nothing really changes? The situation for `scsi_eh_scmd_add()` seems to improve, but that is already way off the hot path.
Hi Benjamin, The advantages of the approach introduced by this patch are as follows: * The size of struct scsi_cmnd is reduced. This may improve performance by reducing the number of cache misses. * One call_rcu() call is eliminated. This reduces the error handler wake-up latency.
if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0 || scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0) { - shost->host_eh_scheduled++; + smp_store_release(&shost->host_eh_scheduled, + shost->host_eh_scheduled + 1);Probably should be documented.
I will add a comment above each new store release / load acquire operation. Thanks, Bart.
