hpsa: remove unneeded loop

From: Tomas Henzl <[email protected]>

When the driver calls scsi_done and after that frees it's internal preallocated memory it can happen that a new job is enqueud before the memory is freed. The allocation fails and the message "cmd_alloc returned NULL" is shown.
Patch below fixes it by moving cmd->scsi_done after cmd_free.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/hpsa.c +2 -2

index d7df01e..48fa81e 100644
@@ -1180,8 +1180,8 @@ static void complete_scsi_command(struct CommandList *cp)
 	scsi_set_resid(cmd, ei->ResidualCnt);
 
 	if (ei->CommandStatus == 0) {
-		cmd->scsi_done(cmd);
 		cmd_free(h, cp);
+		cmd->scsi_done(cmd);
 		return;
 	}
 

@@ -1353,8 +1353,8 @@ static void complete_scsi_command(struct CommandList *cp)
 		dev_warn(&h->pdev->dev, "cp %p returned unknown status %x\n",
 				cp, ei->CommandStatus);
 	}
-	cmd->scsi_done(cmd);
 	cmd_free(h, cp);
+	cmd->scsi_done(cmd);
 }
 
 static void hpsa_pci_unmap(struct pci_dev *pdev,

Signature · 1 line

1.8.3.1

From: Tomas Henzl <[email protected]>

Remove unneeded variables.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 2 --
 drivers/scsi/hpsa.h | 2 --
 2 files changed, 4 deletions(-)

drivers/scsi/hpsa.c +0 -2

index 48fa81e..e0f6b00 100644
@@ -2668,7 +2668,6 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 		return NULL;
 	}
 	set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG));
-	h->nr_allocs++;
 	spin_unlock_irqrestore(&h->lock, flags);
 
 	c = h->cmd_pool + i;

@@ -2740,7 +2739,6 @@ static void cmd_free(struct ctlr_info *h, struct CommandList *c)
 	spin_lock_irqsave(&h->lock, flags);
 	clear_bit(i & (BITS_PER_LONG - 1),
 		  h->cmd_pool_bits + (i / BITS_PER_LONG));
-	h->nr_frees++;
 	spin_unlock_irqrestore(&h->lock, flags);
 }
 

drivers/scsi/hpsa.h +0 -2

index 9816479..bc85e72 100644
@@ -98,8 +98,6 @@ struct ctlr_info {
 	struct ErrorInfo 	*errinfo_pool;
 	dma_addr_t		errinfo_pool_dhandle;
 	unsigned long  		*cmd_pool_bits;
-	int			nr_allocs;
-	int			nr_frees;
 	int			scan_finished;
 	spinlock_t		scan_lock;
 	wait_queue_head_t	scan_wait_queue;

Signature · 1 line

1.8.3.1

From: Tomas Henzl <[email protected]>

The cmd_pool_bits is protected everywhere with a spinlock, we don't need the test_and_set_bit, set_bit is enough and the loop can be removed too.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/scsi/hpsa.c +6 -9

index 796482b..d7df01e 100644
@@ -2662,15 +2662,12 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	unsigned long flags;
 
 	spin_lock_irqsave(&h->lock, flags);
-	do {
-		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds) {
-			spin_unlock_irqrestore(&h->lock, flags);
-			return NULL;
-		}
-	} while (test_and_set_bit
-		 (i & (BITS_PER_LONG - 1),
-		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
+	i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
+	if (i == h->nr_cmds) {
+		spin_unlock_irqrestore(&h->lock, flags);
+		return NULL;
+	}
+	set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG));
 	h->nr_allocs++;
 	spin_unlock_irqrestore(&h->lock, flags);
 

Signature · 1 line

1.8.3.1

Quoted 40 lines · Tomas Henzl

From: Tomas Henzl <[email protected]>

The cmd_pool_bits is protected everywhere with a spinlock, we don't need the test_and_set_bit, set_bit is enough and the loop can be removed too.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/scsi/hpsa.c +6 -9

index 796482b..d7df01e 100644
@@ -2662,15 +2662,12 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	unsigned long flags;
 
 	spin_lock_irqsave(&h->lock, flags);
