Date:   Fri, 14 Jul 2017 09:14:11 -0700
From:   Joe Perches <joe@...ches.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] printk: Move printk_delay to separate file

On Fri, 2017-07-14 at 17:57 +0200, Petr Mladek wrote:
> On Fri 2017-07-07 11:08:59, Joe Perches wrote:
> > printk.c is a huge file with too many local functions for a
> > human to read and easily parse.
> > 
> > Start to separate out bits into smaller files.
> > 
> > Miscellanea:
> > 
> > o Rename suppress_message_printing to printk_suppress_message
> 
> Some renaming might make sense. But please, do it in a separate patch.
> It will make the changes much easier to review.

> Some existing names are ugly. But there should be a good reason
> to rename them, for example, that the existing one causes confusion.
> We should be careful here. Otherwise, it will be a nightmare to
> search the history or backport important fixes.

Disagree.


> Also note that some variables are exported a strange way for
> external utilities line crash, makedumpfile, see
> log_buf_vmcoreinfo_setup(). Renaming such variables would
> require fixing all the external applications.

I believe the only ones that matters are the
log_buf_vmcore<foo>

I already renamed those once before without a
single peep from anyone.

> > o Add function definitions to printk.h
> 
> We should not declare functions in a global header without a reason.
> It would allow to use them anywhere and it is not always intended
> or even safe.

IMO: printk.h isn't really a global header.

I argued against calling it a global header
when I created it.

There needs to be some mechanism, even if
perhaps it's local to kernel/printk/, to share
these symbols.

> If we want to make some function available in global, we should
> do so in a separate patch with a reasonable explanation.
> 
> If we want to share them between kernel/printk/*.c sources,
> we should declare them in a local header, e.g.
> kernel/printk/internal.h

internal.h would be a terrible name.

Specify what each bit is used for not
how it's used.

> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index fc47863f629c..b8e63a5f1558 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1709,8 +1640,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> >  		in_sched = true;
> >  	}
> >  
> > -	boot_delay_msec(level);
> > -	printk_delay();
> > +	printk_delay(level);
> 
> This makes me nervous.

It shouldn't.  It's trivial.

The boot delay is a detail that
shouldn't even be exposed anywhere
let alone in the vprintk path.

> It is not even documented.

printk is completely undocumented.
Think about how long it took you to even
partially understand it and how often you
add defects and need rework to the existing
bits.

> Please, do not do
> any hidden changes in general! The patch that splits the sources
> should only shuffle the code.

That's not possible.

>  It might do only some minimal
> necessary changes like removing "static" for functions that
> newly need to be accessed from other source file.
> 
> Best Regards,
> Petr
> 
> PS: Please, split only the console stuff as a start. It will be
> the most helpful thing. Also we are still maintainers beginners.

No.  There's far too many symbols that would need to be
renamed for that as a start.  You're already objecting
to trivial stuff.  console is complex and involved.

> I would like to start slowly, preferably get some feedback from
> more experienced developers, and get one such change into the
> mainline before we do many more changes.

I believe I am one such experienced developer.

Powered by blists - more mailing lists