From: John Ogness <john.ogness@linutronix.de>
To: Petr Mladek <pmladek@suse.com>
Cc: linux-kernel@vger.kernel.org,
Peter Zijlstra <peterz@infradead.org>,
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>,
Daniel Wang <wonderfly@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Alan Cox <gnomes@lxorguk.ukuu.org.uk>,
Jiri Slaby <jslaby@suse.com>, Peter Feiner <pfeiner@google.com>,
linux-serial@vger.kernel.org,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [RFC PATCH v1 02/25] printk-rb: add prb locking functions
Date: Wed, 13 Feb 2019 22:39:20 +0100 [thread overview]
Message-ID: <87pnrvs707.fsf@linutronix.de> (raw)
In-Reply-To: 20190213154541.wvft64nf352vghou@pathway.suse.cz
On 2019-02-13, Petr Mladek <pmladek@suse.com> wrote:
>> Add processor-reentrant spin locking functions. These allow
>> restricting the number of possible contexts to 2, which can simplify
>> implementing code that also supports NMI interruptions.
>>
>> prb_lock();
>>
>> /*
>> * This code is synchronized with all contexts
>> * except an NMI on the same processor.
>> */
>>
>> prb_unlock();
>>
>> In order to support printk's emergency messages, a
>> processor-reentrant spin lock will be used to control raw access to
>> the emergency console. However, it must be the same
>> processor-reentrant spin lock as the one used by the ring buffer,
>> otherwise a deadlock can occur:
>>
>> CPU1: printk lock -> emergency -> serial lock
>> CPU2: serial lock -> printk lock
>>
>> By making the processor-reentrant implemtation available externally,
>> printk can use the same atomic_t for the ring buffer as for the
>> emergency console and thus avoid the above deadlock.
>
> Interesting idea. I just wonder if it might cause some problems
> when it is shared between printk() and many other consoles.
>
> It sounds like the big kernel lock or console_lock. They
> both caused many troubles.
It causes big troubles (deadlocks) if you have multiple instances of
it. Mainly because printk can be called from _any_ line of code in the
kernel. That is the reason I decided that it needs to be shared and only
used in call chains that are carefully constructed such as:
printk -> write_atomic
and NMI contexts are _never_ allowed to do things that rely on waiting
forever for other CPUs. For that reason it does kinda feel like a BKL.
If we do find some problems, we may want to switch to a ringbuffer
implementation that is fully lockless for both multi-readers and
multi-writers. I have written such a beast, but it is less efficient and
more complex than the ringbuffer in this series. Also, that only shrinks
the window since write_atomic would still need to make use of the
processor-reentrant spinlock to synchronize the console output. That's
why I decided to RFC with the simpler ringbuffer implementation.
>> diff --git a/lib/printk_ringbuffer.c b/lib/printk_ringbuffer.c
>> new file mode 100644
>> index 000000000000..28958b0cf774
>> --- /dev/null
>> +++ b/lib/printk_ringbuffer.c
>> @@ -0,0 +1,77 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#include <linux/smp.h>
>> +#include <linux/printk_ringbuffer.h>
>> +
>> +static bool __prb_trylock(struct prb_cpulock *cpu_lock,
>> + unsigned int *cpu_store)
>> +{
>> + unsigned long *flags;
>> + unsigned int cpu;
>> +
>> + cpu = get_cpu();
>> +
>> + *cpu_store = atomic_read(&cpu_lock->owner);
>> + /* memory barrier to ensure the current lock owner is visible */
>> + smp_rmb();
>> + if (*cpu_store == -1) {
>> + flags = per_cpu_ptr(cpu_lock->irqflags, cpu);
>> + local_irq_save(*flags);
>> + if (atomic_try_cmpxchg_acquire(&cpu_lock->owner,
>> + cpu_store, cpu)) {
>> + return true;
>> + }
>> + local_irq_restore(*flags);
>> + } else if (*cpu_store == cpu) {
>> + return true;
>> + }
>> +
>> + put_cpu();
>
> Is there any reason why you get/put CPU and enable/disable
> in each iteration?
>
> It is a spin lock after all. We do busy waiting anyway. This looks like
> burning CPU power for no real gain. Simple cpu_relax() should be
> enough.
Agreed.
>> + return false;
>> +}
>> +
>> +/*
>> + * prb_lock: Perform a processor-reentrant spin lock.
>> + * @cpu_lock: A pointer to the lock object.
>> + * @cpu_store: A "flags" pointer to store lock status information.
>> + *
>> + * If no processor has the lock, the calling processor takes the lock and
>> + * becomes the owner. If the calling processor is already the owner of the
>> + * lock, this function succeeds immediately. If lock is locked by another
>> + * processor, this function spins until the calling processor becomes the
>> + * owner.
>> + *
>> + * It is safe to call this function from any context and state.
>> + */
>> +void prb_lock(struct prb_cpulock *cpu_lock, unsigned int *cpu_store)
>> +{
>> + for (;;) {
>> + if (__prb_trylock(cpu_lock, cpu_store))
>> + break;
>> + cpu_relax();
>> + }
>> +}
>> +
>> +/*
>> + * prb_unlock: Perform a processor-reentrant spin unlock.
>> + * @cpu_lock: A pointer to the lock object.
>> + * @cpu_store: A "flags" object storing lock status information.
>> + *
>> + * Release the lock. The calling processor must be the owner of the lock.
>> + *
>> + * It is safe to call this function from any context and state.
>> + */
>> +void prb_unlock(struct prb_cpulock *cpu_lock, unsigned int cpu_store)
>> +{
>> + unsigned long *flags;
>> + unsigned int cpu;
>> +
>> + cpu = atomic_read(&cpu_lock->owner);
>> + atomic_set_release(&cpu_lock->owner, cpu_store);
>> +
>> + if (cpu_store == -1) {
>> + flags = per_cpu_ptr(cpu_lock->irqflags, cpu);
>> + local_irq_restore(*flags);
>> + }
>
> cpu_store looks like an implementation detail. The caller
> needs to remember it to handle the nesting properly.
It's really no different than "flags" in irqsave/irqrestore.
> We could achieve the same with a recursion counter hidden
> in struct prb_lock.
The only way I see how that could be implemented is if the cmpxchg
encoded the cpu owner and counter into a single integer. (Upper half as
counter, lower half as cpu owner.) Both fields would need to be updated
with a single cmpxchg. The critical cmpxchg being the one where the CPU
becomes unlocked (counter goes from 1 to 0 and cpu owner goes from N to
-1).
That seems like a lot of extra code just to avoid passing a "flags"
argument.
John Ogness
next prev parent reply other threads:[~2019-02-13 21:39 UTC|newest]
Thread overview: 147+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-12 14:29 [RFC PATCH v1 00/25] printk: new implementation John Ogness
2019-02-12 14:29 ` [RFC PATCH v1 01/25] printk-rb: add printk ring buffer documentation John Ogness
2019-02-12 14:45 ` Greg Kroah-Hartman
2019-02-12 14:29 ` [RFC PATCH v1 02/25] printk-rb: add prb locking functions John Ogness
2019-02-13 15:45 ` Petr Mladek
2019-02-13 21:39 ` John Ogness [this message]
2019-02-14 10:33 ` Petr Mladek
2019-02-14 12:10 ` John Ogness
2019-02-15 10:26 ` Petr Mladek
2019-02-15 10:56 ` John Ogness
2019-03-07 2:12 ` Sergey Senozhatsky
2019-02-12 14:29 ` [RFC PATCH v1 03/25] printk-rb: define ring buffer struct and initializer John Ogness
2019-02-12 14:46 ` Greg Kroah-Hartman
2019-02-14 12:46 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 04/25] printk-rb: add writer interface John Ogness
2019-02-14 15:16 ` Petr Mladek
2019-02-14 23:36 ` John Ogness
2019-02-15 1:19 ` John Ogness
2019-02-15 13:47 ` Petr Mladek
2019-02-17 1:32 ` John Ogness
2019-02-21 13:51 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 05/25] printk-rb: add basic non-blocking reading interface John Ogness
2019-02-18 12:54 ` Petr Mladek
2019-02-19 21:44 ` John Ogness
2019-02-21 16:22 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 06/25] printk-rb: add blocking reader support John Ogness
2019-02-18 14:05 ` Petr Mladek
2019-02-19 21:47 ` John Ogness
2019-02-12 14:29 ` [RFC PATCH v1 07/25] printk-rb: add functionality required by printk John Ogness
2019-02-12 17:15 ` Linus Torvalds
2019-02-13 9:20 ` John Ogness
2019-02-18 15:59 ` Petr Mladek
2019-02-19 22:08 ` John Ogness
2019-02-22 9:58 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 08/25] printk: add ring buffer and kthread John Ogness
2019-02-12 15:47 ` Sergey Senozhatsky
2019-02-19 13:54 ` Petr Mladek
2019-03-04 7:38 ` Sergey Senozhatsky
2019-03-04 10:00 ` Sergey Senozhatsky
2019-03-04 11:07 ` Sergey Senozhatsky
2019-03-05 21:00 ` John Ogness
2019-03-06 15:57 ` Petr Mladek
2019-03-06 21:17 ` John Ogness
2019-03-06 22:22 ` John Ogness
2019-03-07 6:41 ` Sergey Senozhatsky
2019-03-07 6:51 ` Sergey Senozhatsky
2019-03-07 12:50 ` Petr Mladek
2019-03-07 5:15 ` Sergey Senozhatsky
2019-03-11 10:51 ` John Ogness
2019-03-12 9:58 ` Sergey Senozhatsky
2019-03-12 10:30 ` Petr Mladek
2019-03-07 12:06 ` John Ogness
2019-03-08 1:31 ` Sergey Senozhatsky
2019-03-08 10:04 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 09/25] printk: remove exclusive console hack John Ogness
2019-02-19 14:03 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 10/25] printk: redirect emit/store to new ringbuffer John Ogness
2019-02-20 9:01 ` Petr Mladek
2019-02-20 21:25 ` John Ogness
2019-02-22 14:43 ` Petr Mladek
2019-02-22 15:06 ` John Ogness
2019-02-22 15:25 ` Petr Mladek
2019-02-25 12:11 ` Petr Mladek
2019-02-25 16:41 ` John Ogness
2019-02-26 9:45 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 11/25] printk_safe: remove printk safe code John Ogness
2019-02-22 10:37 ` Petr Mladek
2019-02-22 13:38 ` John Ogness
2019-02-22 15:15 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 12/25] printk: minimize console locking implementation John Ogness
2019-02-25 13:44 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 13/25] printk: track seq per console John Ogness
2019-02-25 14:59 ` Petr Mladek
2019-02-26 8:45 ` John Ogness
2019-02-26 13:11 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 14/25] printk: do boot_delay_msec inside printk_delay John Ogness
2019-02-12 14:29 ` [RFC PATCH v1 15/25] printk: print history for new consoles John Ogness
2019-02-26 14:58 ` Petr Mladek
2019-02-26 15:22 ` John Ogness
2019-02-27 9:02 ` Petr Mladek
2019-02-27 10:02 ` John Ogness
2019-02-27 13:12 ` Petr Mladek
2019-03-04 9:24 ` Sergey Senozhatsky
2019-02-12 14:29 ` [RFC PATCH v1 16/25] printk: implement CON_PRINTBUFFER John Ogness
2019-02-26 15:38 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 17/25] printk: add processor number to output John Ogness
2019-02-13 22:29 ` John Ogness
2019-02-12 14:29 ` [RFC PATCH v1 18/25] console: add write_atomic interface John Ogness
2019-02-12 14:29 ` [RFC PATCH v1 19/25] printk: introduce emergency messages John Ogness
2019-03-07 7:30 ` Sergey Senozhatsky
2019-03-08 10:31 ` Petr Mladek
2019-03-11 12:04 ` John Ogness
2019-03-12 2:51 ` Sergey Senozhatsky
2019-03-12 2:58 ` Sergey Senozhatsky
2019-02-12 14:29 ` [RFC PATCH v1 20/25] serial: 8250: implement write_atomic John Ogness
2019-02-27 9:46 ` Petr Mladek
2019-02-27 10:32 ` John Ogness
2019-02-27 13:55 ` Petr Mladek
2019-03-08 4:05 ` John Ogness
2019-03-08 4:17 ` John Ogness
2019-03-08 10:28 ` Petr Mladek
2019-02-12 14:29 ` [RFC PATCH v1 21/25] printk: implement KERN_CONT John Ogness
2019-02-12 14:30 ` [RFC PATCH v1 22/25] printk: implement /dev/kmsg John Ogness
2019-02-12 14:30 ` [RFC PATCH v1 23/25] printk: implement syslog John Ogness
2019-02-12 14:30 ` [RFC PATCH v1 24/25] printk: implement kmsg_dump John Ogness
2019-02-12 14:30 ` [RFC PATCH v1 25/25] printk: remove unused code John Ogness
2019-03-08 14:02 ` Sebastian Andrzej Siewior
2019-03-11 2:46 ` Sergey Senozhatsky
2019-03-11 8:18 ` Sebastian Andrzej Siewior
2019-03-12 9:38 ` Petr Mladek
2019-02-13 1:31 ` [RFC PATCH v1 00/25] printk: new implementation Sergey Senozhatsky
2019-02-13 13:43 ` John Ogness
2019-03-04 6:39 ` Sergey Senozhatsky
2019-02-13 1:41 ` Sergey Senozhatsky
2019-02-13 14:15 ` John Ogness
2019-03-04 5:31 ` Sergey Senozhatsky
2019-02-13 2:55 ` Sergey Senozhatsky
2019-02-13 14:43 ` John Ogness
2019-03-04 5:23 ` Sergey Senozhatsky
2019-03-07 9:53 ` John Ogness
2019-03-08 10:00 ` Petr Mladek
2019-03-11 10:54 ` Sergey Senozhatsky
2019-03-12 12:38 ` Petr Mladek
2019-03-12 15:15 ` John Ogness
2019-03-13 2:15 ` Sergey Senozhatsky
2019-03-13 8:19 ` John Ogness
2019-03-13 8:40 ` Sebastian Siewior
2019-03-13 9:27 ` Sergey Senozhatsky
2019-03-13 10:06 ` Sergey Senozhatsky
2019-03-14 9:27 ` Petr Mladek
2019-03-13 8:46 ` Sergey Senozhatsky
2019-03-14 9:14 ` Petr Mladek
2019-03-14 9:35 ` John Ogness
2019-03-13 2:00 ` Sergey Senozhatsky
2019-02-13 16:54 ` David Laight
2019-02-13 22:20 ` John Ogness
2020-01-20 23:05 ` Eugeniu Rosca
2020-01-21 23:56 ` John Ogness
2020-01-22 2:34 ` Eugeniu Rosca
2020-01-22 7:31 ` Geert Uytterhoeven
2020-01-22 16:58 ` Eugeniu Rosca
2020-01-22 19:48 ` Geert Uytterhoeven
2020-01-24 16:09 ` Eugeniu Rosca
2020-01-27 12:32 ` Petr Mladek
2020-01-27 13:45 ` Eugeniu Rosca
2020-01-22 10:33 ` John Ogness
2020-01-24 12:13 ` Eugeniu Rosca
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=87pnrvs707.fsf@linutronix.de \
--to=john.ogness@linutronix.de \
--cc=akpm@linux-foundation.org \
--cc=gnomes@lxorguk.ukuu.org.uk \
--cc=gregkh@linuxfoundation.org \
--cc=jslaby@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=pfeiner@google.com \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=sergey.senozhatsky.work@gmail.com \
--cc=sergey.senozhatsky@gmail.com \
--cc=torvalds@linux-foundation.org \
--cc=wonderfly@google.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
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).