-	do {
-		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds) {
-			spin_unlock_irqrestore(&h->lock, flags);
-			return NULL;
-		}
-	} while (test_and_set_bit
-		 (i & (BITS_PER_LONG - 1),
-		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
+	i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
+	if (i == h->nr_cmds) {
+		spin_unlock_irqrestore(&h->lock, flags);
+		return NULL;
+	}
+	set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG));
 	h->nr_allocs++;
 	spin_unlock_irqrestore(&h->lock, flags);
 

Signature · 1 line

1.8.3.1

Would it be better instead to just not use the spinlock for protecting cmd_pool_bits? I have thought about doing this for awhile, but haven't gotten around to it.

I think the while loop is safe without the spin lock. And then it is not needed in cmd_free either.

-- steve

--
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 45 lines · On 08/01/2013 03:39 PM, [email protected] wrote:

Quoted 40 lines · Tomas Henzl

From: Tomas Henzl <[email protected]>

The cmd_pool_bits is protected everywhere with a spinlock, we don't need the test_and_set_bit, set_bit is enough and the loop can be removed too.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/scsi/hpsa.c +6 -9

index 796482b..d7df01e 100644
@@ -2662,15 +2662,12 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	unsigned long flags;
 
 	spin_lock_irqsave(&h->lock, flags);
-	do {
-		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds) {
-			spin_unlock_irqrestore(&h->lock, flags);
-			return NULL;
-		}
-	} while (test_and_set_bit
-		 (i & (BITS_PER_LONG - 1),
-		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
+	i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
+	if (i == h->nr_cmds) {
+		spin_unlock_irqrestore(&h->lock, flags);
+		return NULL;
+	}
+	set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG));
 	h->nr_allocs++;
 	spin_unlock_irqrestore(&h->lock, flags);
 

Signature · 1 line

1.8.3.1

Would it be better instead to just not use the spinlock for protecting cmd_pool_bits? I have thought about doing this for awhile, but haven't gotten around to it.

Quoted 2 lines

I think the while loop is safe without the spin lock. And then it is not needed in cmd_free either.

I was evaluating the same idea for a while too, a loop and inside just the test_and_set_bit, maybe even a stored value to start with a likely empty bit from last time to tune it a bit. But I know almost nothing about the use pattern, so I decided for the least invasive change to the existing code, to not make it worse.

Quoted 9 lines

-- steve

--
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 46 lines · Tomas Henzl

Quoted 45 lines · On 08/01/2013 03:39 PM, [email protected] wrote:

Quoted 40 lines · Tomas Henzl

From: Tomas Henzl <[email protected]>

The cmd_pool_bits is protected everywhere with a spinlock, we don't need the test_and_set_bit, set_bit is enough and the loop can be removed too.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/scsi/hpsa.c +6 -9

index 796482b..d7df01e 100644
@@ -2662,15 +2662,12 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	unsigned long flags;
 
 	spin_lock_irqsave(&h->lock, flags);
-	do {
-		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds) {
-			spin_unlock_irqrestore(&h->lock, flags);
-			return NULL;
-		}
-	} while (test_and_set_bit
-		 (i & (BITS_PER_LONG - 1),
-		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
+	i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
+	if (i == h->nr_cmds) {
+		spin_unlock_irqrestore(&h->lock, flags);
+		return NULL;
+	}
+	set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG));
 	h->nr_allocs++;
 	spin_unlock_irqrestore(&h->lock, flags);
 

Signature · 1 line

1.8.3.1

Would it be better instead to just not use the spinlock for protecting cmd_pool_bits? I have thought about doing this for awhile, but haven't gotten around to it.

Quoted 6 lines

Quoted 2 lines

I think the while loop is safe without the spin lock. And then it is not needed in cmd_free either.

I was evaluating the same idea for a while too, a loop and inside just the test_and_set_bit, maybe even a stored value to start with a likely empty bit from last time to tune it a bit. But I know almost nothing about the use pattern, so I decided for the least invasive change to the existing code, to not make it worse.

Only reason I haven't done it is I'm loathe to make such a change to the main i/o path without testing it like crazy before unleashing it, and it's never been a convenient time to slide such a change in around here and get proper testing done (and there are other rather large changes brewing).

