Re: [PATCH] qla2xxx: Fix dpc_thread race on the module unload

[Vladislav Bolkhovitin <vst@vlnb.net>]

James Bottomley wrote:
> On Tue, 2008-07-29 at 19:13 +0400, Vladislav Bolkhovitin wrote:
>> James Bottomley wrote:
>>> On Tue, 2008-07-29 at 13:32 +0400, Vladislav Bolkhovitin wrote:
>>>> Nope, taking only one that hunk from this patch isn't sufficient.
>>>> Around 
>>>> dpc_thread there is pretty simple and classical race. You can't do
>>>>
>>>> if (x != NULL)
>>>>         y = *x;
>>>>
>>>> without any protection, if x can be set to NULL by another thread. It 
>>>> can happen exactly between "if" and "*x" and hence lead to a crash,
>>>> correct?
>>> No.
>> What "No"? The above unlocked "if (x != NULL) y = *x;" is always safe 
>> now? ;)
> 
> No ... no as in your analysis based on the example is not correct to
> conclude protection is required.  We have quite a number of examples of
> this within the linux kernel (the SCSI error thread would be one).
> 
>>> Today we go to reasonable lengths to avoid additional locking.
>>> Spinlocks are nasty in most boxes because of the potential for
>>> introducing hot points and cacheline bouncing.
>>>
>>> The first question you ask is how hot is the variable ... as in how
>>> constantly changing is it?  This one is set at start of day and cleared
>>> when the thread is killed in shutdown.  Basically there's a single not
>>> NULL -> NULL transition.  Once NULL, it never goes back to not NULL.
>>>
>>> The point is that in this particular circumstance, the lock is overkill.
>>> You can either use other indicators to show that the driver is being
>>> removed (and the thread is dying) or you can simply use some type of
>>> refcounting to ensure the thread isn't killed until it's no longer
>>> needed.
>>>
>>> Even if it were a constantly changing variable, and therefore a more
>>> ideal candidate for locking, we might still look into atomics and bitops
>>> before adding a lock.
>> Sure, all you wrote above is correct, but in this *particular* case what 
>> you propose is an overkill, not use of the spinlock. Waking up DPC 
>> thread isn't on the hot path by any mean, it's quite rare event. So, all 
>> those lockfree techniques comparing to a simple single lock would 
>> introduce only additional complicated code without any measurable gain.
> 
> It's important to think about this kind of thing even in cases where it
> doesn't necessarily matter that much just so it becomes standard
> practice in cases where it does.

Agree. For example, in SCST core translation from LUN to internal device 
is done in a lockless manner. As well as commands serialization.

>> But, as I already wrote, I don't care much how this problem will be 
>> fixed. I care only that it should be fixed. So, my point was only that 
>> use from my patch only one hunk
>>
>> -		wake_up_process(ha->dpc_thread);
>> +		qla2xxx_wake_dpc(ha);
>>
>> is insufficient to fix the problem.
> 
> Yes, I agree with that ... I just want to see a better fix.

Let's see then what Andrew will say.

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