Messages in this thread | | | From | NeilBrown <> | Date | Fri, 04 May 2018 09:26:36 +1000 | Subject | RE: [PATCH 1/4] staging: lustre: obdclass: change spinlock of key to rwlock |
| |
On Thu, May 03 2018, David Laight wrote:
> From: James Simmons >> Sent: 02 May 2018 19:22 >> From: Li Xi <lixi@ddn.com> >> >> Most of the time, keys are never changed. So rwlock might be >> better for the concurrency of key read. > > OTOH unless there is contention on the spin lock during reads the > additional cost of a rwlock (probably double that of a spinlock) > will hurt performance.
That's roughly what I was going to say - rwlocks are rarely a win. I think the second patch which caused the lock to be taken less often would have a bigger impact that the switch to rwlocks.
However I suspect a better approach would be to investigate some sort of lockless solution. I think the use of the spinlock in lu_context_key_register() could be replaced with a careful cmp_xchg(). I'm less sure about lu_context_key_degister(), but it might be possible.
> > ... >> - spin_lock(&lu_keys_guard); >> + read_lock(&lu_keys_guard); >> atomic_inc(&lu_key_initing_cnt); >> - spin_unlock(&lu_keys_guard); >> + read_unlock(&lu_keys_guard); > > WTF, seems unlikely that you need to hold any kind of lock > over an atomic_inc(). > > If this is just ensuring that no code holds the lock then > it would need to request the write_lock(). > (and would need a comment)
There is a comment - that patch showed the last 2 lines of it. This is for synchronization with lu_context_key_quiesce(). That spins(!! calling schedule, but still... not good) until the lu_key_initing_cnt is zero while it holds the write lock. Then it is sure that the code protected by this counter isn't running. I'm sure this can be improved! I would need to study it carefully to see how.
Note that I don't object to these patches going in - if they provide a measurable improvement which seems likely, then in they go. But I hope the code won't stay like this long term.
Thanks, NeilBrown [unhandled content-type:application/pgp-signature] | |