Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Dilger, Andreas" <andreas.dilger@intel.com>
To:     David Laight <David.Laight@ACULAB.COM>
CC:     James Simmons <jsimmons@infradead.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "Drokin, Oleg" <oleg.drokin@intel.com>,
        "Siyao, Lai" <lai.siyao@intel.com>,
        Jinshan Xiong <jinshan.xiong@intel.com>,
        NeilBrown <neilb@suse.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Lustre Development List <lustre-devel@lists.lustre.org>,
        Li Xi <lixi@ddn.com>, Gu Zheng <gzheng@ddn.com>
Subject: Re: [PATCH 1/4] staging: lustre: obdclass: change spinlock of key
 to rwlock
Thread-Topic: [PATCH 1/4] staging: lustre: obdclass: change spinlock of key
 to rwlock
Thread-Index: AQHT4uWKeBM3CaAgP0S7TBmjqoBdPaQfKJCA
Date:   Fri, 4 May 2018 00:11:54 +0000
Message-ID: <55458B75-4105-4F4F-BB50-3D506611AB24@intel.com>
References: <1525285308-15347-1-git-send-email-jsimmons@infradead.org>
 <1525285308-15347-2-git-send-email-jsimmons@infradead.org>
 <a3594bd7c393418c8fda5150a075ff1a@AcuMS.aculab.com>
In-Reply-To: <a3594bd7c393418c8fda5150a075ff1a@AcuMS.aculab.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D4A96B45CF899248B046359C410FCF29@intel.com>
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 said, it might be good to include this information into the
commit comment itself.

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