Re: [PATCH v5 0/3] blk-mq: Fix a race between iterating over requests and freeing requests

[Bart Van Assche <bvanassche@acm.org>]

On 4/6/21 1:00 AM, John Garry wrote:
> Hi Bart,
> 
>> Changes between v2 and v3:
>> - Converted the single v2 patch into a series of three patches.
>> - Switched from SRCU to a combination of RCU and semaphores.
> 
> But can you mention why we made to changes from v3 onwards (versus
> v2)?
> 
> The v2 patch just used SRCU as the sync mechanism, and the impression
> I got from Jens was that the marginal performance drop was tolerable.
> And the issues it tries to address seem to be solved. So why change?
> Maybe my impression of the performance drop being acceptable was
> wrong.

Hi John,

It seems like I should have done a better job of explaining that change.
On v2 I received the following feedback from Hannes: "What I don't
particularly like is the global blk_sched_srcu here; can't
we make it per tagset?". My reply was as follows: "I'm concerned about
the additional memory required for one srcu_struct per tag set." Hence
the switch from SRCU to RCU + rwsem. See also
https://lore.kernel.org/linux-block/d1627890-fb10-7ebe-d805-621f925f80e7@suse.de/.

Regarding the 1% performance drop measured by Jens: with debugging
disabled srcu_dereference() is translated into READ_ONCE() and
rcu_assign_pointer() is translated into smp_store_release(). On x86
smp_store_release() is translated into a compiler barrier +
WRITE_ONCE(). In other words, I do not expect that the performance
difference came from the switch to SRCU but rather from the two new
hctx->tags->rqs[] assignments.

I think that the switch to READ_ONCE() / WRITE_ONCE() is unavoidable.
Even if cmpxchg() would be used to clear hctx->tags->rqs[] pointers then
we would need to convert all other hctx->tags->rqs[] accesses into
READ_ONCE() / WRITE_ONCE() to make that cmpxchg() call safe.

Bart.