Re: [PATCH v10 1/2] printk: Make printk() completely async

[Andreas Mohr <andi@lisas.de>]

[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