From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751971Ab3CARsk (ORCPT ); Fri, 1 Mar 2013 12:48:40 -0500 Received: from cn.fujitsu.com ([222.73.24.84]:57591 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751151Ab3CARsi (ORCPT ); Fri, 1 Mar 2013 12:48:38 -0500 X-IronPort-AV: E=Sophos;i="4.84,764,1355068800"; d="scan'208";a="6795425" Message-ID: <5130EA6B.6030901@cn.fujitsu.com> Date: Sat, 02 Mar 2013 01:50:35 +0800 From: Lai Jiangshan User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.9) Gecko/20100921 Fedora/3.1.4-1.fc14 Thunderbird/3.1.4 MIME-Version: 1.0 To: "Srivatsa S. Bhat" CC: Lai Jiangshan , Michel Lespinasse , linux-doc@vger.kernel.org, peterz@infradead.org, fweisbec@gmail.com, linux-kernel@vger.kernel.org, namhyung@kernel.org, mingo@kernel.org, linux-arch@vger.kernel.org, linux@arm.linux.org.uk, xiaoguangrong@linux.vnet.ibm.com, wangyun@linux.vnet.ibm.com, paulmck@linux.vnet.ibm.com, nikunj@linux.vnet.ibm.com, linux-pm@vger.kernel.org, rusty@rustcorp.com.au, rostedt@goodmis.org, rjw@sisk.pl, vincent.guittot@linaro.org, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, oleg@redhat.com, sbw@mit.edu, tj@kernel.org, akpm@linux-foundation.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks References: <20130218123714.26245.61816.stgit@srivatsabhat.in.ibm.com> <20130218123856.26245.46705.stgit@srivatsabhat.in.ibm.com> <5122551E.1080703@linux.vnet.ibm.com> <51226B46.9080707@linux.vnet.ibm.com> <51226F91.7000108@linux.vnet.ibm.com> <512BBAD8.8010006@linux.vnet.ibm.com> <512C7A38.8060906@linux.vnet.ibm.com> <512CC509.1050000@linux.vnet.ibm.com> <512D0D67.9010609@linux.vnet.ibm.com> <512E7879.20109@linux.vnet.ibm.com> In-Reply-To: <512E7879.20109@linux.vnet.ibm.com> X-MIMETrack: Itemize by SMTP Server on mailserver/fnst(Release 8.5.3|September 15, 2011) at 2013/03/02 01:47:39, Serialize by Router on mailserver/fnst(Release 8.5.3|September 15, 2011) at 2013/03/02 01:47:40, Serialize complete at 2013/03/02 01:47:40 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28/02/13 05:19, Srivatsa S. Bhat wrote: > On 02/27/2013 06:03 AM, Lai Jiangshan wrote: >> On Wed, Feb 27, 2013 at 3:30 AM, Srivatsa S. Bhat >> wrote: >>> On 02/26/2013 09:55 PM, Lai Jiangshan wrote: >>>> On Tue, Feb 26, 2013 at 10:22 PM, Srivatsa S. Bhat >>>> wrote: >>>>> >>>>> Hi Lai, >>>>> >>>>> I'm really not convinced that piggy-backing on lglocks would help >>>>> us in any way. But still, let me try to address some of the points >>>>> you raised... >>>>> >>>>> On 02/26/2013 06:29 PM, Lai Jiangshan wrote: >>>>>> On Tue, Feb 26, 2013 at 5:02 PM, Srivatsa S. Bhat >>>>>> wrote: >>>>>>> On 02/26/2013 05:47 AM, Lai Jiangshan wrote: >>>>>>>> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat >>>>>>>> wrote: >>>>>>>>> Hi Lai, >>>>>>>>> >>>>>>>>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote: >>>>>>>>>> Hi, Srivatsa, >>>>>>>>>> >>>>>>>>>> The target of the whole patchset is nice for me. >>>>>>>>> >>>>>>>>> Cool! Thanks :-) >>>>>>>>> >>>>>>> [...] >>>>>>> >>>>>>> Unfortunately, I see quite a few issues with the code above. IIUC, the >>>>>>> writer and the reader both increment the same counters. So how will the >>>>>>> unlock() code in the reader path know when to unlock which of the locks? >>>>>> >>>>>> The same as your code, the reader(which nested in write C.S.) just dec >>>>>> the counters. >>>>> >>>>> And that works fine in my case because the writer and the reader update >>>>> _two_ _different_ counters. >>>> >>>> I can't find any magic in your code, they are the same counter. >>>> >>>> /* >>>> * It is desirable to allow the writer to acquire the percpu-rwlock >>>> * for read (if necessary), without deadlocking or getting complaints >>>> * from lockdep. To achieve that, just increment the reader_refcnt of >>>> * this CPU - that way, any attempt by the writer to acquire the >>>> * percpu-rwlock for read, will get treated as a case of nested percpu >>>> * reader, which is safe, from a locking perspective. >>>> */ >>>> this_cpu_inc(pcpu_rwlock->rw_state->reader_refcnt); >>>> >>> >>> Whoa! Hold on, were you really referring to _this_ increment when you said >>> that, in your patch you would increment the refcnt at the writer? Then I guess >>> there is a major disconnect in our conversations. (I had assumed that you were >>> referring to the update of writer_signal, and were just trying to have a single >>> refcnt instead of reader_refcnt and writer_signal). >> >> https://github.com/laijs/linux/commit/53e5053d5b724bea7c538b11743d0f420d98f38d >> >> Sorry the name "fallback_reader_refcnt" misled you. >> > [...] > >>>> All I was considered is "nested reader is seldom", so I always >>>> fallback to rwlock when nested. >>>> If you like, I can add 6 lines of code, the overhead is >>>> 1 spin_try_lock()(fast path) + N __this_cpu_inc() >>>> >>> >>> I'm assuming that calculation is no longer valid, considering that >>> we just discussed how the per-cpu refcnt that you were using is quite >>> unnecessary and can be removed. >>> >>> IIUC, the overhead with your code, as per above discussion would be: >>> 1 spin_try_lock() [non-nested] + N read_lock(global_rwlock). >> >> https://github.com/laijs/linux/commit/46334544bb7961550b7065e015da76f6dab21f16 >> >> Again, I'm so sorry the name "fallback_reader_refcnt" misled you. >> > > At this juncture I really have to admit that I don't understand your > intentions at all. What are you really trying to prove? Without giving > a single good reason why my code is inferior, why are you even bringing > up the discussion about a complete rewrite of the synchronization code? > http://article.gmane.org/gmane.linux.kernel.cross-arch/17103 > http://article.gmane.org/gmane.linux.power-management.general/31345 > > I'm beginning to add 2 + 2 together based on the kinds of questions you > have been asking... > > You posted a patch in this thread and started a discussion around it without > even establishing a strong reason to do so. Now you point me to your git > tree where your patches have even more traces of ideas being borrowed from > my patchset (apart from my own ideas/code, there are traces of others' ideas > being borrowed too - for example, it was Oleg who originally proposed the > idea of splitting up the counter into 2 parts and I'm seeing that it is > slowly crawling into your code with no sign of appropriate credits). > http://article.gmane.org/gmane.linux.network/260288 > > And in reply to my mail pointing out the performance implications of the > global read_lock at the reader side in your code, you said you'll come up > with a comparison between that and my patchset. > http://article.gmane.org/gmane.linux.network/260288 > The issue has been well-documented in my patch description of patch 4. > http://article.gmane.org/gmane.linux.kernel/1443258 > > Are you really trying to pit bits and pieces of my own ideas/versions > against one another and claiming them as your own? > > You projected the work involved in handling the locking issues pertaining > to CPU_DYING notifiers etc as a TODO, despite the fact that I had explicitly > noted in my cover letter that I had audited and taken care of all of them. > http://article.gmane.org/gmane.linux.documentation/9727 > http://article.gmane.org/gmane.linux.documentation/9520 > > You failed to acknowledge (on purpose?) that I had done a tree-wide > conversion despite the fact that you were replying to the very thread which > had the 46 patches which did exactly that (and I had also mentioned it > explicitly in my cover letter). > http://article.gmane.org/gmane.linux.documentation/9727 > http://article.gmane.org/gmane.linux.documentation/9520 > > You then started probing more and more about the technique I used to do > the tree-wide conversion. > http://article.gmane.org/gmane.linux.kernel.cross-arch/17111 > > You also retorted saying you did go through my patch descriptions, so > its not like you have missed reading them. > http://article.gmane.org/gmane.linux.power-management.general/31345 > > Each of these when considered individually, might appear like innocuous and > honest attempts at evaluating my code. But when put together, I'm beginning > to sense a whole different angle to it altogether, as if you are trying > to spin your own patch series, complete with the locking framework _and_ > the tree-wide conversion, heavily borrowed from mine. At the beginning of > this discussion, I predicted that the lglock version that you are proposing > would end up being either less efficient than my version or look very similar > to my version. http://article.gmane.org/gmane.linux.kernel/1447139 > > I thought it was just the former till now, but its not hard to see how it > is getting closer to becoming the latter too. So yeah, I'm not amused. > > Maybe (and hopefully) you are just trying out different ideas on your own, > and I'm just being paranoid. I really hope that is the case. If you are just > trying to review my code, then please stop sending patches with borrowed ideas > with your sole Signed-off-by, and purposefully ignoring the work already done > in my patchset, because it is really starting to look suspicious, at least > to me. > > Don't get me wrong - I'll whole-heartedly acknowledge and appreciate if > _your_ code is better than mine. I just don't like the idea of somebody > plagiarizing my ideas/code (or even others' ideas for that matter). > However, I sincerely apologize in advance if I misunderstood/misjudged your > intentions; I just wanted to voice my concerns out loud at this point, > considering the bad feeling I got by looking at your responses collectively. > Hi, Srivatsa I'm sorry, big apology to you. I'm bad in communication and I did be wrong. I tended to improve the codes but in false direction. Thanks, Lai From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lai Jiangshan Subject: Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks Date: Sat, 02 Mar 2013 01:50:35 +0800 Message-ID: <5130EA6B.6030901@cn.fujitsu.com> References: <20130218123714.26245.61816.stgit@srivatsabhat.in.ibm.com> <20130218123856.26245.46705.stgit@srivatsabhat.in.ibm.com> <5122551E.1080703@linux.vnet.ibm.com> <51226B46.9080707@linux.vnet.ibm.com> <51226F91.7000108@linux.vnet.ibm.com> <512BBAD8.8010006@linux.vnet.ibm.com> <512C7A38.8060906@linux.vnet.ibm.com> <512CC509.1050000@linux.vnet.ibm.com> <512D0D67.9010609@linux.vnet .ibm.com> <512E7879.20109@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Lai Jiangshan , Michel Lespinasse , linux-doc@vger.kernel.org, peterz@infradead.org, fweisbec@gmail.com, linux-kernel@vger.kernel.org, namhyung@kernel.org, mingo@kernel.org, linux-arch@vger.kernel.org, linux@arm.linux.org.uk, xiaoguangrong@linux.vnet.ibm.com, wangyun@linux.vnet.ibm.com, paulmck@linux.vnet.ibm.com, nikunj@linux.vnet.ibm.com, linux-pm@vger.kernel.org, rusty@rustcorp.com.au, rostedt@goodmis.org, rjw@sisk.pl, vincent.guittot@linaro.org, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, oleg@redhat.com, sbw@mit.edu, tj@kernel.org, akpm@linux-foundation.org, linuxppc-dev@lists.ozlabs.org To: "Srivatsa S. Bhat" Return-path: Received: from cn.fujitsu.com ([222.73.24.84]:57591 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751151Ab3CARsi (ORCPT ); Fri, 1 Mar 2013 12:48:38 -0500 In-Reply-To: <512E7879.20109@linux.vnet.ibm.com> Sender: netdev-owner@vger.kernel.org List-ID: On 28/02/13 05:19, Srivatsa S. Bhat wrote: > On 02/27/2013 06:03 AM, Lai Jiangshan wrote: >> On Wed, Feb 27, 2013 at 3:30 AM, Srivatsa S. Bhat >> wrote: >>> On 02/26/2013 09:55 PM, Lai Jiangshan wrote: >>>> On Tue, Feb 26, 2013 at 10:22 PM, Srivatsa S. Bhat >>>> wrote: >>>>> >>>>> Hi Lai, >>>>> >>>>> I'm really not convinced that piggy-backing on lglocks would help >>>>> us in any way. But still, let me try to address some of the points >>>>> you raised... >>>>> >>>>> On 02/26/2013 06:29 PM, Lai Jiangshan wrote: >>>>>> On Tue, Feb 26, 2013 at 5:02 PM, Srivatsa S. Bhat >>>>>> wrote: >>>>>>> On 02/26/2013 05:47 AM, Lai Jiangshan wrote: >>>>>>>> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat >>>>>>>> wrote: >>>>>>>>> Hi Lai, >>>>>>>>> >>>>>>>>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote: >>>>>>>>>> Hi, Srivatsa, >>>>>>>>>> >>>>>>>>>> The target of the whole patchset is nice for me. >>>>>>>>> >>>>>>>>> Cool! Thanks :-) >>>>>>>>> >>>>>>> [...] >>>>>>> >>>>>>> Unfortunately, I see quite a few issues with the code above. IIUC, the >>>>>>> writer and the reader both increment the same counters. So how will the >>>>>>> unlock() code in the reader path know when to unlock which of the locks? >>>>>> >>>>>> The same as your code, the reader(which nested in write C.S.) just dec >>>>>> the counters. >>>>> >>>>> And that works fine in my case because the writer and the reader update >>>>> _two_ _different_ counters. >>>> >>>> I can't find any magic in your code, they are the same counter. >>>> >>>> /* >>>> * It is desirable to allow the writer to acquire the percpu-rwlock >>>> * for read (if necessary), without deadlocking or getting complaints >>>> * from lockdep. To achieve that, just increment the reader_refcnt of >>>> * this CPU - that way, any attempt by the writer to acquire the >>>> * percpu-rwlock for read, will get treated as a case of nested percpu >>>> * reader, which is safe, from a locking perspective. >>>> */ >>>> this_cpu_inc(pcpu_rwlock->rw_state->reader_refcnt); >>>> >>> >>> Whoa! Hold on, were you really referring to _this_ increment when you said >>> that, in your patch you would increment the refcnt at the writer? Then I guess >>> there is a major disconnect in our conversations. (I had assumed that you were >>> referring to the update of writer_signal, and were just trying to have a single >>> refcnt instead of reader_refcnt and writer_signal). >> >> https://github.com/laijs/linux/commit/53e5053d5b724bea7c538b11743d0f420d98f38d >> >> Sorry the name "fallback_reader_refcnt" misled you. >> > [...] > >>>> All I was considered is "nested reader is seldom", so I always >>>> fallback to rwlock when nested. >>>> If you like, I can add 6 lines of code, the overhead is >>>> 1 spin_try_lock()(fast path) + N __this_cpu_inc() >>>> >>> >>> I'm assuming that calculation is no longer valid, considering that >>> we just discussed how the per-cpu refcnt that you were using is quite >>> unnecessary and can be removed. >>> >>> IIUC, the overhead with your code, as per above discussion would be: >>> 1 spin_try_lock() [non-nested] + N read_lock(global_rwlock). >> >> https://github.com/laijs/linux/commit/46334544bb7961550b7065e015da76f6dab21f16 >> >> Again, I'm so sorry the name "fallback_reader_refcnt" misled you. >> > > At this juncture I really have to admit that I don't understand your > intentions at all. What are you really trying to prove? Without giving > a single good reason why my code is inferior, why are you even bringing > up the discussion about a complete rewrite of the synchronization code? > http://article.gmane.org/gmane.linux.kernel.cross-arch/17103 > http://article.gmane.org/gmane.linux.power-management.general/31345 > > I'm beginning to add 2 + 2 together based on the kinds of questions you > have been asking... > > You posted a patch in this thread and started a discussion around it without > even establishing a strong reason to do so. Now you point me to your git > tree where your patches have even more traces of ideas being borrowed from > my patchset (apart from my own ideas/code, there are traces of others' ideas > being borrowed too - for example, it was Oleg who originally proposed the > idea of splitting up the counter into 2 parts and I'm seeing that it is > slowly crawling into your code with no sign of appropriate credits). > http://article.gmane.org/gmane.linux.network/260288 > > And in reply to my mail pointing out the performance implications of the > global read_lock at the reader side in your code, you said you'll come up > with a comparison between that and my patchset. > http://article.gmane.org/gmane.linux.network/260288 > The issue has been well-documented in my patch description of patch 4. > http://article.gmane.org/gmane.linux.kernel/1443258 > > Are you really trying to pit bits and pieces of my own ideas/versions > against one another and claiming them as your own? > > You projected the work involved in handling the locking issues pertaining > to CPU_DYING notifiers etc as a TODO, despite the fact that I had explicitly > noted in my cover letter that I had audited and taken care of all of them. > http://article.gmane.org/gmane.linux.documentation/9727 > http://article.gmane.org/gmane.linux.documentation/9520 > > You failed to acknowledge (on purpose?) that I had done a tree-wide > conversion despite the fact that you were replying to the very thread which > had the 46 patches which did exactly that (and I had also mentioned it > explicitly in my cover letter). > http://article.gmane.org/gmane.linux.documentation/9727 > http://article.gmane.org/gmane.linux.documentation/9520 > > You then started probing more and more about the technique I used to do > the tree-wide conversion. > http://article.gmane.org/gmane.linux.kernel.cross-arch/17111 > > You also retorted saying you did go through my patch descriptions, so > its not like you have missed reading them. > http://article.gmane.org/gmane.linux.power-management.general/31345 > > Each of these when considered individually, might appear like innocuous and > honest attempts at evaluating my code. But when put together, I'm beginning > to sense a whole different angle to it altogether, as if you are trying > to spin your own patch series, complete with the locking framework _and_ > the tree-wide conversion, heavily borrowed from mine. At the beginning of > this discussion, I predicted that the lglock version that you are proposing > would end up being either less efficient than my version or look very similar > to my version. http://article.gmane.org/gmane.linux.kernel/1447139 > > I thought it was just the former till now, but its not hard to see how it > is getting closer to becoming the latter too. So yeah, I'm not amused. > > Maybe (and hopefully) you are just trying out different ideas on your own, > and I'm just being paranoid. I really hope that is the case. If you are just > trying to review my code, then please stop sending patches with borrowed ideas > with your sole Signed-off-by, and purposefully ignoring the work already done > in my patchset, because it is really starting to look suspicious, at least > to me. > > Don't get me wrong - I'll whole-heartedly acknowledge and appreciate if > _your_ code is better than mine. I just don't like the idea of somebody > plagiarizing my ideas/code (or even others' ideas for that matter). > However, I sincerely apologize in advance if I misunderstood/misjudged your > intentions; I just wanted to voice my concerns out loud at this point, > considering the bad feeling I got by looking at your responses collectively. > Hi, Srivatsa I'm sorry, big apology to you. I'm bad in communication and I did be wrong. I tended to improve the codes but in false direction. Thanks, Lai From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from song.cn.fujitsu.com (unknown [222.73.24.84]) by ozlabs.org (Postfix) with ESMTP id 9356A2C02FE for ; Sat, 2 Mar 2013 04:48:37 +1100 (EST) Message-ID: <5130EA6B.6030901@cn.fujitsu.com> Date: Sat, 02 Mar 2013 01:50:35 +0800 From: Lai Jiangshan MIME-Version: 1.0 To: "Srivatsa S. Bhat" Subject: Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks References: <20130218123714.26245.61816.stgit@srivatsabhat.in.ibm.com> <20130218123856.26245.46705.stgit@srivatsabhat.in.ibm.com> <5122551E.1080703@linux.vnet.ibm.com> <51226B46.9080707@linux.vnet.ibm.com> <51226F91.7000108@linux.vnet.ibm.com> <512BBAD8.8010006@linux.vnet.ibm.com> <512C7A38.8060906@linux.vnet.ibm.com> <512CC509.1050000@linux.vnet.ibm.com> <512D0D67.9010609@linux.vnet.ibm.com> <512E7879.20109@linux.vnet.ibm.com> In-Reply-To: <512E7879.20109@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8 Cc: Lai Jiangshan , linux-doc@vger.kernel.org, peterz@infradead.org, fweisbec@gmail.com, oleg@redhat.com, Michel Lespinasse , mingo@kernel.org, linux-arch@vger.kernel.org, linux@arm.linux.org.uk, xiaoguangrong@linux.vnet.ibm.com, wangyun@linux.vnet.ibm.com, paulmck@linux.vnet.ibm.com, nikunj@linux.vnet.ibm.com, linux-pm@vger.kernel.org, rusty@rustcorp.com.au, rostedt@goodmis.org, rjw@sisk.pl, namhyung@kernel.org, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, vincent.guittot@linaro.org, sbw@mit.edu, tj@kernel.org, akpm@linux-foundation.org, linuxppc-dev@lists.ozlabs.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 28/02/13 05:19, Srivatsa S. Bhat wrote: > On 02/27/2013 06:03 AM, Lai Jiangshan wrote: >> On Wed, Feb 27, 2013 at 3:30 AM, Srivatsa S. Bhat >> wrote: >>> On 02/26/2013 09:55 PM, Lai Jiangshan wrote: >>>> On Tue, Feb 26, 2013 at 10:22 PM, Srivatsa S. Bhat >>>> wrote: >>>>> >>>>> Hi Lai, >>>>> >>>>> I'm really not convinced that piggy-backing on lglocks would help >>>>> us in any way. But still, let me try to address some of the points >>>>> you raised... >>>>> >>>>> On 02/26/2013 06:29 PM, Lai Jiangshan wrote: >>>>>> On Tue, Feb 26, 2013 at 5:02 PM, Srivatsa S. Bhat >>>>>> wrote: >>>>>>> On 02/26/2013 05:47 AM, Lai Jiangshan wrote: >>>>>>>> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat >>>>>>>> wrote: >>>>>>>>> Hi Lai, >>>>>>>>> >>>>>>>>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote: >>>>>>>>>> Hi, Srivatsa, >>>>>>>>>> >>>>>>>>>> The target of the whole patchset is nice for me. >>>>>>>>> >>>>>>>>> Cool! Thanks :-) >>>>>>>>> >>>>>>> [...] >>>>>>> >>>>>>> Unfortunately, I see quite a few issues with the code above. IIUC, the >>>>>>> writer and the reader both increment the same counters. So how will the >>>>>>> unlock() code in the reader path know when to unlock which of the locks? >>>>>> >>>>>> The same as your code, the reader(which nested in write C.S.) just dec >>>>>> the counters. >>>>> >>>>> And that works fine in my case because the writer and the reader update >>>>> _two_ _different_ counters. >>>> >>>> I can't find any magic in your code, they are the same counter. >>>> >>>> /* >>>> * It is desirable to allow the writer to acquire the percpu-rwlock >>>> * for read (if necessary), without deadlocking or getting complaints >>>> * from lockdep. To achieve that, just increment the reader_refcnt of >>>> * this CPU - that way, any attempt by the writer to acquire the >>>> * percpu-rwlock for read, will get treated as a case of nested percpu >>>> * reader, which is safe, from a locking perspective. >>>> */ >>>> this_cpu_inc(pcpu_rwlock->rw_state->reader_refcnt); >>>> >>> >>> Whoa! Hold on, were you really referring to _this_ increment when you said >>> that, in your patch you would increment the refcnt at the writer? Then I guess >>> there is a major disconnect in our conversations. (I had assumed that you were >>> referring to the update of writer_signal, and were just trying to have a single >>> refcnt instead of reader_refcnt and writer_signal). >> >> https://github.com/laijs/linux/commit/53e5053d5b724bea7c538b11743d0f420d98f38d >> >> Sorry the name "fallback_reader_refcnt" misled you. >> > [...] > >>>> All I was considered is "nested reader is seldom", so I always >>>> fallback to rwlock when nested. >>>> If you like, I can add 6 lines of code, the overhead is >>>> 1 spin_try_lock()(fast path) + N __this_cpu_inc() >>>> >>> >>> I'm assuming that calculation is no longer valid, considering that >>> we just discussed how the per-cpu refcnt that you were using is quite >>> unnecessary and can be removed. >>> >>> IIUC, the overhead with your code, as per above discussion would be: >>> 1 spin_try_lock() [non-nested] + N read_lock(global_rwlock). >> >> https://github.com/laijs/linux/commit/46334544bb7961550b7065e015da76f6dab21f16 >> >> Again, I'm so sorry the name "fallback_reader_refcnt" misled you. >> > > At this juncture I really have to admit that I don't understand your > intentions at all. What are you really trying to prove? Without giving > a single good reason why my code is inferior, why are you even bringing > up the discussion about a complete rewrite of the synchronization code? > http://article.gmane.org/gmane.linux.kernel.cross-arch/17103 > http://article.gmane.org/gmane.linux.power-management.general/31345 > > I'm beginning to add 2 + 2 together based on the kinds of questions you > have been asking... > > You posted a patch in this thread and started a discussion around it without > even establishing a strong reason to do so. Now you point me to your git > tree where your patches have even more traces of ideas being borrowed from > my patchset (apart from my own ideas/code, there are traces of others' ideas > being borrowed too - for example, it was Oleg who originally proposed the > idea of splitting up the counter into 2 parts and I'm seeing that it is > slowly crawling into your code with no sign of appropriate credits). > http://article.gmane.org/gmane.linux.network/260288 > > And in reply to my mail pointing out the performance implications of the > global read_lock at the reader side in your code, you said you'll come up > with a comparison between that and my patchset. > http://article.gmane.org/gmane.linux.network/260288 > The issue has been well-documented in my patch description of patch 4. > http://article.gmane.org/gmane.linux.kernel/1443258 > > Are you really trying to pit bits and pieces of my own ideas/versions > against one another and claiming them as your own? > > You projected the work involved in handling the locking issues pertaining > to CPU_DYING notifiers etc as a TODO, despite the fact that I had explicitly > noted in my cover letter that I had audited and taken care of all of them. > http://article.gmane.org/gmane.linux.documentation/9727 > http://article.gmane.org/gmane.linux.documentation/9520 > > You failed to acknowledge (on purpose?) that I had done a tree-wide > conversion despite the fact that you were replying to the very thread which > had the 46 patches which did exactly that (and I had also mentioned it > explicitly in my cover letter). > http://article.gmane.org/gmane.linux.documentation/9727 > http://article.gmane.org/gmane.linux.documentation/9520 > > You then started probing more and more about the technique I used to do > the tree-wide conversion. > http://article.gmane.org/gmane.linux.kernel.cross-arch/17111 > > You also retorted saying you did go through my patch descriptions, so > its not like you have missed reading them. > http://article.gmane.org/gmane.linux.power-management.general/31345 > > Each of these when considered individually, might appear like innocuous and > honest attempts at evaluating my code. But when put together, I'm beginning > to sense a whole different angle to it altogether, as if you are trying > to spin your own patch series, complete with the locking framework _and_ > the tree-wide conversion, heavily borrowed from mine. At the beginning of > this discussion, I predicted that the lglock version that you are proposing > would end up being either less efficient than my version or look very similar > to my version. http://article.gmane.org/gmane.linux.kernel/1447139 > > I thought it was just the former till now, but its not hard to see how it > is getting closer to becoming the latter too. So yeah, I'm not amused. > > Maybe (and hopefully) you are just trying out different ideas on your own, > and I'm just being paranoid. I really hope that is the case. If you are just > trying to review my code, then please stop sending patches with borrowed ideas > with your sole Signed-off-by, and purposefully ignoring the work already done > in my patchset, because it is really starting to look suspicious, at least > to me. > > Don't get me wrong - I'll whole-heartedly acknowledge and appreciate if > _your_ code is better than mine. I just don't like the idea of somebody > plagiarizing my ideas/code (or even others' ideas for that matter). > However, I sincerely apologize in advance if I misunderstood/misjudged your > intentions; I just wanted to voice my concerns out loud at this point, > considering the bad feeling I got by looking at your responses collectively. > Hi, Srivatsa I'm sorry, big apology to you. I'm bad in communication and I did be wrong. I tended to improve the codes but in false direction. Thanks, Lai From mboxrd@z Thu Jan 1 00:00:00 1970 From: laijs@cn.fujitsu.com (Lai Jiangshan) Date: Sat, 02 Mar 2013 01:50:35 +0800 Subject: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks In-Reply-To: <512E7879.20109@linux.vnet.ibm.com> References: <20130218123714.26245.61816.stgit@srivatsabhat.in.ibm.com> <20130218123856.26245.46705.stgit@srivatsabhat.in.ibm.com> <5122551E.1080703@linux.vnet.ibm.com> <51226B46.9080707@linux.vnet.ibm.com> <51226F91.7000108@linux.vnet.ibm.com> <512BBAD8.8010006@linux.vnet.ibm.com> <512C7A38.8060906@linux.vnet.ibm.com> <512CC509.1050000@linux.vnet.ibm.com> <512D0D67.9010609@linux.vnet.ibm.com> <512E7879.20109@linux.vnet.ibm.com> Message-ID: <5130EA6B.6030901@cn.fujitsu.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 28/02/13 05:19, Srivatsa S. Bhat wrote: > On 02/27/2013 06:03 AM, Lai Jiangshan wrote: >> On Wed, Feb 27, 2013 at 3:30 AM, Srivatsa S. Bhat >> wrote: >>> On 02/26/2013 09:55 PM, Lai Jiangshan wrote: >>>> On Tue, Feb 26, 2013 at 10:22 PM, Srivatsa S. Bhat >>>> wrote: >>>>> >>>>> Hi Lai, >>>>> >>>>> I'm really not convinced that piggy-backing on lglocks would help >>>>> us in any way. But still, let me try to address some of the points >>>>> you raised... >>>>> >>>>> On 02/26/2013 06:29 PM, Lai Jiangshan wrote: >>>>>> On Tue, Feb 26, 2013 at 5:02 PM, Srivatsa S. Bhat >>>>>> wrote: >>>>>>> On 02/26/2013 05:47 AM, Lai Jiangshan wrote: >>>>>>>> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat >>>>>>>> wrote: >>>>>>>>> Hi Lai, >>>>>>>>> >>>>>>>>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote: >>>>>>>>>> Hi, Srivatsa, >>>>>>>>>> >>>>>>>>>> The target of the whole patchset is nice for me. >>>>>>>>> >>>>>>>>> Cool! Thanks :-) >>>>>>>>> >>>>>>> [...] >>>>>>> >>>>>>> Unfortunately, I see quite a few issues with the code above. IIUC, the >>>>>>> writer and the reader both increment the same counters. So how will the >>>>>>> unlock() code in the reader path know when to unlock which of the locks? >>>>>> >>>>>> The same as your code, the reader(which nested in write C.S.) just dec >>>>>> the counters. >>>>> >>>>> And that works fine in my case because the writer and the reader update >>>>> _two_ _different_ counters. >>>> >>>> I can't find any magic in your code, they are the same counter. >>>> >>>> /* >>>> * It is desirable to allow the writer to acquire the percpu-rwlock >>>> * for read (if necessary), without deadlocking or getting complaints >>>> * from lockdep. To achieve that, just increment the reader_refcnt of >>>> * this CPU - that way, any attempt by the writer to acquire the >>>> * percpu-rwlock for read, will get treated as a case of nested percpu >>>> * reader, which is safe, from a locking perspective. >>>> */ >>>> this_cpu_inc(pcpu_rwlock->rw_state->reader_refcnt); >>>> >>> >>> Whoa! Hold on, were you really referring to _this_ increment when you said >>> that, in your patch you would increment the refcnt at the writer? Then I guess >>> there is a major disconnect in our conversations. (I had assumed that you were >>> referring to the update of writer_signal, and were just trying to have a single >>> refcnt instead of reader_refcnt and writer_signal). >> >> https://github.com/laijs/linux/commit/53e5053d5b724bea7c538b11743d0f420d98f38d >> >> Sorry the name "fallback_reader_refcnt" misled you. >> > [...] > >>>> All I was considered is "nested reader is seldom", so I always >>>> fallback to rwlock when nested. >>>> If you like, I can add 6 lines of code, the overhead is >>>> 1 spin_try_lock()(fast path) + N __this_cpu_inc() >>>> >>> >>> I'm assuming that calculation is no longer valid, considering that >>> we just discussed how the per-cpu refcnt that you were using is quite >>> unnecessary and can be removed. >>> >>> IIUC, the overhead with your code, as per above discussion would be: >>> 1 spin_try_lock() [non-nested] + N read_lock(global_rwlock). >> >> https://github.com/laijs/linux/commit/46334544bb7961550b7065e015da76f6dab21f16 >> >> Again, I'm so sorry the name "fallback_reader_refcnt" misled you. >> > > At this juncture I really have to admit that I don't understand your > intentions at all. What are you really trying to prove? Without giving > a single good reason why my code is inferior, why are you even bringing > up the discussion about a complete rewrite of the synchronization code? > http://article.gmane.org/gmane.linux.kernel.cross-arch/17103 > http://article.gmane.org/gmane.linux.power-management.general/31345 > > I'm beginning to add 2 + 2 together based on the kinds of questions you > have been asking... > > You posted a patch in this thread and started a discussion around it without > even establishing a strong reason to do so. Now you point me to your git > tree where your patches have even more traces of ideas being borrowed from > my patchset (apart from my own ideas/code, there are traces of others' ideas > being borrowed too - for example, it was Oleg who originally proposed the > idea of splitting up the counter into 2 parts and I'm seeing that it is > slowly crawling into your code with no sign of appropriate credits). > http://article.gmane.org/gmane.linux.network/260288 > > And in reply to my mail pointing out the performance implications of the > global read_lock at the reader side in your code, you said you'll come up > with a comparison between that and my patchset. > http://article.gmane.org/gmane.linux.network/260288 > The issue has been well-documented in my patch description of patch 4. > http://article.gmane.org/gmane.linux.kernel/1443258 > > Are you really trying to pit bits and pieces of my own ideas/versions > against one another and claiming them as your own? > > You projected the work involved in handling the locking issues pertaining > to CPU_DYING notifiers etc as a TODO, despite the fact that I had explicitly > noted in my cover letter that I had audited and taken care of all of them. > http://article.gmane.org/gmane.linux.documentation/9727 > http://article.gmane.org/gmane.linux.documentation/9520 > > You failed to acknowledge (on purpose?) that I had done a tree-wide > conversion despite the fact that you were replying to the very thread which > had the 46 patches which did exactly that (and I had also mentioned it > explicitly in my cover letter). > http://article.gmane.org/gmane.linux.documentation/9727 > http://article.gmane.org/gmane.linux.documentation/9520 > > You then started probing more and more about the technique I used to do > the tree-wide conversion. > http://article.gmane.org/gmane.linux.kernel.cross-arch/17111 > > You also retorted saying you did go through my patch descriptions, so > its not like you have missed reading them. > http://article.gmane.org/gmane.linux.power-management.general/31345 > > Each of these when considered individually, might appear like innocuous and > honest attempts at evaluating my code. But when put together, I'm beginning > to sense a whole different angle to it altogether, as if you are trying > to spin your own patch series, complete with the locking framework _and_ > the tree-wide conversion, heavily borrowed from mine. At the beginning of > this discussion, I predicted that the lglock version that you are proposing > would end up being either less efficient than my version or look very similar > to my version. http://article.gmane.org/gmane.linux.kernel/1447139 > > I thought it was just the former till now, but its not hard to see how it > is getting closer to becoming the latter too. So yeah, I'm not amused. > > Maybe (and hopefully) you are just trying out different ideas on your own, > and I'm just being paranoid. I really hope that is the case. If you are just > trying to review my code, then please stop sending patches with borrowed ideas > with your sole Signed-off-by, and purposefully ignoring the work already done > in my patchset, because it is really starting to look suspicious, at least > to me. > > Don't get me wrong - I'll whole-heartedly acknowledge and appreciate if > _your_ code is better than mine. I just don't like the idea of somebody > plagiarizing my ideas/code (or even others' ideas for that matter). > However, I sincerely apologize in advance if I misunderstood/misjudged your > intentions; I just wanted to voice my concerns out loud at this point, > considering the bad feeling I got by looking at your responses collectively. > Hi, Srivatsa I'm sorry, big apology to you. I'm bad in communication and I did be wrong. I tended to improve the codes but in false direction. Thanks, Lai