Messages in this thread | | | From | NeilBrown <> | Date | Fri, 04 May 2018 10:53:36 +1000 | Subject | Re: [PATCH 1/4] staging: lustre: obdclass: change spinlock of key to rwlock |
| |
On Fri, May 04 2018, Dilger, Andreas wrote:
> On May 3, 2018, at 07:50, David Laight <David.Laight@ACULAB.COM> 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. >> >> ... >>> - 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 was a fair amount of benchmarking done for this that shows the > performance is significantly improved with the patch, which can be > seen in the ticket that was referenced in the original commit comment: > > https://jira.hpdd.intel.com/browse/LU-6800?focusedCommentId=121776#comment-121776
That does surprise me. The only places where the lock is held for read are very short - clearing a few fields or incrementing a value. But numbers don't lie. I wonder if the next patch would have had just as big an effect. Taking and dropping the lock 40 times is not likely to be good for performance.
Thanks, NeilBrown
> > That said, it might be good to include this information into the > commit comment itself. > > Cheers, Andreas > -- > Andreas Dilger > Lustre Principal Architect > Intel Corporation [unhandled content-type:application/pgp-signature] | |