scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

From: Nicholas Bellinger <[email protected]>

This patch converts scsi_dispatch_cmd() to only hold struct Scsi_Host->host_lock for scsi_cmd_get_serial(), and drops the lock because the call into the LLD with host->hostt->queuecommand().

Signed-off-by Nicholas A. Bellinger <[email protected]>

---
 drivers/scsi/scsi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/scsi/scsi.c +0 -0

index ad0ed21..06e5b3a 100644

Quoted 11 lines · Nicholas A. Bellinger

From: Nicholas Bellinger <[email protected]>

This patch converts scsi_dispatch_cmd() to only hold struct Scsi_Host->host_lock for scsi_cmd_get_serial(), and drops the lock because the call into the LLD with host->hostt->queuecommand().

Signed-off-by Nicholas A. Bellinger <[email protected]>

---
 drivers/scsi/scsi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/scsi/scsi.c int scsi_dispatch_cmd(struct scsi_cmnd *cmd) · line 748

Quoted 9 lines

drivers/scsi/scsi.c +1 -0

index ad0ed21..06e5b3a 100644
@@ -745,6 +745,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 	 * TODO: kill serial or move to blk layer
 	 */
 	scsi_cmd_get_serial(host, cmd); 
+	spin_unlock_irqrestore(host->host_lock, flags);

So at least from where I stand, my object is to reduce the number of times we take and release the lock, which this doesn't do. As I said
before: we need to figure out the rest, which likely includes an atomic for the serial number (which is almost unused). I think the check against SHOST_DEL is fine unlocked.

So at least from where I stand, my object is to reduce the number of times we take and release the lock, which this doesn't do. As I said
before: we need to figure out the rest, which likely includes an atomic for the serial number (which is almost unused). I think the check against SHOST_DEL is fine unlocked.

So at least from where I stand, my object is to reduce the number of times we take and release the lock, which this doesn't do. As I said
before: we need to figure out the rest, which likely includes an atomic for the serial number (which is almost unused). I think the check

Quoted 4 lines

So at least from where I stand, my object is to reduce the number of times we take and release the lock, which this doesn't do. As I said
before: we need to figure out the rest, which likely includes an atomic for the serial number (which is almost unused). I think the check

If it's unused it should be removed, make optional.
Atomics are a scalability problem too and not much cheaper than spinlocks.

I don't disagree with the idea of removing it, especially as it has so few users, but replacing the host lock with an atomic here would still vastly reduce the contention, which is the initial complaint. The

contention occurs because the host lock is so widely used for other things. The way to reduce that contention is firstly to reduce the length of code covered by the lock and also reduce the actual number of places where the lock is taken. Compared with host lock's current vast footprint, and atomic here is tiny.

Quoted 3 lines

I don't disagree with the idea of removing it, especially as it has so few users, but replacing the host lock with an atomic here would still vastly reduce the contention, which is the initial complaint. The

Actually the complaint is the overhead of the spin lock. This can be either caused
by contention or by cache line bounce time.

Quoted 5 lines

contention occurs because the host lock is so widely used for other things. The way to reduce that contention is firstly to reduce the length of code covered by the lock and also reduce the actual number of places where the lock is taken. Compared with host lock's current vast footprint, and atomic here is tiny.

That assumes that it's contention that is the problem and not simply bounce time.

Quoted 7 lines · Andi Kleen

Quoted 3 lines

I don't disagree with the idea of removing it, especially as it has so few users, but replacing the host lock with an atomic here would still vastly reduce the contention, which is the initial complaint. The

Actually the complaint is the overhead of the spin lock. This can be either caused
by contention or by cache line bounce time.

The original complaint was contention. My desire is to reduce the locked path coverage, so I saw an opportunity.

What I was actually thinking of for the atomic is that we'd let it range [1..INT_MAX] so a zero was an indicator of no use of this. Then the actual code could become

if (atomic_read(x)) {
	do {
		y = atomic_add_return(1, x);
	} while (y == 0);
}

So "fast" cards not using the serial number set a zero there (we'd default initialise to one), the line is shared so no bouncing (because it's never updated). This should satisfy everyone.

Quoted 8 lines

Quoted 5 lines

contention occurs because the host lock is so widely used for other things. The way to reduce that contention is firstly to reduce the length of code covered by the lock and also reduce the actual number of places where the lock is taken. Compared with host lock's current vast footprint, and atomic here is tiny.

That assumes that it's contention that is the problem and not simply bounce time.

