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, 4 May 2018 00:11:54 +0000
From:   "Dilger, Andreas" <andreas.dilger@...el.com>
To:     David Laight <David.Laight@...LAB.COM>
CC:     James Simmons <jsimmons@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "Drokin, Oleg" <oleg.drokin@...el.com>,
        "Siyao, Lai" <lai.siyao@...el.com>,
        Jinshan Xiong <jinshan.xiong@...el.com>,
        NeilBrown <neilb@...e.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 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 said, it might be good to include this information into the
commit comment itself.

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation