Re: [PATCH 1/3] hpsa: remove unneeded loop

[Tomas Henzl <thenzl@redhat.com>]

On 08/01/2013 06:18 PM, scameron@beardog.cce.hp.com wrote:
> On Thu, Aug 01, 2013 at 05:39:36PM +0200, Tomas Henzl wrote:
>> On 08/01/2013 05:19 PM, scameron@beardog.cce.hp.com wrote:
> [...]
>
>>>> 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.

OK.

>
> That being said, I'm poking around other bits of code lying around here
> looking for similar problems, so thanks again for that one.
>
>>> 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.
> 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.

tomash

 

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