From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754764Ab3BRSxH (ORCPT ); Mon, 18 Feb 2013 13:53:07 -0500 Received: from e28smtp03.in.ibm.com ([122.248.162.3]:48929 "EHLO e28smtp03.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753636Ab3BRSxD (ORCPT ); Mon, 18 Feb 2013 13:53:03 -0500 Message-ID: <51227810.6090009@linux.vnet.ibm.com> Date: Tue, 19 Feb 2013 00:20:56 +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: Michel Lespinasse CC: tglx@linutronix.de, peterz@infradead.org, tj@kernel.org, oleg@redhat.com, paulmck@linux.vnet.ibm.com, rusty@rustcorp.com.au, mingo@kernel.org, akpm@linux-foundation.org, namhyung@kernel.org, rostedt@goodmis.org, wangyun@linux.vnet.ibm.com, xiaoguangrong@linux.vnet.ibm.com, rjw@sisk.pl, sbw@mit.edu, fweisbec@gmail.com, linux@arm.linux.org.uk, nikunj@linux.vnet.ibm.com, linux-pm@vger.kernel.org, linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, netdev@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, vincent.guittot@linaro.org Subject: Re: [PATCH v6 08/46] CPU hotplug: Provide APIs to prevent CPU offline from atomic context References: <20130218123714.26245.61816.stgit@srivatsabhat.in.ibm.com> <20130218123920.26245.56709.stgit@srivatsabhat.in.ibm.com> <51225A36.40600@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13021818-3864-0000-0000-000006E20376 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/18/2013 10:51 PM, Michel Lespinasse wrote: > On Tue, Feb 19, 2013 at 12:43 AM, Srivatsa S. Bhat > wrote: >> On 02/18/2013 09:53 PM, Michel Lespinasse wrote: >>> I am wondering though, if you could take care of recursive uses in >>> get/put_online_cpus_atomic() instead of doing it as a property of your >>> rwlock: >>> >>> get_online_cpus_atomic() >>> { >>> unsigned long flags; >>> local_irq_save(flags); >>> if (this_cpu_inc_return(hotplug_recusion_count) == 1) >>> percpu_read_lock_irqsafe(&hotplug_pcpu_rwlock); >>> local_irq_restore(flags); >>> } >>> >>> Once again, the idea there is to avoid baking the reader side >>> recursive properties into your rwlock itself, so that it won't be >>> impossible to implement reader/writer fairness into your rwlock in the >>> future (which may be not be very important for the hotplug use, but >>> could be when other uses get introduced). >> >> Hmm, your proposal above looks good to me, at first glance. >> (Sorry, I had mistaken your earlier mails to mean that you were against >> recursive reader-side, while you actually meant that you didn't like >> implementing the recursive reader-side logic using the recursive property >> of rwlocks). > > To be honest, I was replying as I went through the series, so I hadn't > digested your hotplug use case yet :) > > But yes - I don't like having the rwlock itself be recursive, but I do > understand that you have a legitimate requirement for > get_online_cpus_atomic() to be recursive. This IMO points to the > direction I suggested, of explicitly handling the recusion in > get_online_cpus_atomic() so that the underlying rwlock doesn't have to > support recursive reader side itself. > > (And this would work for the idea of making writers own the reader > side as well - you can do it with the hotplug_recursion_count instead > of with the underlying rwlock). > >> While your idea above looks good, it might introduce more complexity >> in the unlock path, since this would allow nesting of heterogeneous readers >> (ie., if hotplug_recursion_count == 1, you don't know whether you need to >> simply decrement the counter or unlock the rwlock as well). > > Well, I think the idea doesn't make the underlying rwlock more > complex, since you could in principle keep your existing > percpu_read_lock_irqsafe implementation as is and just remove the > recursive behavior from its documentation. > > Now ideally if we're adding a bit of complexity in > get_online_cpus_atomic() it'd be nice if we could remove some from > percpu_read_lock_irqsafe, but I haven't thought about that deeply > either. I think you'd still want to have the equivalent of a percpu > reader_refcnt, except it could now be a bool instead of an int, and > percpu_read_lock_irqsafe would still set it to back to 0/false after > acquiring the global read side if a writer is signaled. Basically your > existing percpu_read_lock_irqsafe code should still work, and we could > remove just the parts that were only there to deal with the recursive > property. > But, the whole intention behind removing the parts depending on the recursive property of rwlocks would be to make it easier to make rwlocks fair (going forward) right? Then, that won't work for CPU hotplug, because, just like we have a legitimate reason to have recursive get_online_cpus_atomic(), we also have a legitimate reason to have unfairness in locking (i.e., for deadlock-safety). So we simply can't afford to make the locking fair - we'll end up in too many deadlock possibilities, as hinted in the changelog of patch 1. (Remember, we are replacing preempt_disable(), which had absolutely no special nesting rules or locking implications. That is why we are forced to provide maximum locking flexibility and safety against new/previously non-existent deadlocks, in the new synchronization scheme). So the only long-term solution I can think of is to decouple percpu-rwlocks and rwlock_t (like what Tejun suggested) by implementing our own unfair locking scheme inside. What do you think? Regards, Srivatsa S. Bhat From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e28smtp04.in.ibm.com (e28smtp04.in.ibm.com [122.248.162.4]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "e28smtp04.in.ibm.com", Issuer "GeoTrust SSL CA" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 867B82C0291 for ; Tue, 19 Feb 2013 05:53:01 +1100 (EST) Received: from /spool/local by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 19 Feb 2013 00:20:33 +0530 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 31476394004D for ; Tue, 19 Feb 2013 00:22:56 +0530 (IST) Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r1IIqrgp33816576 for ; Tue, 19 Feb 2013 00:22:53 +0530 Received: from d28av04.in.ibm.com (loopback [127.0.0.1]) by d28av04.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r1IIqsOL013002 for ; Tue, 19 Feb 2013 05:52:55 +1100 Message-ID: <51227810.6090009@linux.vnet.ibm.com> Date: Tue, 19 Feb 2013 00:20:56 +0530 From: "Srivatsa S. Bhat" MIME-Version: 1.0 To: Michel Lespinasse Subject: Re: [PATCH v6 08/46] CPU hotplug: Provide APIs to prevent CPU offline from atomic context References: <20130218123714.26245.61816.stgit@srivatsabhat.in.ibm.com> <20130218123920.26245.56709.stgit@srivatsabhat.in.ibm.com> <51225A36.40600@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Cc: linux-doc@vger.kernel.org, peterz@infradead.org, fweisbec@gmail.com, linux-kernel@vger.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, namhyung@kernel.org, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, oleg@redhat.com, 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 02/18/2013 10:51 PM, Michel Lespinasse wrote: > On Tue, Feb 19, 2013 at 12:43 AM, Srivatsa S. Bhat > wrote: >> On 02/18/2013 09:53 PM, Michel Lespinasse wrote: >>> I am wondering though, if you could take care of recursive uses in >>> get/put_online_cpus_atomic() instead of doing it as a property of your >>> rwlock: >>> >>> get_online_cpus_atomic() >>> { >>> unsigned long flags; >>> local_irq_save(flags); >>> if (this_cpu_inc_return(hotplug_recusion_count) == 1) >>> percpu_read_lock_irqsafe(&hotplug_pcpu_rwlock); >>> local_irq_restore(flags); >>> } >>> >>> Once again, the idea there is to avoid baking the reader side >>> recursive properties into your rwlock itself, so that it won't be >>> impossible to implement reader/writer fairness into your rwlock in the >>> future (which may be not be very important for the hotplug use, but >>> could be when other uses get introduced). >> >> Hmm, your proposal above looks good to me, at first glance. >> (Sorry, I had mistaken your earlier mails to mean that you were against >> recursive reader-side, while you actually meant that you didn't like >> implementing the recursive reader-side logic using the recursive property >> of rwlocks). > > To be honest, I was replying as I went through the series, so I hadn't > digested your hotplug use case yet :) > > But yes - I don't like having the rwlock itself be recursive, but I do > understand that you have a legitimate requirement for > get_online_cpus_atomic() to be recursive. This IMO points to the > direction I suggested, of explicitly handling the recusion in > get_online_cpus_atomic() so that the underlying rwlock doesn't have to > support recursive reader side itself. > > (And this would work for the idea of making writers own the reader > side as well - you can do it with the hotplug_recursion_count instead > of with the underlying rwlock). > >> While your idea above looks good, it might introduce more complexity >> in the unlock path, since this would allow nesting of heterogeneous readers >> (ie., if hotplug_recursion_count == 1, you don't know whether you need to >> simply decrement the counter or unlock the rwlock as well). > > Well, I think the idea doesn't make the underlying rwlock more > complex, since you could in principle keep your existing > percpu_read_lock_irqsafe implementation as is and just remove the > recursive behavior from its documentation. > > Now ideally if we're adding a bit of complexity in > get_online_cpus_atomic() it'd be nice if we could remove some from > percpu_read_lock_irqsafe, but I haven't thought about that deeply > either. I think you'd still want to have the equivalent of a percpu > reader_refcnt, except it could now be a bool instead of an int, and > percpu_read_lock_irqsafe would still set it to back to 0/false after > acquiring the global read side if a writer is signaled. Basically your > existing percpu_read_lock_irqsafe code should still work, and we could > remove just the parts that were only there to deal with the recursive > property. > But, the whole intention behind removing the parts depending on the recursive property of rwlocks would be to make it easier to make rwlocks fair (going forward) right? Then, that won't work for CPU hotplug, because, just like we have a legitimate reason to have recursive get_online_cpus_atomic(), we also have a legitimate reason to have unfairness in locking (i.e., for deadlock-safety). So we simply can't afford to make the locking fair - we'll end up in too many deadlock possibilities, as hinted in the changelog of patch 1. (Remember, we are replacing preempt_disable(), which had absolutely no special nesting rules or locking implications. That is why we are forced to provide maximum locking flexibility and safety against new/previously non-existent deadlocks, in the new synchronization scheme). So the only long-term solution I can think of is to decouple percpu-rwlocks and rwlock_t (like what Tejun suggested) by implementing our own unfair locking scheme inside. What do you think? 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: Tue, 19 Feb 2013 00:20:56 +0530 Subject: [PATCH v6 08/46] CPU hotplug: Provide APIs to prevent CPU offline from atomic context In-Reply-To: References: <20130218123714.26245.61816.stgit@srivatsabhat.in.ibm.com> <20130218123920.26245.56709.stgit@srivatsabhat.in.ibm.com> <51225A36.40600@linux.vnet.ibm.com> Message-ID: <51227810.6090009@linux.vnet.ibm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 02/18/2013 10:51 PM, Michel Lespinasse wrote: > On Tue, Feb 19, 2013 at 12:43 AM, Srivatsa S. Bhat > wrote: >> On 02/18/2013 09:53 PM, Michel Lespinasse wrote: >>> I am wondering though, if you could take care of recursive uses in >>> get/put_online_cpus_atomic() instead of doing it as a property of your >>> rwlock: >>> >>> get_online_cpus_atomic() >>> { >>> unsigned long flags; >>> local_irq_save(flags); >>> if (this_cpu_inc_return(hotplug_recusion_count) == 1) >>> percpu_read_lock_irqsafe(&hotplug_pcpu_rwlock); >>> local_irq_restore(flags); >>> } >>> >>> Once again, the idea there is to avoid baking the reader side >>> recursive properties into your rwlock itself, so that it won't be >>> impossible to implement reader/writer fairness into your rwlock in the >>> future (which may be not be very important for the hotplug use, but >>> could be when other uses get introduced). >> >> Hmm, your proposal above looks good to me, at first glance. >> (Sorry, I had mistaken your earlier mails to mean that you were against >> recursive reader-side, while you actually meant that you didn't like >> implementing the recursive reader-side logic using the recursive property >> of rwlocks). > > To be honest, I was replying as I went through the series, so I hadn't > digested your hotplug use case yet :) > > But yes - I don't like having the rwlock itself be recursive, but I do > understand that you have a legitimate requirement for > get_online_cpus_atomic() to be recursive. This IMO points to the > direction I suggested, of explicitly handling the recusion in > get_online_cpus_atomic() so that the underlying rwlock doesn't have to > support recursive reader side itself. > > (And this would work for the idea of making writers own the reader > side as well - you can do it with the hotplug_recursion_count instead > of with the underlying rwlock). > >> While your idea above looks good, it might introduce more complexity >> in the unlock path, since this would allow nesting of heterogeneous readers >> (ie., if hotplug_recursion_count == 1, you don't know whether you need to >> simply decrement the counter or unlock the rwlock as well). > > Well, I think the idea doesn't make the underlying rwlock more > complex, since you could in principle keep your existing > percpu_read_lock_irqsafe implementation as is and just remove the > recursive behavior from its documentation. > > Now ideally if we're adding a bit of complexity in > get_online_cpus_atomic() it'd be nice if we could remove some from > percpu_read_lock_irqsafe, but I haven't thought about that deeply > either. I think you'd still want to have the equivalent of a percpu > reader_refcnt, except it could now be a bool instead of an int, and > percpu_read_lock_irqsafe would still set it to back to 0/false after > acquiring the global read side if a writer is signaled. Basically your > existing percpu_read_lock_irqsafe code should still work, and we could > remove just the parts that were only there to deal with the recursive > property. > But, the whole intention behind removing the parts depending on the recursive property of rwlocks would be to make it easier to make rwlocks fair (going forward) right? Then, that won't work for CPU hotplug, because, just like we have a legitimate reason to have recursive get_online_cpus_atomic(), we also have a legitimate reason to have unfairness in locking (i.e., for deadlock-safety). So we simply can't afford to make the locking fair - we'll end up in too many deadlock possibilities, as hinted in the changelog of patch 1. (Remember, we are replacing preempt_disable(), which had absolutely no special nesting rules or locking implications. That is why we are forced to provide maximum locking flexibility and safety against new/previously non-existent deadlocks, in the new synchronization scheme). So the only long-term solution I can think of is to decouple percpu-rwlocks and rwlock_t (like what Tejun suggested) by implementing our own unfair locking scheme inside. What do you think? Regards, Srivatsa S. Bhat