From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935110AbcATEQ7 (ORCPT ); Tue, 19 Jan 2016 23:16:59 -0500 Received: from mail-pf0-f196.google.com ([209.85.192.196]:36330 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934328AbcATEQw (ORCPT ); Tue, 19 Jan 2016 23:16:52 -0500 Date: Wed, 20 Jan 2016 13:18:04 +0900 From: Sergey Senozhatsky To: Petr Mladek Cc: Sergey Senozhatsky , Sergey Senozhatsky , Andrew Morton , Tejun Heo , Jan Kara , Kyle McMartin , Dave Jones , Calvin Owens , linux-kernel@vger.kernel.org Subject: Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of console_trylock_for_printk Message-ID: <20160120041804.GB506@swordfish> References: <1452747443-9927-1-git-send-email-sergey.senozhatsky@gmail.com> <1452747443-9927-2-git-send-email-sergey.senozhatsky@gmail.com> <20160118154228.GY3178@pathway.suse.cz> <20160119004236.GA4963@swordfish> <20160119133136.GA2148@dhcp128.suse.cz> <20160119150040.GA558@swordfish> <20160119161656.GC2148@dhcp128.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160119161656.GC2148@dhcp128.suse.cz> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On (01/19/16 17:16), Petr Mladek wrote: > On Wed 2016-01-20 00:00:40, Sergey Senozhatsky wrote: > > > But do we really need to repeat the check in every cycle? > > > > well, on every iteration in the best case we check cpu_online() > > only. which is what we would have done anyway in vprintk_emit(), > > so no additional checks added. at the same time call_console_drivers > > does not do '!cpu_online && !CON_ANYTIME' for each console now, so > > in some sense there are less checks now (this is far even from a > > micro-optimization, just noted). > > Hmm, we need to keep the check in call_console_drivers(). It iterates > over all registered consoles. Only some of them could habe CON_ANYTIME > flag. We need to skip the others when the CPU is not online. oh... good point, you are right! my bad. so we basically need both. the first one (can_use_console() before call_console_drivers()) ensures that it's _generally_ OK to call call_console_drivers() later and that it will not drain messages, the second one _in_ call_console_drivers() filters out only CON_ANYTIME consoles if !cpu_online(), but by this time we are sure that there is at least one CON_ANYTIME console. [..] > > when we up_console_sem(), consoles may appear and disappear, since we > > don't keep the semaphore. Suppose that we have OFFLINE cpu and we had a > > CON_ANYTIME console, but in between up_console_sem--console_trylock > > that single CON_ANYTIME console was removed. So now we have !cpu_online > > and !CON_ANYTIME. > > Ah, I have missed that the console_sem is released/taken before goto > again. Hmm, your proposed solution adds even more twists into the code. > I think how to make it easier. I think that we could move the again: > target and call console_cont_flush() when there is some new content. > It is a corner case, anyway. Then we could do: > > > do_cond_resched = console_may_schedule; > console_may_schedule = 0; > > +again: > + if (!can_use_console()) { > + console_locked = 0; > + up_console_sem(); > + return; > } > > /* flush buffered message fragment immediately to console */ > console_cont_flush(text, sizeof(text)); > -again: > for (;;) { > struct printk_log *msg; > looks better. we do extra IRQ disable/enable (spin lock irq) when we jump to `again' label, but I don't think this introduces any significant overhead. however, if it does, we always can avoid extra console_cont_flush() by simply checking @retry -- it's `false' only once, when we execute this part for the first time in the current console_unlock() call; all goto-jumps imply that @retry is `true'. IOW: bool retry = false; again: can_use_console if (!retry) /* if we jumped to again, retry is true */ console_cont_flush for (;;) { ... } retry = ... if retry && console_trylock() goto retry; > Then we remove this check from console_trylock_for_printk() and > eventually remove this function altogether. yes, that was the plan :) > It means that the code will be easier and protected against the > theoretical race. > > How does that sound, please? sounds good, thanks. > Best Regards, > Petr -ss