From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>,
Thomas Gleixner <tglx@linutronix.de>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH next v1 1/3] printk: track/limit recursion
Date: Wed, 24 Mar 2021 09:41:13 +0100 [thread overview]
Message-ID: <YFr7KSwgX/WexbXy@alley> (raw)
In-Reply-To: <87czvp7e0f.fsf@jogness.linutronix.de>
On Tue 2021-03-23 22:32:00, John Ogness wrote:
> On 2021-03-22, Petr Mladek <pmladek@suse.com> wrote:
> > On Wed 2021-03-17 00:33:24, John Ogness wrote:
> >> Track printk() recursion and limit it to 3 levels per-CPU and per-context.
> >
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 2f829fbf0a13..c666e3e43f0c 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> +/* Return a pointer to the dedicated counter for the CPU+context of the caller. */
> >> +static char *printk_recursion_counter(void)
> >> +{
> >> + int ctx = 0;
> >> +
> >> +#ifdef CONFIG_PRINTK_NMI
> >> + if (in_nmi())
> >> + ctx = 1;
> >> +#endif
> >> + if (!printk_percpu_data_ready())
> >> + return &printk_count_early[ctx];
> >> + return &((*this_cpu_ptr(&printk_count))[ctx]);
> >> +}
> >
> > It is not a big deal. But using an array for two contexts looks strange
> > especially when only one is used on some architectures.
> > Also &((*this_cpu_ptr(&printk_count))[ctx]) is quite tricky ;-)
> >
> > What do you think about the following, please?
> >
> > static DEFINE_PER_CPU(u8 printk_count);
> > static u8 printk_count_early;
> >
> > #ifdef CONFIG_HAVE_NMI
> > static DEFINE_PER_CPU(u8 printk_count_nmi);
> > static u8 printk_count_nmi_early;
> > #endif
> >
> > static u8 *printk_recursion_counter(void)
> > {
> > if (IS_ENABLED(CONFIG_HAVE_NMI) && in_nmi()) {
> > if (printk_cpu_data_ready())
> > return this_cpu_ptr(&printk_count_nmi);
> > return printk_count_nmi_early;
> > }
> >
> > if (printk_cpu_data_ready())
> > return this_cpu_ptr(&printk_count);
> > return printk_count_early;
> > }
>
> I can split it into explicit variables. But is the use of the IS_ENABLED
> macro preferred over ifdef? I would prefer:
>
> static u8 *printk_recursion_counter(void)
> {
> #ifdef CONFIG_HAVE_NMI
> if (in_nmi()) {
> if (printk_cpu_data_ready())
> return this_cpu_ptr(&printk_count_nmi);
> return printk_count_nmi_early;
> }
> #endif
> if (printk_cpu_data_ready())
> return this_cpu_ptr(&printk_count);
> return printk_count_early;
> }
>
> Since @printk_count_nmi and @printk_count_nmi_early would not exist, I
> would prefer the pre-processor removes that code block rather than
> relying on compiler optimization.
Feel free to use #ifdef.
Best Regards,
Petr
next prev parent reply other threads:[~2021-03-24 8:42 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-16 23:33 [PATCH next v1 0/3] printk: remove safe buffers John Ogness
2021-03-16 23:33 ` [PATCH next v1 1/3] printk: track/limit recursion John Ogness
2021-03-21 5:34 ` Sergey Senozhatsky
2021-03-22 10:53 ` John Ogness
2021-03-22 11:13 ` Sergey Senozhatsky
2021-03-22 15:07 ` Petr Mladek
2021-03-22 14:49 ` Petr Mladek
2021-03-23 21:32 ` John Ogness
2021-03-24 8:41 ` Petr Mladek [this message]
2021-03-16 23:33 ` [PATCH next v1 2/3] printk: remove safe buffers John Ogness
2021-03-21 5:26 ` Sergey Senozhatsky
2021-03-22 11:16 ` John Ogness
2021-03-22 18:02 ` Petr Mladek
2021-03-22 21:58 ` John Ogness
2021-03-23 9:46 ` Petr Mladek
2021-03-23 10:47 ` Petr Mladek
2021-03-26 11:12 ` John Ogness
2021-03-29 10:04 ` Petr Mladek
2021-03-29 15:10 ` John Ogness
2021-03-29 15:13 ` John Ogness
2021-03-16 23:33 ` [PATCH next v1 3/3] printk: convert @syslog_lock to spin_lock John Ogness
2021-03-23 12:01 ` Petr Mladek
2021-03-26 11:23 ` John Ogness
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=YFr7KSwgX/WexbXy@alley \
--to=pmladek@suse.com \
--cc=john.ogness@linutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=sergey.senozhatsky.work@gmail.com \
--cc=sergey.senozhatsky@gmail.com \
--cc=tglx@linutronix.de \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).