On Thu, May 03 2018, David Laight wrote: > From: James Simmons >> Sent: 02 May 2018 19:22 >> From: Li Xi <lixi@xxxxxxx> >> >> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