That's what the patch and data that started this whole thread showed, yes ... but I think actual bounce in the spinlock is also a problem ... we just don't have data to show it.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 16 lines · James Bottomley

Quoted 7 lines · Andi Kleen

Quoted 3 lines

I don't disagree with the idea of removing it, especially as it has so few users, but replacing the host lock with an atomic here would still vastly reduce the contention, which is the initial complaint. The

Actually the complaint is the overhead of the spin lock. This can be either caused
by contention or by cache line bounce time.

The original complaint was contention. My desire is to reduce the locked path coverage, so I saw an opportunity.

What I was actually thinking of for the atomic is that we'd let it range [1..INT_MAX] so a zero was an indicator of no use of this. Then the actual code could become

Quoted 5 lines

if (atomic_read(x)) {
	do {
		y = atomic_add_return(1, x);
	} while (y == 0);
}

The conversion of struct scsi_cmnd->serial_number to atomic_t and the above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.

Quoted 11 lines

Quoted 10 lines

What I was actually thinking of for the atomic is that we'd let it range [1..INT_MAX] so a zero was an indicator of no use of this. Then the actual code could become

if (atomic_read(x)) {
	do {
		y = atomic_add_return(1, x);
	} while (y == 0);
}

Quoted 3 lines

The conversion of struct scsi_cmnd->serial_number to atomic_t and the above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.

Actually, that should be the conversion of struct
Scsi_Host->cmd_serial_number to an atomic_t. AFAICT there is no reason
for struct scsi_cmnd->serial_number needing to be an atomic_t.

Quoted 11 lines · Nicholas A. Bellinger

Quoted 11 lines

Quoted 10 lines

What I was actually thinking of for the atomic is that we'd let it range [1..INT_MAX] so a zero was an indicator of no use of this. Then the actual code could become

if (atomic_read(x)) {
	do {
		y = atomic_add_return(1, x);
	} while (y == 0);
}

Quoted 7 lines

Quoted 3 lines

The conversion of struct scsi_cmnd->serial_number to atomic_t and the above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.

Actually, that should be the conversion of struct
Scsi_Host->cmd_serial_number to an atomic_t. AFAICT there is no reason
for struct scsi_cmnd->serial_number needing to be an atomic_t.

Just want to verify the hidden assumption we have here when the atomic int Scsi_Host->cmd_serial_number counter overflow after increment. The counter itself then becomes negative. We are assuming that when we do type conversion back to unsigned long scsi_cmnd->serial_number, we will get the right thing.

So for 32-bit int, we expect if we start with 0x7fffffff in hex and the expected sequence will be

	2147483647 (int) -> 2147483647 (unsigned long)  [0x7fffffff]
	+1
	-2147483648 (int) -> 2147483648 (unsigned long) [0x80000000]
	+1
	-2147483647 (int) -> 2147483649 (unsigned long) [0x80000001]

If there is architecture where the above assumption is not true (which I'm not aware of but just checking), then we should manually wrap the atomic counter to 1 when counter overflow.

Quoted 16 lines · James Bottomley

Quoted 7 lines · Andi Kleen

Quoted 3 lines

I don't disagree with the idea of removing it, especially as it has so few users, but replacing the host lock with an atomic here would still vastly reduce the contention, which is the initial complaint. The

Actually the complaint is the overhead of the spin lock. This can be either caused
by contention or by cache line bounce time.

The original complaint was contention. My desire is to reduce the locked path coverage, so I saw an opportunity.

What I was actually thinking of for the atomic is that we'd let it range [1..INT_MAX] so a zero was an indicator of no use of this. Then the actual code could become

Quoted 5 lines

if (atomic_read(x)) {
	do {
		y = atomic_add_return(1, x);
	} while (y == 0);
}

The conversion of struct scsi_cmnd->serial_number to atomic_t and the above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.

I will take a look at this conversion and respin a complete set of patches for review a bit later today.

Thanks!

--nab

Quoted 25 lines

So "fast" cards not using the serial number set a zero there (we'd default initialise to one), the line is shared so no bouncing (because it's never updated). This should satisfy everyone.

Quoted 8 lines

Quoted 5 lines

contention occurs because the host lock is so widely used for other things. The way to reduce that contention is firstly to reduce the length of code covered by the lock and also reduce the actual number of places where the lock is taken. Compared with host lock's current vast footprint, and atomic here is tiny.

That assumes that it's contention that is the problem and not simply bounce time.

That's what the patch and data that started this whole thread showed, yes ... but I think actual bounce in the spinlock is also a problem ... we just don't have data to show it.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html