However, we have been using a similar scheme with the SCSI over PCIe driver,
here: https://github.com/HPSmartStorage/scsi-over-pcie/blob/master/block/sop.c
in alloc_request() around line 1476 without problems, and nvme-core.c contains similar code in alloc_cmdid(), so I am confident it's sound in principle. I would want to beat on it though, in case it ends up exposing a firmware bug or something (not that I think it will, but you never know.)

-- steve

Quoted 7 lines

Quoted 7 lines

-- steve

--
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 58 lines · On 08/01/2013 04:21 PM, [email protected] wrote:

Quoted 46 lines · Tomas Henzl

Quoted 45 lines · On 08/01/2013 03:39 PM, [email protected] wrote:

Quoted 40 lines · Tomas Henzl

From: Tomas Henzl <[email protected]>

The cmd_pool_bits is protected everywhere with a spinlock, we don't need the test_and_set_bit, set_bit is enough and the loop can be removed too.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/scsi/hpsa.c +6 -9

index 796482b..d7df01e 100644
@@ -2662,15 +2662,12 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	unsigned long flags;
 
 	spin_lock_irqsave(&h->lock, flags);
-	do {
-		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds) {
-			spin_unlock_irqrestore(&h->lock, flags);
-			return NULL;
-		}
-	} while (test_and_set_bit
-		 (i & (BITS_PER_LONG - 1),
-		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
+	i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
+	if (i == h->nr_cmds) {
+		spin_unlock_irqrestore(&h->lock, flags);
+		return NULL;
+	}
+	set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG));
 	h->nr_allocs++;
 	spin_unlock_irqrestore(&h->lock, flags);
 

Signature · 1 line

1.8.3.1

Would it be better instead to just not use the spinlock for protecting cmd_pool_bits? I have thought about doing this for awhile, but haven't gotten around to it.

Quoted 6 lines

Quoted 2 lines

I think the while loop is safe without the spin lock. And then it is not needed in cmd_free either.

I was evaluating the same idea for a while too, a loop and inside just the test_and_set_bit, maybe even a stored value to start with a likely empty bit from last time to tune it a bit. But I know almost nothing about the use pattern, so I decided for the least invasive change to the existing code, to not make it worse.

Only reason I haven't done it is I'm loathe to make such a change to the main i/o path without testing it like crazy before unleashing it, and it's never been a convenient time to slide such a change in around here and get proper testing done (and there are other rather large changes brewing).

Quoted 6 lines

However, we have been using a similar scheme with the SCSI over PCIe driver,
here: https://github.com/HPSmartStorage/scsi-over-pcie/blob/master/block/sop.c
in alloc_request() around line 1476 without problems, and nvme-core.c contains similar code in alloc_cmdid(), so I am confident it's sound in principle. I would want to beat on it though, in case it ends up exposing a firmware bug or something (not that I think it will, but you never know.)

I think the code is sound, maybe it could hypothetically return -EBUSY, because the find_first_zero_bit is not atomic, but this possibility is so low that it doesn't matter. Btw. on line 1284 - isn't it similar to patch 2/3 ?

Back to this patch - we can take it as it is, because of the spinlock it should be safe, or omit it, you can then post a spinlock-less patch. I'll let the decision on you.

tomash

Quoted 12 lines

-- steve

Quoted 7 lines

Quoted 7 lines

-- steve

--
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 65 lines · Tomas Henzl

Quoted 58 lines · On 08/01/2013 04:21 PM, [email protected] wrote:

Quoted 46 lines · Tomas Henzl

Quoted 45 lines · On 08/01/2013 03:39 PM, [email protected] wrote:

Quoted 40 lines · Tomas Henzl

From: Tomas Henzl <[email protected]>

The cmd_pool_bits is protected everywhere with a spinlock, we don't need the test_and_set_bit, set_bit is enough and the loop can be removed too.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/scsi/hpsa.c +6 -9

index 796482b..d7df01e 100644
@@ -2662,15 +2662,12 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	unsigned long flags;
 
 	spin_lock_irqsave(&h->lock, flags);
