On 06/04/2021 18:49, Bart Van Assche wrote:
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 Bart,
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."
I actually said the same thing about the global blk_sched_srcu, but
would not want a per-tagset srcu struct unless it was necessary for your
same reason (additional memory).
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().
I think it depends on whether assigning NULL, which gives WRITE_ONCE(),
while non-NULL is smp_store_release().
On x86
smp_store_release() is translated into a compiler barrier +
WRITE_ONCE().
Right, asm-generic gives smp mb + WRITE_ONCE(), and arm64 has a
dedicated store-release instruction.
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.
OK, I tend to agree with this. But it seems to me that they are needed
anyway.
Anyway, I'll have a look at the latest series...
Thanks,
John
