From: Petr Mladek <pmladek@suse.com> To: John Ogness <john.ogness@linutronix.de> Cc: "Daniel Thompson" <daniel.thompson@linaro.org>, "Sergey Senozhatsky" <senozhatsky@chromium.org>, "Steven Rostedt" <rostedt@goodmis.org>, "Thomas Gleixner" <tglx@linutronix.de>, linux-kernel@vger.kernel.org, "Michael Ellerman" <mpe@ellerman.id.au>, "Benjamin Herrenschmidt" <benh@kernel.crashing.org>, "Paul Mackerras" <paulus@samba.org>, "Ingo Molnar" <mingo@redhat.com>, "Borislav Petkov" <bp@alien8.de>, x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>, "Jason Wessel" <jason.wessel@windriver.com>, "Douglas Anderson" <dianders@chromium.org>, "Srikar Dronamraju" <srikar@linux.vnet.ibm.com>, "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>, "Chengyang Fan" <cy.fan@huawei.com>, "Christophe Leroy" <christophe.leroy@csgroup.eu>, "Bhaskar Chowdhury" <unixbhaskar@gmail.com>, "Nicholas Piggin" <npiggin@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, "Gustavo A. R. Silva" <gustavoars@kernel.org>, "Peter Zijlstra" <peterz@infradead.org>, linuxppc-dev@lists.ozlabs.org, kgdb-bugreport@lists.sourceforge.net Subject: Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock Date: Wed, 4 Aug 2021 14:31:36 +0200 [thread overview] Message-ID: <YQqIqKjRcNhZSaAZ@alley> (raw) In-Reply-To: <87tuk635rb.fsf@jogness.linutronix.de> On Tue 2021-08-03 17:36:32, John Ogness wrote: > On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote: > >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active) > >> during cpu roundup. This will conflict with the printk cpulock. > > > >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > >> index 3d0c933937b4..1b546e117f10 100644 > >> --- a/kernel/printk/printk.c > >> +++ b/kernel/printk/printk.c > >> @@ -214,6 +215,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, > >> #ifdef CONFIG_SMP > >> static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1); > >> static atomic_t printk_cpulock_nested = ATOMIC_INIT(0); > >> +static unsigned int kgdb_cpu = -1; > > > > Is this the flag to provoke retriggering? It appears to be a write-only > > variable (at least in this patch). How is it consumed? > > Critical catch! Thank you. I am quite unhappy to see these hunks were > accidentally dropped when generating this series. > > @@ -3673,6 +3675,9 @@ EXPORT_SYMBOL(__printk_cpu_trylock); > */ > void __printk_cpu_unlock(void) > { > + bool trigger_kgdb = false; > + unsigned int cpu; > + > if (atomic_read(&printk_cpulock_nested)) { > atomic_dec(&printk_cpulock_nested); > return; > @@ -3683,6 +3688,12 @@ void __printk_cpu_unlock(void) > * LMM(__printk_cpu_unlock:A) > */ > > + cpu = smp_processor_id(); > + if (kgdb_cpu == cpu) { > + trigger_kgdb = true; > + kgdb_cpu = -1; > + } Just in case that this approach is used in the end. This code looks racy. kgdb_roundup_delay() seems to be called in NMI context. NMI might happen at this point and set kgdb_cpu after it was checked. I am afraid that it won't be easy to make this safe using a single global variable. A solution might be a per-CPU variable set by kgdb_roundup_delay() when it owns printk_cpu_lock. __printk_cpu_unlock() would call kgdb_roundup_cpu(cpu) when the variable is set. Nit: The name "kgdb_cpu" is too generic. It is not clear what is so special about this CPU. I would call the per-CPU variable "kgdb_delayed_roundup" or so. Best Regards, Petr > /* > * Guarantee loads and stores from this CPU when it was the > * lock owner are visible to the next lock owner. This pairs > @@ -3703,6 +3714,21 @@ void __printk_cpu_unlock(void) > */ > atomic_set_release(&printk_cpulock_owner, > -1); /* LMM(__printk_cpu_unlock:B) */ > + > + if (trigger_kgdb) { > + pr_warn("re-triggering kgdb roundup for CPU#%d\n", cpu); > + kgdb_roundup_cpu(cpu); > + } > }
WARNING: multiple messages have this Message-ID (diff)
From: Petr Mladek <pmladek@suse.com> To: John Ogness <john.ogness@linutronix.de> Cc: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>, "Douglas Anderson" <dianders@chromium.org>, "Srikar Dronamraju" <srikar@linux.vnet.ibm.com>, "Peter Zijlstra" <peterz@infradead.org>, linux-kernel@vger.kernel.org, "Paul Mackerras" <paulus@samba.org>, "H. Peter Anvin" <hpa@zytor.com>, "Chengyang Fan" <cy.fan@huawei.com>, "Daniel Thompson" <daniel.thompson@linaro.org>, "Bhaskar Chowdhury" <unixbhaskar@gmail.com>, x86@kernel.org, "Ingo Molnar" <mingo@redhat.com>, kgdb-bugreport@lists.sourceforge.net, "Nicholas Piggin" <npiggin@gmail.com>, "Borislav Petkov" <bp@alien8.de>, "Steven Rostedt" <rostedt@goodmis.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Gustavo A. R. Silva" <gustavoars@kernel.org>, "Sergey Senozhatsky" <senozhatsky@chromium.org>, "Jason Wessel" <jason.wessel@windriver.com>, linuxppc-dev@lists.ozlabs.org, "Cédric Le Goater" <clg@kaod.org> Subject: Re: [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock Date: Wed, 4 Aug 2021 14:31:36 +0200 [thread overview] Message-ID: <YQqIqKjRcNhZSaAZ@alley> (raw) In-Reply-To: <87tuk635rb.fsf@jogness.linutronix.de> On Tue 2021-08-03 17:36:32, John Ogness wrote: > On 2021-08-03, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Tue, Aug 03, 2021 at 03:18:54PM +0206, John Ogness wrote: > >> kgdb makes use of its own cpulock (@dbg_master_lock, @kgdb_active) > >> during cpu roundup. This will conflict with the printk cpulock. > > > >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > >> index 3d0c933937b4..1b546e117f10 100644 > >> --- a/kernel/printk/printk.c > >> +++ b/kernel/printk/printk.c > >> @@ -214,6 +215,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, > >> #ifdef CONFIG_SMP > >> static atomic_t printk_cpulock_owner = ATOMIC_INIT(-1); > >> static atomic_t printk_cpulock_nested = ATOMIC_INIT(0); > >> +static unsigned int kgdb_cpu = -1; > > > > Is this the flag to provoke retriggering? It appears to be a write-only > > variable (at least in this patch). How is it consumed? > > Critical catch! Thank you. I am quite unhappy to see these hunks were > accidentally dropped when generating this series. > > @@ -3673,6 +3675,9 @@ EXPORT_SYMBOL(__printk_cpu_trylock); > */ > void __printk_cpu_unlock(void) > { > + bool trigger_kgdb = false; > + unsigned int cpu; > + > if (atomic_read(&printk_cpulock_nested)) { > atomic_dec(&printk_cpulock_nested); > return; > @@ -3683,6 +3688,12 @@ void __printk_cpu_unlock(void) > * LMM(__printk_cpu_unlock:A) > */ > > + cpu = smp_processor_id(); > + if (kgdb_cpu == cpu) { > + trigger_kgdb = true; > + kgdb_cpu = -1; > + } Just in case that this approach is used in the end. This code looks racy. kgdb_roundup_delay() seems to be called in NMI context. NMI might happen at this point and set kgdb_cpu after it was checked. I am afraid that it won't be easy to make this safe using a single global variable. A solution might be a per-CPU variable set by kgdb_roundup_delay() when it owns printk_cpu_lock. __printk_cpu_unlock() would call kgdb_roundup_cpu(cpu) when the variable is set. Nit: The name "kgdb_cpu" is too generic. It is not clear what is so special about this CPU. I would call the per-CPU variable "kgdb_delayed_roundup" or so. Best Regards, Petr > /* > * Guarantee loads and stores from this CPU when it was the > * lock owner are visible to the next lock owner. This pairs > @@ -3703,6 +3714,21 @@ void __printk_cpu_unlock(void) > */ > atomic_set_release(&printk_cpulock_owner, > -1); /* LMM(__printk_cpu_unlock:B) */ > + > + if (trigger_kgdb) { > + pr_warn("re-triggering kgdb roundup for CPU#%d\n", cpu); > + kgdb_roundup_cpu(cpu); > + } > }
next prev parent reply other threads:[~2021-08-04 12:31 UTC|newest] Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-03 13:12 [PATCH printk v1 00/10] printk: introduce atomic consoles and sync mode John Ogness 2021-08-03 13:12 ` John Ogness 2021-08-03 13:12 ` John Ogness 2021-08-03 13:12 ` [PATCH printk v1 01/10] printk: relocate printk cpulock functions John Ogness 2021-08-04 9:24 ` Petr Mladek 2021-08-03 13:12 ` [PATCH printk v1 02/10] printk: rename printk cpulock API and always disable interrupts John Ogness 2021-08-04 9:52 ` Petr Mladek 2021-08-03 13:12 ` [PATCH printk v1 03/10] kgdb: delay roundup if holding printk cpulock John Ogness 2021-08-03 13:12 ` John Ogness 2021-08-03 14:25 ` Daniel Thompson 2021-08-03 14:25 ` Daniel Thompson 2021-08-03 15:30 ` John Ogness 2021-08-03 15:30 ` John Ogness 2021-08-04 11:31 ` Daniel Thompson 2021-08-04 11:31 ` Daniel Thompson 2021-08-04 12:12 ` Petr Mladek 2021-08-04 12:12 ` Petr Mladek 2021-08-04 15:04 ` Daniel Thompson 2021-08-04 15:04 ` Daniel Thompson 2021-08-05 3:46 ` John Ogness 2021-08-05 3:46 ` John Ogness 2021-08-06 12:06 ` Daniel Thompson 2021-08-06 12:06 ` Daniel Thompson 2021-08-04 12:31 ` Petr Mladek [this message] 2021-08-04 12:31 ` Petr Mladek 2021-08-03 13:12 ` [PATCH printk v1 04/10] printk: relocate printk_delay() John Ogness 2021-08-04 13:07 ` Petr Mladek 2021-08-03 13:12 ` [PATCH printk v1 05/10] printk: call boot_delay_msec() in printk_delay() John Ogness 2021-08-04 13:09 ` Petr Mladek 2021-08-31 1:04 ` Sergey Senozhatsky 2021-08-03 13:12 ` [PATCH printk v1 06/10] printk: use seqcount_latch for console_seq John Ogness 2021-08-05 12:16 ` Petr Mladek 2021-08-05 15:26 ` John Ogness 2021-08-06 15:56 ` Petr Mladek 2021-08-31 3:05 ` Sergey Senozhatsky 2021-08-03 13:12 ` [PATCH printk v1 07/10] console: add write_atomic interface John Ogness 2021-08-03 14:02 ` Andy Shevchenko 2021-08-06 10:56 ` John Ogness 2021-08-06 11:18 ` Andy Shevchenko 2021-08-31 2:55 ` Sergey Senozhatsky 2021-08-03 13:12 ` [PATCH printk v1 08/10] printk: introduce kernel sync mode John Ogness 2021-08-05 17:11 ` Petr Mladek 2021-08-05 21:25 ` John Ogness 2021-08-03 13:13 ` [PATCH printk v1 09/10] kdb: if available, only use atomic consoles for output mirroring John Ogness 2021-08-03 13:13 ` [PATCH printk v1 10/10] serial: 8250: implement write_atomic John Ogness 2021-08-03 13:13 ` John Ogness 2021-08-03 13:13 ` John Ogness 2021-08-03 14:07 ` Andy Shevchenko 2021-08-03 14:07 ` Andy Shevchenko 2021-08-03 14:07 ` Andy Shevchenko 2021-08-05 7:47 ` Jiri Slaby 2021-08-05 7:47 ` Jiri Slaby 2021-08-05 7:47 ` Jiri Slaby 2021-08-05 8:26 ` John Ogness 2021-08-05 8:26 ` John Ogness 2021-08-05 8:26 ` John Ogness 2021-08-03 13:52 ` [PATCH printk v1 00/10] printk: introduce atomic consoles and sync mode Andy Shevchenko 2021-08-03 13:52 ` Andy Shevchenko 2021-08-03 13:52 ` Andy Shevchenko 2021-08-05 15:47 ` Petr Mladek 2021-08-05 15:47 ` Petr Mladek 2021-08-05 15:47 ` Petr Mladek 2021-08-31 0:33 ` Sergey Senozhatsky 2021-08-31 0:33 ` Sergey Senozhatsky 2021-08-31 0:33 ` Sergey Senozhatsky
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=YQqIqKjRcNhZSaAZ@alley \ --to=pmladek@suse.com \ --cc=benh@kernel.crashing.org \ --cc=bp@alien8.de \ --cc=christophe.leroy@csgroup.eu \ --cc=clg@kaod.org \ --cc=cy.fan@huawei.com \ --cc=daniel.thompson@linaro.org \ --cc=dianders@chromium.org \ --cc=ego@linux.vnet.ibm.com \ --cc=gustavoars@kernel.org \ --cc=hpa@zytor.com \ --cc=jason.wessel@windriver.com \ --cc=john.ogness@linutronix.de \ --cc=kgdb-bugreport@lists.sourceforge.net \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mingo@redhat.com \ --cc=mpe@ellerman.id.au \ --cc=npiggin@gmail.com \ --cc=paulus@samba.org \ --cc=peterz@infradead.org \ --cc=rostedt@goodmis.org \ --cc=senozhatsky@chromium.org \ --cc=srikar@linux.vnet.ibm.com \ --cc=tglx@linutronix.de \ --cc=unixbhaskar@gmail.com \ --cc=x86@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.