Re: [PATCH] blk-mq: Fix recently introduced races in the timeout handling code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/9/18 8:58 AM, Bart Van Assche wrote:
> On Mon, 2018-04-09 at 11:37 +0200, Christoph Hellwig wrote:
>> This looks sensible, but I'm worried about taking a whole spinlock
>> for every request completion, including irq disabling.  However it seems
>> like your new updated pattern would fit use of cmpxchg() very nicely.
> 
> Hello Christoph,
> 
> Thanks for the review. I had a look at the spin lock implementation on
> x86 and apparently on x86 spin locks are implemented as qspinlocks
> (include/asm-generic/qspinlock.h). queued_spin_lock() already uses
> atomic_cmpxchg_acquire(). Are you sure that replacing the spin lock
> by cmpxchg() will yield a performance improvement?

It's definitely worth a shot, especially since this looks like a clear
cut case for cmpxchg(). Additionally, it also kills the local irq
disable.

Conversion should be trivial and we can do some perf testing around
that.

Neither solution really makes me happy though, the prospect of
either kind of synchronization cost isn't appealing at all. I'll
have to ponder this a lot more, but it would be ideal if we could
shift that cost to the extremely unlikely case of a timeout
triggering rather than add the cost upfront to EVERY request.

-- 
Jens Axboe




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]