-	do {
-		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds) {
-			spin_unlock_irqrestore(&h->lock, flags);
-			return NULL;
-		}
-	} while (test_and_set_bit
-		 (i & (BITS_PER_LONG - 1),
-		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
+	i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
+	if (i == h->nr_cmds) {
+		spin_unlock_irqrestore(&h->lock, flags);
+		return NULL;
+	}
+	set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG));
 	h->nr_allocs++;
 	spin_unlock_irqrestore(&h->lock, flags);
 

Signature · 1 line

1.8.3.1

Would it be better instead to just not use the spinlock for protecting cmd_pool_bits? I have thought about doing this for awhile, but haven't gotten around to it.

Quoted 6 lines

Quoted 2 lines

I think the while loop is safe without the spin lock. And then it is not needed in cmd_free either.

I was evaluating the same idea for a while too, a loop and inside just the test_and_set_bit, maybe even a stored value to start with a likely empty bit from last time to tune it a bit. But I know almost nothing about the use pattern, so I decided for the least invasive change to the existing code, to not make it worse.

Only reason I haven't done it is I'm loathe to make such a change to the main i/o path without testing it like crazy before unleashing it, and it's never been a convenient time to slide such a change in around here and get proper testing done (and there are other rather large changes brewing).

Quoted 6 lines

However, we have been using a similar scheme with the SCSI over PCIe driver,
here: https://github.com/HPSmartStorage/scsi-over-pcie/blob/master/block/sop.c
in alloc_request() around line 1476 without problems, and nvme-core.c contains similar code in alloc_cmdid(), so I am confident it's sound in principle. I would want to beat on it though, in case it ends up exposing a firmware bug or something (not that I think it will, but you never know.)

Quoted 3 lines

I think the code is sound, maybe it could hypothetically return -EBUSY, because the find_first_zero_bit is not atomic, but this possibility is so low that it doesn't matter. Btw. on line 1284 - isn't it similar to patch 2/3 ?

find_first_zero_bit is not atomic, but the test_and_set_bit, which is what
counts, is atomic. That find_first_zero_bit is not atomic confused me about
this code for a long time, and is why the spin lock was there in the first place. But if there's a race on the find_first_zero_bit and it returns the same bit to multiple concurrent threads, only one thread will win the test_and_set_bit, and the other threads will go back around the loop to try again, and get a different bit.

I don't think a thread can get stuck in there never winning until all the bits are used up because there should always be enough bits for all the commands we would ever try to send concurrently, so every thread that gets in there should eventually get a bit.

Or, am I missing some subtlety?

Quoted 2 lines

Back to this patch - we can take it as it is, because of the spinlock it should be safe, or omit it, you can then post a spinlock-less patch. I'll let the decision on you.

I think I like the spin-lock-less variant better. But I want to test it out here for awhile first.

-- steve

Quoted 13 lines

tomash

Quoted 10 lines

-- steve

Quoted 6 lines

Quoted 6 lines

-- steve

--
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

On 08/01/2013 05:19 PM, [email protected] wrote:

Quoted 1 line

Quoted 1 line

Btw. on line 1284 - isn't it similar to patch 2/3 ?

Quoted 7 lines

find_first_zero_bit is not atomic, but the test_and_set_bit, which is what
counts, is atomic. That find_first_zero_bit is not atomic confused me about
this code for a long time, and is why the spin lock was there in the first place. But if there's a race on the find_first_zero_bit and it returns the same bit to multiple concurrent threads, only one thread will win the test_and_set_bit, and the other threads will go back around the loop to try again, and get a different bit.

Yes.
But, let's expect just one zero bit at the end of the list. The find_first_zero_bit(ffzb) starts now, thread+1 zeroes a new bit at the beginning, ffzb continues, thread+2 takes the zero bit at the end. The result it that ffzb hasn't found a zero bit even though that at every moment that bit was there.Ffter that the function returns -EBUSY. rc = (u16) find_first_zero_bit(qinfo->request_bits, qinfo->qdepth);
if (rc >= qinfo->qdepth-1)

	return (u16) -EBUSY;

Still, I think that this is almost impossible, and if it should happen a requeue is not so bad.

Quoted 1 line · Tomas Henzl

On 08/01/2013 05:19 PM, [email protected] wrote:

[...]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Btw. on line 1284 - isn't it similar to patch 2/3 ?

