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@xxxxxxx/. 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.
