LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Pavel Tatashin <pasha.tatashin@soleen.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Kees Cook <keescook@chromium.org>,
	Anton Vorontsov <anton@enomsg.org>,
	Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Rob Herring <robh+dt@kernel.org>,
	Benson Leung <bleung@chromium.org>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	James Morris <jmorris@namei.org>, Sasha Levin <sashal@kernel.org>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events
Date: Tue, 12 May 2020 12:49:10 -0400
Message-ID: <CA+CK2bC0argMNHzynedpwN6ekOg8yypN03JvmAKGWQ5Aegxh+Q@mail.gmail.com> (raw)
In-Reply-To: <20200512155207.GF17734@linux-b0ei>

On Tue, May 12, 2020 at 11:52 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2020-05-12 10:03:44, Pavel Tatashin wrote:
> > > OK, I personally see this as two separate problems:
> > >
> > >    1. Missing support to set loglevel per console.
> > >    2. Missing support to dump messages for other reasons.
> > >
> > > I would remove the paragraph about console log levels completely.
> >
> > OK, I see your point, this paragraph can be removed, however, I think
> > it makes it clear to understand the rationale for this change. As I
> > understand, the per console loglevel has been proposed but were never
> > accepted.
>
> The proposal was not accepted because there were more requirements:
>
>    + add console device into sysfs so that it can be modified there
>    + make a reasonable backward compatible behavior
>
> I guess that the sysfs interface discouraged the author to continue
> on it.
>
> Note that console loglevel handling is very complicated. There are
> already four variables, see console_printk array in
> kernel/printk/printk.c. Per console loglevel would make it even
> more complicated.
>
> It is a nighmare. And introducing max_reason parameter goes the same way.
>
> > > Now, the max_reason logic makes sense only when all the values
> > > have some ordering. Is this the case?
> > >
> > > I see it as two distinct sets:
> > >
> > >    + panic, oops, emerg: describe how critical is an error situation
> > >    + restart, halt, poweroff: describe behavior when the system goes down
> > >
> > > Let's say that panic is more critical than oops. Is restart more
> > > critical than halt?
> > >
> > > If you want the dump during restart. Does it mean that you want it
> > > also during emergency situation?
> > >
> > > My fear is that this patchset is going to introduce user interface
> > > (max_reason) with a weird logic. IMHO, max_reason is confusing even
> > > in the code and we should not spread this to users.
> > >
> > > Is there any reason why the existing printk.always_kmsg_dump option
> > > is not enough for you?
> >
> > printk.always_kmsg_dump is not working for me because ramoops has its
> > own filtering based on dump_oops boolean, and ignores everything but
> > panics and conditionally oops.
> > max_reason makes the ramoops internal logic cleaner compared to using dump_oops.
>
> I see. Just to be sure. Is the main reason to add max_reason parameter
> to keep complatibility of the deprecated dump_oops parameter? Or is
> there any real use case for this granularity?
>
> I made some arecheology. ramoops.dump_oops parameter was added in 2010 by the
> initial commit 56d611a04fb2db77334e ("char drivers: RAM oops/panic
> logger."
>
> Note that the initial implementation printed Oops reason only when
> dump_oops was set. It printed all other reasons otherwise. It seems
> that there were only the two reasons at that time.
>
> Now, printk.always_kmsg_dump parameter was added later in 2012 by
> the commit c22ab332902333f8376601 ("kmsg_dump: don't run on non-error
> paths by default").
>
> IMHO, the later commit actually fixed the default behavior of ramoops.
>
> I wonder if anyone is actually using the ramoops.dump_oops parameter
> in reality. I would personally make it deprecated and change the
> default behavior to work according to printk.always_kmsg_dump parameter.

This sounds alright to me with one slight problem. I am doing this
work for an embedded arm64 SoC, so controlling everything via device
tree is preferable compared to having some settings via device tree
and others via kernel parameters, especially because the kernel
parameters are hardcoded by firmware that we try not to update too
often for uptime reasons.

>
> IMHO, ramoops.dump_oops just increases complexity and should not have
> been introduced at all. I would try hard to avoid introducing even bigger
> complecity and mess.

I agree, amoops.dump_oops should be depricated with or without
max_reason change.

>
> I know that there is the "do not break existing userspace" rule. The
> question is if there is any user and if it is worth it.
>
> > I agree, the reasons in kmsg_dump_reason do not order well  (I
> > actually want to add another reason for kexec type reboots, and where
> > do I put it?), so how about if we change the ordering list to
> > bitfield/flags, and instead of max_reason provide: "reasons" bitset?
>
> It looks too complicated. I would really try hard to avoid the
> parameter at all.

OK. Should we remove max_reason from struct kmsg_dumper and also
remove the misleading comment about kmsg_dump_reason ordering?

Thank you,
Pasha

  parent reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-06 21:15 Kees Cook
2020-05-06 21:15 ` [PATCH v3 1/6] printk: honor the max_reason field in kmsg_dumper Kees Cook
2020-05-06 21:15 ` [PATCH v3 2/6] pstore/platform: Pass max_reason to kmesg dump Kees Cook
2020-05-06 21:25   ` Joe Perches
2020-05-06 22:40     ` Kees Cook
2020-05-06 21:15 ` [PATCH v3 3/6] pstore/ram: Refactor DT size parsing Kees Cook
2020-05-07 12:57   ` Pavel Tatashin
2020-05-07 18:04     ` Kees Cook
2020-05-06 21:15 ` [PATCH v3 4/6] pstore/ram: Introduce max_reason and convert dump_oops Kees Cook
2020-05-06 21:17   ` Kees Cook
2020-05-12 23:35   ` Tyler Hicks
2020-05-12 23:57     ` Kees Cook
2020-05-16  2:43     ` Kees Cook
2020-05-06 21:15 ` [PATCH v3 5/6] ramoops: Add max_reason optional field to ramoops DT node Kees Cook
2020-05-06 21:15 ` [PATCH v3 6/6] pstore/ram: Adjust module param permissions to reflect reality Kees Cook
2020-05-12 13:16 ` [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events Petr Mladek
2020-05-12 14:03   ` Pavel Tatashin
2020-05-12 15:52     ` Petr Mladek
2020-05-12 16:03       ` Steven Rostedt
2020-05-12 16:49       ` Pavel Tatashin [this message]
2020-05-12 18:53         ` Kees Cook
2020-05-12 18:45       ` Kees Cook
2020-05-13  7:34         ` Petr Mladek
2020-05-13  7:47           ` Kees Cook
2020-05-13 14:35             ` Pavel Tatashin
2020-05-13 20:15               ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+CK2bC0argMNHzynedpwN6ekOg8yypN03JvmAKGWQ5Aegxh+Q@mail.gmail.com \
    --to=pasha.tatashin@soleen.com \
    --cc=anton@enomsg.org \
    --cc=bleung@chromium.org \
    --cc=ccross@android.com \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=enric.balletbo@collabora.com \
    --cc=jmorris@namei.org \
    --cc=keescook@chromium.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pmladek@suse.com \
    --cc=robh+dt@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=sashal@kernel.org \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=tony.luck@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git