Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1678035

spin_unlock_wait() in ata_scsi_cmd_error_handler()?

Show all headers | View raw

Hello, Tejun!

We are having some discussion about the semantics of spin_unlock_wait(),
and your code has one of them.
(https://marc.info/?l=linux-kernel&m=149730349001044)

We seem to agree that spin_unlock_wait() should provide acquire semantics.
Consider the following admittedly bizarre code fragment:

	CPU 0			CPU 1
	-----			-----
	spin_unlock_wait(&ml);	/* Lock held initially. */
	WRITE_ONCE(x, 1);	r2 = READ_ONCE(x);
	r1 = READ_ONCE(y);	WRITE_ONCE(y, 1);
				spin_unlock(&ml);

	r1 == 0 || r2 == 1 /* again, evaluated "at the end of time" */

CPU 0's spin_unlock_wait() must wait for CPU 1 to release the lock,
which means that CPU 0's memory references must see the result of
CPU 1's memory references and not vice versa.  In other words, the
expression beneath the code fragment cannot hold.

The current sense is that spin_unlock_wait() will -not- provide
release semantics.  This calls for an even more bizarre code fragment:

	CPU 0			CPU 1
	-----			-----
	WRITE_ONCE(x, 1);	spin_lock(&ml);
	r1 = READ_ONCE(y);	r2 = READ_ONCE(x);
	spin_unlock_wait(&ml);	WRITE_ONCE(y, 1);
	WRITE_ONCE(z, 1);	/* Intentionally not releasing lock! */

	z == 1 && (r1 == 1 || r2 == 0) /* evaluated "at the end of time" */

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.

One of the uses of spin_unlock_wait() is in ata_scsi_cmd_error_handler()
in the file drivers/ata/libata-eh.c.  Your commit ad9e27624479b
("libata-eh-fw: update ata_scsi_error() for new EH") last touched it,
though it predates that commit.

My question to you is whether the code in ata_scsi_cmd_error_handler()
needs release semantics.  If it does, my recommendation is to replace
the spin_unlock_wait(ap->lock) with this (adding the needed curly braces,
of course):

	spin_lock(ap->lock);
	spin_unlock(ap->lock);

If the code only needs acquire semantics, no change required.

If your code requires release semantics, and there is some reason why
my suggested replacement above is a bad idea, please let me know!

							Thanx, Paul

Back to linux.kernel | Previous | Next — Next in thread | Find similar | Unroll thread

Thread

spin_unlock_wait() in ata_scsi_cmd_error_handler()? "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-29 20:20 +0200
  Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()? Tejun Heo <tj@kernel.org> - 2017-06-29 22:00 +0200
    Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()? "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-29 22:20 +0200
      Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()? Tejun Heo <tj@kernel.org> - 2017-06-29 22:20 +0200
        Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()? "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-06-29 22:50 +0200

csiph-web