From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755832AbbE2K4U (ORCPT ); Fri, 29 May 2015 06:56:20 -0400 Received: from cantor2.suse.de ([195.135.220.15]:48865 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755510AbbE2K4L (ORCPT ); Fri, 29 May 2015 06:56:11 -0400 Date: Fri, 29 May 2015 12:56:07 +0200 From: Petr Mladek To: Andrew Morton Cc: Frederic Weisbecker , Steven Rostedt , Dave Anderson , "Paul E. McKenney" , Kay Sievers , Jiri Kosina , Michal Hocko , Jan Kara , linux-kernel@vger.kernel.org, Wang Long , peifeiyue@huawei.com, dzickus@redhat.com, morgan.wang@huawei.com, sasha.levin@oracle.com Subject: Re: [PATCH 02/10] printk: Try harder to get logbuf_lock on NMI Message-ID: <20150529105607.GG3135@pathway.suse.cz> References: <1432557993-20458-1-git-send-email-pmladek@suse.cz> <1432557993-20458-3-git-send-email-pmladek@suse.cz> <20150527161409.c1c37d25af1c09c340bd04a3@linux-foundation.org> <20150528135054.GF3135@pathway.suse.cz> <20150528130944.9dde0f591a18d656f2a7c519@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150528130944.9dde0f591a18d656f2a7c519@linux-foundation.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 2015-05-28 13:09:44, Andrew Morton wrote: > On Thu, 28 May 2015 15:50:54 +0200 Petr Mladek wrote: > > > > > +{ > > > > + u64 start_time, current_time; > > > > + int this_cpu = smp_processor_id(); > > > > + > > > > + /* no way if we are already locked on this CPU */ > > > > + if (logbuf_cpu == this_cpu) > > > > + return 0; > > > > + > > > > + /* try hard to get the lock but do not wait forever */ > > > > + start_time = cpu_clock(this_cpu); > > > > + current_time = start_time; > > > > + while (current_time - start_time < TRY_LOCKBUF_LOCK_MAX_DELAY_NS) { > > > > + if (raw_spin_trylock(&logbuf_lock)) > > > > + return 1; > > > > + cpu_relax(); > > > > + current_time = cpu_clock(this_cpu); > > > > + } > > > > > > (Looks at the read_seqcount_retry() in > > > kernel/time/sched_clock.c:sched_clock()) > > > > > > Running cpu_clock() in NMI context seems a generally bad idea. > > > > I am sorry but this is too cryptic for me :-) > > read_seqcount_retry() looks safe to me under NMI. > > hmpf. If you guys say so... > > Note that it's not just a matter of "safe to call from NMI context". > The above loop also assume that cpu_clock() is *being updated* within > the context of single NMI. Is that true/safe now and in the future? > Probably. I didn't check all architectures but ARM looks OK at present. > > We should at least update Documentation/timers/timekeeping.txt: "a sane > value" becomes "the correct value", no alternatives. > > > > There are many sites in kernel/printk/printk.c which take logbuf_lock, > > > but this patch only sets logbuf_cpu in one of those cases: > > > vprintk_emit(). I suggest adding helper functions to take/release > > > logbuf_lock. And rename logbuf_lock to something else to ensure that > > > nobody accidentally takes the lock directly. > > > > IMHO, vprintk_emit() is special. It is the only location where the > > lock is taken in NMI context. The other functions are used to dump > > @logbuf and are called in normal context. > > > > try_logbuf_lock_in_nmi() could fail and we need to handle the error > > path. We do not need to do this in the other locations. > > > > Note that we do not want to get the console in NMI because > > there are even more locks that might cause a deadlock. > > Consider the case where a CPU has taken logbuf_lock within > devkmsg_read() and then receives an NMI, from which it calls > try_logbuf_lock_in_nmi(): I am not sure that I understand. My point is that we do not call devkmsg_read() from NMI context, so we do not need to use try_logbuf_lock_in_nmi() there. IMHO, the same is true for all other locations except for vprintk_emit(). > > +/* We must be careful in NMI when we managed to preempt a running printk */ > > +static int try_logbuf_lock_in_nmi(void) > > +{ > > + u64 start_time, current_time; > > + int this_cpu = smp_processor_id(); > > + > > + /* no way if we are already locked on this CPU */ > > + if (logbuf_cpu == this_cpu) > > + return 0; Or do you have this check in mind? It will detect the deadlock immediately but @logbuf_cpu is set only in vprintk_emit(). We will spin when NMI comes inside the other functions, e.g. devkmsg_read(). > > + /* try hard to get the lock but do not wait forever */ > > + start_time = cpu_clock(this_cpu); > > + current_time = start_time; > > + while (current_time - start_time < TRY_LOCKBUF_LOCK_MAX_DELAY_NS) { > > + if (raw_spin_trylock(&logbuf_lock)) > > + return 1; > > + cpu_relax(); > > + current_time = cpu_clock(this_cpu); > > + } > > + > > + return 0; > > +} > > That CPU is now going to spin around for 100us and then time out. Yes, there was a deadlock without the patch. So, limited spinning is still a win. Or would you like to detect the deadlock immediately in all cases? I mean to add the proposed wrapper around take/release lock calls and set/test some cpu-specific variable there? It sounds interesting. Well, the detection will not be 100% correct because there is a small race window between taking @logbuf_lock and setting @lockbuf_cpu. I wonder if it is worth doing. But I will do it if you want. Best Regards, Petr