From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755263AbcHWDmA (ORCPT ); Mon, 22 Aug 2016 23:42:00 -0400 Received: from rhlx01.hs-esslingen.de ([129.143.116.10]:33768 "EHLO rhlx01.hs-esslingen.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752487AbcHWDl7 (ORCPT ); Mon, 22 Aug 2016 23:41:59 -0400 X-Greylist: delayed 585 seconds by postgrey-1.27 at vger.kernel.org; Mon, 22 Aug 2016 23:41:59 EDT Date: Tue, 23 Aug 2016 05:32:02 +0200 From: Andreas Mohr To: Sergey Senozhatsky , Jan Kara , Petr Mladek , linux-kernel@vger.kernel.org Subject: Re: [PATCH v10 1/2] printk: Make printk() completely async Message-ID: <20160823033202.GA12229@rhlx01.hs-esslingen.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Priority: none User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [I *AGAIN* seem to have lost access to linux-kernel subscription, thus no proper In-Reply-To: reply possible, ARGH, sorry] Some random yet maybe still helpful observations from reading this mail content (Sat, 20 Aug 2016 14:24:30 +0900) and reviewing trunk printk.c: . vprintk_emit() static variables are not grouped (logbuf_cpu plus comment should probably be near other static:s since they are related, and perhaps add a separation line after static:s group to emphasize the distinction) . many, many ugly static:s sitting in this file (build unit), i.e. no proper instance definition, with the result of e.g. being unable to easily convert things into log-multi-instance handling (not necessarily required, admittedly). Probably should group related static:s into helper struct:s instead and then instantiate these struct:s, to be passed as "session instance this" into properly *non*-free-running log functions (prime free-running functions where this is obvious are e.g. logbuf_has_space(), log_make_free_space()). Having hard-coded static:s sitting at random locations within a file IMHO simply is *MUCH* less flexible/clean than having things properly struct-*grouped* and thus flexibly *reusable*/*rearrangeable*. Note that there also is the verbose/lengthy comment /* * The logbuf_lock protects kmsg buffer, indices, counters. This can be taken * within the scheduler's rq lock. It must be released before calling * console_unlock() or anything else that might wake up a process. */ which would immediately lose large parts of its reason for existence once things were done in a clean/structured/elegant/self-documenting manner where all/many lock-affected variables (quote: "kmsg buffer, indices, counters") would be grouped in one/few properly component-grouped struct:s and where an *outer* struct definition (to be passed as an "instance this" to implementation functions) would then be used to treat/cure the existing multi-context/multi-threading access disease, by containing one/few members of the struct(s) and one *sibling* lock member, for access to these inner lock-free implementation struct:s in those unfortunate use situations where we need to deal with multi-context access and thus a lock needs to be used. --> immediately visible *cleanly structured* implementation hierarchy, plus the *reason* for this hierarchy. Performance aspects IMHO shouldn't play much of a role here, despite this being printk implementation code (inefficiencies caused by modernization delay due to insufficiently modernizable/layerable code play a much larger role IMHO) Put differently: the file seems somewhat chaotic (i.e., with sufficiently large amounts of unrelated/distracting noise within relevant scope width of developer examination efforts) to sufficiently *prevent* people from easily reviewing it and thus being able to come to implementation modernization conclusions in a much faster manner, thus it likely should better be refactored prior to messing up/complicating things while trying to solve some behaviour issues. I'm usually focussing on achieving non-redundancy / hierarchy *symmetry* / consistency - with some minor luck important treatment clues will then follow easily, due to much reduced total implementation examination scope. . boot_delay_msec(): . system_state != SYSTEM_BOOTING conditional has wrong evaluation order, which leads to configurations with boot_delay != 0 having performance characteristics *different* (always needs to check one conditional more!) from == 0 configurations (once system_state has proceeded beyond SYSTEM_BOOTING i.e. *normal*(!) system operation, that is) . time_after(): perhaps it would be useful to use ktime (non-jiffies-based) APIs instead? (open questions: algorithmic performance, dependency availability at boot time?) . the entire code in printk.c seems very busy/complex (as can also be witnessed via the impressively[not?] long list of #include:s) - quite likely the code is doing some annoying layer violations, seemingly drawing in as many dependencies as it can, rather than implementing tiny, focussed helper functions which then would get invoked by an *outer*, *aggregating* layer which simply knows how to invoke these functions due to consulting all relevant global, aggregation-related configuration aspects *before* invoking these implementation-focussed functions; prime sample: vprintk_emit(), with its weirdly scheduler-dependency-affected epilogue parts - quite likely it should do internal cleanly targeted queuing-only *layer implementation* stuff, with "wakeup"/"scheduler"-related dependency crap then being done *outside* of this focussed helper, at suitable crucial situations/times during system management; it all boils down to a matter of: which distinct dependency components do we have in our system, and how do we *avoid* having them all reference each other (and Santa Claus) in an overly complex manner, and with several global/*outer* system configuration state parts then also *internally* interspersed to boot? [printk_delay() is another minor example of dependency inversion, where the global/*outer* aggregation-related configuration aspect printk_delay_msec is handled *internally*, i.e. a probably unholy inversion against properly separated implementation hierarchies] . use of logbuf_lock seems to be randomly spread all over the file, rather than being locally contained in one linear block of small, reviewable number of clean, concise "logbuf_lock-augmenting layer" helper functions . dump_stack_print_info() / show_regs_print_info() (etc.?) seem to be *users* of a successful printk() *implementation* and thus most certainly should not have any business whatsoever also sitting within the (already sufficiently complex / unreviewable - 3.3k lines!!) printk() *implementation mechanics* file HTH & HAND, Andreas Mohr