| Message ID | 20170629201443.GD2393@linux.vnet.ibm.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello, On Thu, Jun 29, 2017 at 01:14:43PM -0700, Paul E. McKenney wrote: > On Thu, Jun 29, 2017 at 03:53:22PM -0400, Tejun Heo wrote: > > Hello, Paul. > > > > On Thu, Jun 29, 2017 at 11:10:57AM -0700, Paul E. McKenney wrote: > > > If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait() > > > must have executed before CPU 1's spin_lock(). However, even on x86, > > > CPU 0's prior writes can be reordered with its subsequent reads, which > > > means that r1 == 0 is possible, which means that the above condition > > > could hold, even on x86. > > > > I see. Ah, that's a mind bender. > > It has indeed been providing at least its share of entertainment over > the past little while. ;-) lol :) > > That part of the code should be dead now. I don't think we no longer > > have any driver which doesn't have error handler set. I should rip > > out that if/else. Also, ACQUIRE semantics should be enough there. > > Nothing changes from the EH side there. > > It looks like we actually might get rid of spin_unlock_wait entirely. > But how about if I just pull the spin_lock_irqsave() before the "if" > and the spin_lock_irqrestore() after the "if"? Same effect, only > difference is that the "if" and the "ap->eh_tries = ATA_EH_MAX_TRIES" > end up under the lock, and I bet that you won't be able to measure > the difference. (Please see below.) > > I will do this because I just now happened to be editing that file on > my "eradicate spin_unlock_wait()" quest, but can easily rework the > patch as desired. If you want something different, just let me know! Sounds good to me. That path isn't hot at all. No change made at this level is gonna have any actual impact. Please go for whatever is the simplest. For moving out the lock/unlock outside if/else, Acked-by: Tejun Heo <tj@kernel.org> Thanks.
On Thu, Jun 29, 2017 at 04:17:54PM -0400, Tejun Heo wrote: > Hello, > > On Thu, Jun 29, 2017 at 01:14:43PM -0700, Paul E. McKenney wrote: > > On Thu, Jun 29, 2017 at 03:53:22PM -0400, Tejun Heo wrote: > > > Hello, Paul. > > > > > > On Thu, Jun 29, 2017 at 11:10:57AM -0700, Paul E. McKenney wrote: > > > > If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait() > > > > must have executed before CPU 1's spin_lock(). However, even on x86, > > > > CPU 0's prior writes can be reordered with its subsequent reads, which > > > > means that r1 == 0 is possible, which means that the above condition > > > > could hold, even on x86. > > > > > > I see. Ah, that's a mind bender. > > > > It has indeed been providing at least its share of entertainment over > > the past little while. ;-) > > lol :) > > > > That part of the code should be dead now. I don't think we no longer > > > have any driver which doesn't have error handler set. I should rip > > > out that if/else. Also, ACQUIRE semantics should be enough there. > > > Nothing changes from the EH side there. > > > > It looks like we actually might get rid of spin_unlock_wait entirely. > > But how about if I just pull the spin_lock_irqsave() before the "if" > > and the spin_lock_irqrestore() after the "if"? Same effect, only > > difference is that the "if" and the "ap->eh_tries = ATA_EH_MAX_TRIES" > > end up under the lock, and I bet that you won't be able to measure > > the difference. (Please see below.) > > > > I will do this because I just now happened to be editing that file on > > my "eradicate spin_unlock_wait()" quest, but can easily rework the > > patch as desired. If you want something different, just let me know! > > Sounds good to me. That path isn't hot at all. No change made at > this level is gonna have any actual impact. Please go for whatever is > the simplest. For moving out the lock/unlock outside if/else, > > Acked-by: Tejun Heo <tj@kernel.org> Applied, and thank you! Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index ef68232b5222..779f6f18c1f4 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -645,12 +645,11 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, * completions are honored. A scmd is determined to have * timed out iff its associated qc is active and not failed. */ + spin_lock_irqsave(ap->lock, flags); if (ap->ops->error_handler) { struct scsi_cmnd *scmd, *tmp; int nr_timedout = 0; - spin_lock_irqsave(ap->lock, flags); - /* This must occur under the ap->lock as we don't want a polled recovery to race the real interrupt handler @@ -700,12 +699,11 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, if (nr_timedout) __ata_port_freeze(ap); - spin_unlock_irqrestore(ap->lock, flags); /* initialize eh_tries */ ap->eh_tries = ATA_EH_MAX_TRIES; - } else - spin_unlock_wait(ap->lock); + } + spin_unlock_irqrestore(ap->lock, flags); } EXPORT_SYMBOL(ata_scsi_cmd_error_handler);