^^^ Oh, missed this the first time around, the sop driver uses the make_request_fn() interface, and it's not a stacked driver either, so there is no limit to the number of bios the block layer can stuff in -- make_request_fn must succeed. If we get full we just chain them together using pointers already in the struct bio for that purpose, so storing them in the driver requires no memory allocation on the driver's part. So while it's somewhat similar, we already have to handle the case of the block layer stuffing infinite bios into the driver, so getting full is not terribly out of the ordinary in that driver.

That being said, I'm poking around other bits of code lying around here looking for similar problems, so thanks again for that one.

Quoted 14 lines

Quoted 7 lines

find_first_zero_bit is not atomic, but the test_and_set_bit, which is what
counts, is atomic. That find_first_zero_bit is not atomic confused me about
this code for a long time, and is why the spin lock was there in the first place. But if there's a race on the find_first_zero_bit and it returns the same bit to multiple concurrent threads, only one thread will win the test_and_set_bit, and the other threads will go back around the loop to try again, and get a different bit.

Yes.
But, let's expect just one zero bit at the end of the list. The find_first_zero_bit(ffzb) starts now, thread+1 zeroes a new bit at the beginning, ffzb continues, thread+2 takes the zero bit at the end. The result it that ffzb hasn't found a zero bit even though that at every moment that bit was there.Ffter that the function returns -EBUSY. rc = (u16) find_first_zero_bit(qinfo->request_bits, qinfo->qdepth);
if (rc >= qinfo->qdepth-1)

Quoted 3 lines

	return (u16) -EBUSY;

Still, I think that this is almost impossible, and if it should happen a requeue is not so bad.

Oh, wow. Didn't think of that. Hmm, technically no guarantee that any given thread would ever get a bit, if all the other threads keep snatching them away just ahead of an unlucky thread.

Could we, instead of giving up, go back around and try again on the theory that some bits should be free in there someplace and the thread shouldn't be infinitely unlucky?

[...]

-- steve
--
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 5 lines · On 08/01/2013 06:18 PM, [email protected] wrote:

Quoted 1 line · Tomas Henzl

On 08/01/2013 05:19 PM, [email protected] wrote:

[...]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Btw. on line 1284 - isn't it similar to patch 2/3 ?

Quoted 8 lines

^^^ Oh, missed this the first time around, the sop driver uses the make_request_fn() interface, and it's not a stacked driver either, so there is no limit to the number of bios the block layer can stuff in -- make_request_fn must succeed. If we get full we just chain them together using pointers already in the struct bio for that purpose, so storing them in the driver requires no memory allocation on the driver's part. So while it's somewhat similar, we already have to handle the case of the block layer stuffing infinite bios into the driver, so getting full is not terribly out of the ordinary in that driver.

OK.

Quoted 21 lines

That being said, I'm poking around other bits of code lying around here looking for similar problems, so thanks again for that one.

Quoted 14 lines

Quoted 7 lines

find_first_zero_bit is not atomic, but the test_and_set_bit, which is what
counts, is atomic. That find_first_zero_bit is not atomic confused me about
this code for a long time, and is why the spin lock was there in the first place. But if there's a race on the find_first_zero_bit and it returns the same bit to multiple concurrent threads, only one thread will win the test_and_set_bit, and the other threads will go back around the loop to try again, and get a different bit.

Yes.
But, let's expect just one zero bit at the end of the list. The find_first_zero_bit(ffzb) starts now, thread+1 zeroes a new bit at the beginning, ffzb continues, thread+2 takes the zero bit at the end. The result it that ffzb hasn't found a zero bit even though that at every moment that bit was there.Ffter that the function returns -EBUSY. rc = (u16) find_first_zero_bit(qinfo->request_bits, qinfo->qdepth);
if (rc >= qinfo->qdepth-1)

Quoted 3 lines

	return (u16) -EBUSY;

Still, I think that this is almost impossible, and if it should happen a requeue is not so bad.

Quoted 7 lines

Oh, wow. Didn't think of that. Hmm, technically no guarantee that any given thread would ever get a bit, if all the other threads keep snatching them away just ahead of an unlucky thread.

Could we, instead of giving up, go back around and try again on the theory that some bits should be free in there someplace and the thread shouldn't be infinitely unlucky?

