linux-kernel - Re: [PATCH 1/4] staging: lustre: obdclass: change spinlock of key to rwlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 04 May 2018 10:53:36 +1000
From:   NeilBrown <neilb@...e.com>
To:     "Dilger\, Andreas" <andreas.dilger@...el.com>,
        David Laight <David.Laight@...LAB.COM>
Cc:     James Simmons <jsimmons@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "devel\@driverdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "Drokin\, Oleg" <oleg.drokin@...el.com>,
        "Siyao\, Lai" <lai.siyao@...el.com>,
        Jinshan Xiong <jinshan.xiong@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lustre Development List <lustre-devel@...ts.lustre.org>,
        Li Xi <lixi@....com>, Gu Zheng <gzheng@....com>
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@...LAB.COM> wrote:
>> 
>> From: James Simmons
>>> Sent: 02 May 2018 19:22
>>> From: Li Xi <lixi@....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

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)