From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754294Ab3CATuJ (ORCPT ); Fri, 1 Mar 2013 14:50:09 -0500 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:56826 "EHLO e23smtp04.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754637Ab3CATuC (ORCPT ); Fri, 1 Mar 2013 14:50:02 -0500 Message-ID: <513105DD.8010908@linux.vnet.ibm.com> Date: Sat, 02 Mar 2013 01:17:41 +0530 From: "Srivatsa S. Bhat" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120828 Thunderbird/15.0 MIME-Version: 1.0 To: Lai Jiangshan 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> <5130EA6B.6030901@cn.fujitsu.com> In-Reply-To: <5130EA6B.6030901@cn.fujitsu.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13030119-9264-0000-0000-0000033E317C Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/01/2013 11:20 PM, Lai Jiangshan wrote: > 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. > OK, in that case, I'm extremely sorry too, for jumping on you like that. I hope you'll forgive me for the uneasiness it caused. Now that I understand that you were simply trying to help, I would like to express my gratitude for your time, effort and inputs in improving the design of the stop-machine replacement. I'm looking forward to working with you on this as well as future endeavours, so I sincerely hope that we can put this unfortunate incident behind us and collaborate effectively with renewed mutual trust and good-will. Thank you very much! Regards, Srivatsa S. Bhat From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Srivatsa S. Bhat" Subject: Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks Date: Sat, 02 Mar 2013 01:17:41 +0530 Message-ID: <513105DD.8010908@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> <5130EA6B.6030901@cn.fujitsu.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: Lai Jiangshan Return-path: In-Reply-To: <5130EA6B.6030901@cn.fujitsu.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On 03/01/2013 11:20 PM, Lai Jiangshan wrote: > 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. > OK, in that case, I'm extremely sorry too, for jumping on you like that. I hope you'll forgive me for the uneasiness it caused. Now that I understand that you were simply trying to help, I would like to express my gratitude for your time, effort and inputs in improving the design of the stop-machine replacement. I'm looking forward to working with you on this as well as future endeavours, so I sincerely hope that we can put this unfortunate incident behind us and collaborate effectively with renewed mutual trust and good-will. Thank you very much! Regards, Srivatsa S. Bhat From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp05.au.ibm.com (e23smtp05.au.ibm.com [202.81.31.147]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "e23smtp05.au.ibm.com", Issuer "GeoTrust SSL CA" (not verified)) by ozlabs.org (Postfix) with ESMTPS id E4C3A2C0303 for ; Sat, 2 Mar 2013 06:50:01 +1100 (EST) Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sat, 2 Mar 2013 05:45:55 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 690D73578053 for ; Sat, 2 Mar 2013 06:49:54 +1100 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r21JnpGJ40239188 for ; Sat, 2 Mar 2013 06:49:51 +1100 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r21Jnpog021296 for ; Sat, 2 Mar 2013 06:49:53 +1100 Message-ID: <513105DD.8010908@linux.vnet.ibm.com> Date: Sat, 02 Mar 2013 01:17:41 +0530 From: "Srivatsa S. Bhat" MIME-Version: 1.0 To: Lai Jiangshan 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> <5130EA6B.6030901@cn.fujitsu.com> In-Reply-To: <5130EA6B.6030901@cn.fujitsu.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 03/01/2013 11:20 PM, Lai Jiangshan wrote: > 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. > OK, in that case, I'm extremely sorry too, for jumping on you like that. I hope you'll forgive me for the uneasiness it caused. Now that I understand that you were simply trying to help, I would like to express my gratitude for your time, effort and inputs in improving the design of the stop-machine replacement. I'm looking forward to working with you on this as well as future endeavours, so I sincerely hope that we can put this unfortunate incident behind us and collaborate effectively with renewed mutual trust and good-will. Thank you very much! Regards, Srivatsa S. Bhat From mboxrd@z Thu Jan 1 00:00:00 1970 From: srivatsa.bhat@linux.vnet.ibm.com (Srivatsa S. Bhat) Date: Sat, 02 Mar 2013 01:17:41 +0530 Subject: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks In-Reply-To: <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> <5130EA6B.6030901@cn.fujitsu.com> Message-ID: <513105DD.8010908@linux.vnet.ibm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 03/01/2013 11:20 PM, Lai Jiangshan wrote: > 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. > OK, in that case, I'm extremely sorry too, for jumping on you like that. I hope you'll forgive me for the uneasiness it caused. Now that I understand that you were simply trying to help, I would like to express my gratitude for your time, effort and inputs in improving the design of the stop-machine replacement. I'm looking forward to working with you on this as well as future endeavours, so I sincerely hope that we can put this unfortunate incident behind us and collaborate effectively with renewed mutual trust and good-will. Thank you very much! Regards, Srivatsa S. Bhat