In theory that gives you also no guarantee, it's likely that for a guarantee some kind of locking is needed, the spinlock, which already is there, gives you that. Otoh, a very high likelihood is probably enough and give better overall throughput, maybe some statistics/testing is needed? I don't know how much faster is it without the spinlock.

Quoted 40 lines · Tomas Henzl

Quoted 5 lines · On 08/01/2013 06:18 PM, [email protected] wrote:

Quoted 1 line · Tomas Henzl

On 08/01/2013 05:19 PM, [email protected] wrote:

[...]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Btw. on line 1284 - isn't it similar to patch 2/3 ?

Quoted 8 lines

^^^ Oh, missed this the first time around, the sop driver uses the make_request_fn() interface, and it's not a stacked driver either, so there is no limit to the number of bios the block layer can stuff in -- make_request_fn must succeed. If we get full we just chain them together using pointers already in the struct bio for that purpose, so storing them in the driver requires no memory allocation on the driver's part. So while it's somewhat similar, we already have to handle the case of the block layer stuffing infinite bios into the driver, so getting full is not terribly out of the ordinary in that driver.

OK.

Quoted 24 lines

That being said, I'm poking around other bits of code lying around here looking for similar problems, so thanks again for that one.

Quoted 14 lines

Quoted 7 lines

find_first_zero_bit is not atomic, but the test_and_set_bit, which is what
counts, is atomic. That find_first_zero_bit is not atomic confused me about
this code for a long time, and is why the spin lock was there in the first place. But if there's a race on the find_first_zero_bit and it returns the same bit to multiple concurrent threads, only one thread will win the test_and_set_bit, and the other threads will go back around the loop to try again, and get a different bit.

Yes.
But, let's expect just one zero bit at the end of the list. The find_first_zero_bit(ffzb) starts now, thread+1 zeroes a new bit at the beginning, ffzb continues, thread+2 takes the zero bit at the end. The result it that ffzb hasn't found a zero bit even though that at every moment that bit was there.Ffter that the function returns -EBUSY. rc = (u16) find_first_zero_bit(qinfo->request_bits, qinfo->qdepth);
if (rc >= qinfo->qdepth-1)

Quoted 3 lines

	return (u16) -EBUSY;

Still, I think that this is almost impossible, and if it should happen a requeue is not so bad.

Oh, wow. Didn't think of that. Hmm, technically no guarantee that any given thread would ever get a bit, if all the other threads keep snatching them away just ahead of an unlucky thread.

Quoted 8 lines

Quoted 3 lines

Could we, instead of giving up, go back around and try again on the theory that some bits should be free in there someplace and the thread shouldn't be infinitely unlucky?

In theory that gives you also no guarantee, it's likely that for a guarantee some kind of locking is needed, the spinlock, which already is there, gives you that. Otoh, a very high likelihood is probably enough and give better overall throughput, maybe some statistics/testing is needed? I don't know how much faster is it without the spinlock.

On thinking about this a bit more, it would be a shame if we closed the hole allowing the "cmd_alloc returned NULL" message (the scsi_done() cmd_free() race) and then immediately opened up another different hole that permitted the same problem to occur.

So to be safe, I think we should go with your patch as is -- leave the spin lock, but get rid of the unnecessary loop.

-- steve

--
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

From: Tomas Henzl <[email protected]>

The cmd_pool_bits is protected everywhere with a spinlock, we don't need the test_and_set_bit, set_bit is enough and the loop can be removed too.

Signed-off-by Tomas Henzl <[email protected]>

---
 drivers/scsi/hpsa.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

drivers/scsi/hpsa.c +6 -9

index 796482b..d7df01e 100644
@@ -2662,15 +2662,12 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	unsigned long flags;
 
 	spin_lock_irqsave(&h->lock, flags);
-	do {
-		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
-		if (i == h->nr_cmds) {
-			spin_unlock_irqrestore(&h->lock, flags);
-			return NULL;
-		}
-	} while (test_and_set_bit
-		 (i & (BITS_PER_LONG - 1),
-		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
+	i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
+	if (i == h->nr_cmds) {
+		spin_unlock_irqrestore(&h->lock, flags);
+		return NULL;
+	}
+	set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG));
 	h->nr_allocs++;
 	spin_unlock_irqrestore(&h->lock, flags);