linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2 v4] Init the hashed pointer from a worker.
@ 2022-09-27 10:49 Sebastian Andrzej Siewior
  2022-09-27 10:49 ` [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval() Sebastian Andrzej Siewior
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-09-27 10:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: Theodore Ts'o, Andy Shevchenko, Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Petr Mladek,
	Rasmus Villemoes, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

This is a mini series to initialize the random value, needed for the %p
format argument, upfront during boot instead on demand. The latter is
problematic on PREEMPT_RT if the first user happens to be in an atomic
region.

v3…v4:
    - Added a __read_mostly.
    - Added Jason's Acked-by for 2/2 after talking to him at Plumbers.
      While we were discussion several ways of tackling this differently
      and the possible problems/ side effects that this may cause we
      happen to notice that the current way of doing things is also a
      problem if the first printk("%p\n") user happens to be in NMI
      context.
      Therefore I leave it to the vsprintf/ printk maintainer to decide
      if this is -stable material or not. I'm not aware of any NMI code
      path using %p but then it is not officially forbidden.
      Assuming unknown_nmi_error() contains %p format the string, then
      the backtrace at the end of the email will be printed.

v2…v3:
    - schedule a worker every two seconds if the RNG core is not ready.
    https://lore.kernel.org/all/YueeIgPGUJgsnsAh@linutronix.de

v1…v2:
   - Remove the static_branch_likely() as suggested by Petr Mladek.
   - Jason wasn't onboard with fiddling in random core to get the job
     done. Instead a worker is scheduled from an initcall and
     get_random_bytes_wait() is used to get the date once it is
     available.
   https://lore.kernel.org/all/20220729154716.429964-1-bigeasy@linutronix.de/

v1:
   https://lore.kernel.org/all/YuOf6qu453dOkR+S@linutronix.de/

Before the series after adding "%p" to unknown_nmi_error() and
triggering a NMI:
| ================================
| WARNING: inconsistent lock state
| 6.0.0-rc7+ #6 Not tainted
| --------------------------------
| inconsistent {INITIAL USE} -> {IN-NMI} usage.
| swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
| ffffffff82aea4f0 (input_pool.lock){..-.}-{2:2}, at: extract_entropy.constprop.0+0x76/0x240
| {INITIAL USE} state was registered at:
| irq event stamp: 37104
| hardirqs last  enabled at (37103): [<ffffffff81cc8e80>] default_idle_call+0x20/0x90
| hardirqs last disabled at (37104): [<ffffffff81cbbb2b>] exc_nmi+0x7b/0x120
| softirqs last  enabled at (37098): [<ffffffff810f3a8c>] __irq_exit_rcu+0x8c/0xb0
| softirqs last disabled at (37085): [<ffffffff810f3a8c>] __irq_exit_rcu+0x8c/0xb0
| 
| other info that might help us debug this:
|  Possible unsafe locking scenario:
| 
|        CPU0
|        ----
|   lock(input_pool.lock);
|   <Interrupt>
|     lock(input_pool.lock);
| 
|  *** DEADLOCK ***
| 
| no locks held by swapper/0/0.
| 
| stack backtrace:
| CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.0.0-rc7+ #6
| Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
| Call Trace:
|  <NMI>
|  dump_stack_lvl+0x4c/0x63
|  lock_acquire.cold+0x43/0x48
|  _raw_spin_lock_irqsave+0x33/0x50
|  extract_entropy.constprop.0+0x76/0x240
|  crng_reseed+0x20/0xf0
|  crng_make_state+0x51/0x2b0
|  _get_random_bytes.part.0+0x47/0x150
|  default_pointer+0x3e9/0x420
|  vsnprintf+0x1a8/0x550
|  vprintk_store+0x13e/0x4c0
|  vprintk+0x2e/0x50
|  _printk+0x53/0x6e
|  default_do_nmi+0x224/0x290
|  exc_nmi+0xf1/0x120
|  end_repeat_nmi+0x16/0x67
| RIP: 0010:default_idle+0xb/0x10
…
|  </NMI>
|  <TASK>
|  default_idle_call+0x51/0x90
|  do_idle+0x201/0x270
|  cpu_startup_entry+0x14/0x20
|  rest_init+0xe5/0x170
|  arch_call_rest_init+0x5/0xa
|  start_kernel+0x68c/0x6b5
|  secondary_startup_64_no_verify+0xe0/0xeb
|  </TASK>
| ------------[ cut here ]------------
| WARNING: CPU: 0 PID: 0 at arch/x86/kernel/fpu/core.c:60 irq_fpu_usable+0x34/0x40
| Modules linked in:
| CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.0.0-rc7+ #6
| Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
| RIP: 0010:irq_fpu_usable+0x34/0x40
…
| Call Trace:
|  <NMI>
|  blake2s_compress+0x1b/0xa0
|  blake2s_final+0x3c/0x60
|  extract_entropy.constprop.0+0x8d/0x240
|  crng_reseed+0x20/0xf0
|  crng_make_state+0x51/0x2b0
|  _get_random_bytes.part.0+0x47/0x150
|  default_pointer+0x3e9/0x420
|  vsnprintf+0x1a8/0x550
|  vprintk_store+0x13e/0x4c0
|  vprintk+0x2e/0x50
|  _printk+0x53/0x6e
|  default_do_nmi+0x224/0x290
|  exc_nmi+0xf1/0x120
|  end_repeat_nmi+0x16/0x67
| RIP: 0010:default_idle+0xb/0x10
…
|  </NMI>
|  <TASK>
|  default_idle_call+0x51/0x90
|  do_idle+0x201/0x270
|  cpu_startup_entry+0x14/0x20
|  rest_init+0xe5/0x170
|  arch_call_rest_init+0x5/0xa
|  start_kernel+0x68c/0x6b5
|  secondary_startup_64_no_verify+0xe0/0xeb
|  </TASK>
| irq event stamp: 37104
| hardirqs last  enabled at (37103): [<ffffffff81cc8e80>] default_idle_call+0x20/0x90
| hardirqs last disabled at (37104): [<ffffffff81cbbb2b>] exc_nmi+0x7b/0x120
| softirqs last  enabled at (37098): [<ffffffff810f3a8c>] __irq_exit_rcu+0x8c/0xb0
| softirqs last disabled at (37085): [<ffffffff810f3a8c>] __irq_exit_rcu+0x8c/0xb0
| ---[ end trace 0000000000000000 ]---
| Uhhuh. NMI received for unknown reason 30 on CPU 0 / 00000000f8da9c8a.
| Dazed and confused, but trying to continue

Sebastian


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval().
  2022-09-27 10:49 [PATCH 0/2 v4] Init the hashed pointer from a worker Sebastian Andrzej Siewior
@ 2022-09-27 10:49 ` Sebastian Andrzej Siewior
  2022-09-27 14:49   ` Petr Mladek
  2022-09-30 10:27   ` Sergey Senozhatsky
  2022-09-27 10:49 ` [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready Sebastian Andrzej Siewior
  2022-09-29 12:43 ` [PATCH 0/2 v4] Init the hashed pointer from a worker Petr Mladek
  2 siblings, 2 replies; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-09-27 10:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: Theodore Ts'o, Andy Shevchenko, Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Petr Mladek,
	Rasmus Villemoes, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner, Sebastian Andrzej Siewior

Using static_branch_likely() to signal that ptr_key has been filled is a
bit much given that it is not a fast path.

Replace static_branch_likely() with bool for condition and a memory
barrier for ptr_key.

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 lib/vsprintf.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3c1853a9d1c09..bce63cbf23779 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -750,12 +750,7 @@ static int __init debug_boot_weak_hash_enable(char *str)
 }
 early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
 
-static DEFINE_STATIC_KEY_FALSE(filled_random_ptr_key);
-
-static void enable_ptr_key_workfn(struct work_struct *work)
-{
-	static_branch_enable(&filled_random_ptr_key);
-}
+static bool filled_random_ptr_key __read_mostly;
 
 /* Maps a pointer to a 32 bit unique identifier. */
 static inline int __ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
@@ -763,24 +758,26 @@ static inline int __ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
 	static siphash_key_t ptr_key __read_mostly;
 	unsigned long hashval;
 
-	if (!static_branch_likely(&filled_random_ptr_key)) {
+	if (!READ_ONCE(filled_random_ptr_key)) {
 		static bool filled = false;
 		static DEFINE_SPINLOCK(filling);
-		static DECLARE_WORK(enable_ptr_key_work, enable_ptr_key_workfn);
 		unsigned long flags;
 
-		if (!system_unbound_wq || !rng_is_initialized() ||
+		if (!rng_is_initialized() ||
 		    !spin_trylock_irqsave(&filling, flags))
 			return -EAGAIN;
 
 		if (!filled) {
 			get_random_bytes(&ptr_key, sizeof(ptr_key));
-			queue_work(system_unbound_wq, &enable_ptr_key_work);
+			/* Pairs with smp_rmb() before reading ptr_key. */
+			smp_wmb();
+			WRITE_ONCE(filled_random_ptr_key, true);
 			filled = true;
 		}
 		spin_unlock_irqrestore(&filling, flags);
 	}
-
+	/* Pairs with smp_wmb() after writing ptr_key. */
+	smp_rmb();
 
 #ifdef CONFIG_64BIT
 	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready.
  2022-09-27 10:49 [PATCH 0/2 v4] Init the hashed pointer from a worker Sebastian Andrzej Siewior
  2022-09-27 10:49 ` [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval() Sebastian Andrzej Siewior
@ 2022-09-27 10:49 ` Sebastian Andrzej Siewior
  2022-09-27 11:20   ` Jason A. Donenfeld
  2022-09-27 16:40   ` Petr Mladek
  2022-09-29 12:43 ` [PATCH 0/2 v4] Init the hashed pointer from a worker Petr Mladek
  2 siblings, 2 replies; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-09-27 10:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: Theodore Ts'o, Andy Shevchenko, Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Petr Mladek,
	Rasmus Villemoes, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner, Sebastian Andrzej Siewior

The printk code invokes vnsprintf in order to compute the complete
string before adding it into its buffer. This happens in an IRQ-off
region which leads to a warning on PREEMPT_RT in the random code if the
format strings contains a %p for pointer printing. This happens because
the random core acquires locks which become sleeping locks on PREEMPT_RT
which must not be acquired with disabled interrupts and or preemption
disabled.
By default the pointers are hashed which requires a random value on the
first invocation (either by printk or another user which comes first.

One could argue that there is no need for printk to disable interrupts
during the vsprintf() invocation which would fix the just mentioned
problem. However printk itself can be invoked in a context with
disabled interrupts which would lead to the very same problem.

Move the initialization of ptr_key into a worker and schedule it from
subsys_initcall(). This happens early but after the workqueue subsystem
is ready. Use get_random_bytes() to retrieve the random value if the RNG
core is ready, otherwise schedule a worker in two seconds and try again.

Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 lib/vsprintf.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bce63cbf23779..44b39ba56b796 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -751,31 +751,39 @@ static int __init debug_boot_weak_hash_enable(char *str)
 early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
 
 static bool filled_random_ptr_key __read_mostly;
+static siphash_key_t ptr_key __read_mostly;
+static void fill_ptr_key_workfn(struct work_struct *work);
+static DECLARE_DELAYED_WORK(fill_ptr_key_work, fill_ptr_key_workfn);
+
+static void fill_ptr_key_workfn(struct work_struct *work)
+{
+	if (!rng_is_initialized()) {
+		queue_delayed_work(system_unbound_wq, &fill_ptr_key_work, HZ  * 2);
+		return;
+	}
+
+	get_random_bytes(&ptr_key, sizeof(ptr_key));
+
+	/* Pairs with smp_rmb() before reading ptr_key. */
+	smp_wmb();
+	WRITE_ONCE(filled_random_ptr_key, true);
+}
+
+static int __init vsprintf_init_hashval(void)
+{
+	fill_ptr_key_workfn(NULL);
+	return 0;
+}
+subsys_initcall(vsprintf_init_hashval)
 
 /* Maps a pointer to a 32 bit unique identifier. */
 static inline int __ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
 {
-	static siphash_key_t ptr_key __read_mostly;
 	unsigned long hashval;
 
-	if (!READ_ONCE(filled_random_ptr_key)) {
-		static bool filled = false;
-		static DEFINE_SPINLOCK(filling);
-		unsigned long flags;
+	if (!READ_ONCE(filled_random_ptr_key))
+		return -EBUSY;
 
-		if (!rng_is_initialized() ||
-		    !spin_trylock_irqsave(&filling, flags))
-			return -EAGAIN;
-
-		if (!filled) {
-			get_random_bytes(&ptr_key, sizeof(ptr_key));
-			/* Pairs with smp_rmb() before reading ptr_key. */
-			smp_wmb();
-			WRITE_ONCE(filled_random_ptr_key, true);
-			filled = true;
-		}
-		spin_unlock_irqrestore(&filling, flags);
-	}
 	/* Pairs with smp_wmb() after writing ptr_key. */
 	smp_rmb();
 
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready.
  2022-09-27 10:49 ` [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready Sebastian Andrzej Siewior
@ 2022-09-27 11:20   ` Jason A. Donenfeld
  2022-09-27 16:40   ` Petr Mladek
  1 sibling, 0 replies; 12+ messages in thread
From: Jason A. Donenfeld @ 2022-09-27 11:20 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Theodore Ts'o, Andy Shevchenko, John Ogness,
	Mike Galbraith, Peter Zijlstra, Petr Mladek, Rasmus Villemoes,
	Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

You added my Acked-by already (which is fine), but I thought I should
still add some final notes to that ack, for posterity:

On Tue, Sep 27, 2022 at 12:49:12PM +0200, Sebastian Andrzej Siewior wrote:
> The printk code invokes vnsprintf in order to compute the complete
> string before adding it into its buffer. This happens in an IRQ-off
> region which leads to a warning on PREEMPT_RT in the random code if the
> format strings contains a %p for pointer printing. This happens because
> the random core acquires locks which become sleeping locks on PREEMPT_RT
> which must not be acquired with disabled interrupts and or preemption
> disabled.
> By default the pointers are hashed which requires a random value on the
> first invocation (either by printk or another user which comes first.
> 
> One could argue that there is no need for printk to disable interrupts
> during the vsprintf() invocation which would fix the just mentioned
> problem. However printk itself can be invoked in a context with
> disabled interrupts which would lead to the very same problem.
> 
> Move the initialization of ptr_key into a worker and schedule it from
> subsys_initcall(). This happens early but after the workqueue subsystem
> is ready. Use get_random_bytes() to retrieve the random value if the RNG
> core is ready, otherwise schedule a worker in two seconds and try again.
> 
> Reported-by: Mike Galbraith <efault@gmx.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>

I really do hate the idea of polling every 2 seconds. But as we
discussed, this seems like the least bad solution, at least for now. If
we discover another bug in the tree that needs a gross solution like
that, then at that point, I'll move ahead with adding a notifier_block
to random.c, to avoid polling. But so long as this is a one-off (as we
understand it to be at the moment), this seems okay.

Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>

Jason

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval().
  2022-09-27 10:49 ` [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval() Sebastian Andrzej Siewior
@ 2022-09-27 14:49   ` Petr Mladek
  2022-09-30 10:27   ` Sergey Senozhatsky
  1 sibling, 0 replies; 12+ messages in thread
From: Petr Mladek @ 2022-09-27 14:49 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Theodore Ts'o, Andy Shevchenko,
	Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Rasmus Villemoes,
	Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

On Tue 2022-09-27 12:49:11, Sebastian Andrzej Siewior wrote:
> Using static_branch_likely() to signal that ptr_key has been filled is a
> bit much given that it is not a fast path.
> 
> Replace static_branch_likely() with bool for condition and a memory
> barrier for ptr_key.
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready.
  2022-09-27 10:49 ` [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready Sebastian Andrzej Siewior
  2022-09-27 11:20   ` Jason A. Donenfeld
@ 2022-09-27 16:40   ` Petr Mladek
  2022-09-28  9:11     ` Sebastian Andrzej Siewior
  2022-09-30 10:31     ` Sergey Senozhatsky
  1 sibling, 2 replies; 12+ messages in thread
From: Petr Mladek @ 2022-09-27 16:40 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Theodore Ts'o, Andy Shevchenko,
	Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Rasmus Villemoes,
	Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

On Tue 2022-09-27 12:49:12, Sebastian Andrzej Siewior wrote:
> The printk code invokes vnsprintf in order to compute the complete
> string before adding it into its buffer. This happens in an IRQ-off
> region which leads to a warning on PREEMPT_RT in the random code if the
> format strings contains a %p for pointer printing. This happens because
> the random core acquires locks which become sleeping locks on PREEMPT_RT
> which must not be acquired with disabled interrupts and or preemption
> disabled.
> By default the pointers are hashed which requires a random value on the
> first invocation (either by printk or another user which comes first.
> 
> One could argue that there is no need for printk to disable interrupts
> during the vsprintf() invocation which would fix the just mentioned
> problem. However printk itself can be invoked in a context with
> disabled interrupts which would lead to the very same problem.
> 
> Move the initialization of ptr_key into a worker and schedule it from
> subsys_initcall(). This happens early but after the workqueue subsystem
> is ready. Use get_random_bytes() to retrieve the random value if the RNG
> core is ready, otherwise schedule a worker in two seconds and try again.

Another advantage is that it removes a nested lock from the printk()
code path. A deadlock was partly prevented by the trylock. But there was
still a risk of a deadlock when printk() was called under base_crng.lock.

> Reported-by: Mike Galbraith <efault@gmx.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  lib/vsprintf.c | 44 ++++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index bce63cbf23779..44b39ba56b796 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -751,31 +751,39 @@ static int __init debug_boot_weak_hash_enable(char *str)
>  early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
>  
>  static bool filled_random_ptr_key __read_mostly;
> +static siphash_key_t ptr_key __read_mostly;
> +static void fill_ptr_key_workfn(struct work_struct *work);
> +static DECLARE_DELAYED_WORK(fill_ptr_key_work, fill_ptr_key_workfn);
> +
> +static void fill_ptr_key_workfn(struct work_struct *work)
> +{
> +	if (!rng_is_initialized()) {
> +		queue_delayed_work(system_unbound_wq, &fill_ptr_key_work, HZ  * 2);

Is there any particular reason not to use system_wq, please?

The unbound workqueue should be used only in special situations:

--- cut Documentation/core-api/workqueue.rst ---
``WQ_UNBOUND``
  Work items queued to an unbound wq are served by the special
  worker-pools which host workers which are not bound to any
  specific CPU.  This makes the wq behave as a simple execution
  context provider without concurrency management.  The unbound
  worker-pools try to start execution of work items as soon as
  possible.  Unbound wq sacrifices locality but is useful for
  the following cases.

  * Wide fluctuation in the concurrency level requirement is
    expected and using bound wq may end up creating large number
    of mostly unused workers across different CPUs as the issuer
    hops through different CPUs.

  * Long running CPU intensive workloads which can be better
    managed by the system scheduler.
--- cut ---

The thing is that unbound workqueues always process work items
in parallel. They create new workers when there is no idle one.

In compare, system_wq serializes the work items. They use
another worker only when a busy one goes into a sleep.
It is perfectly fine and preferred for work items that
do not need much CPU time.

I have tried it and system_wq works well here. It actually
even initializes the hash earlier here. But it is only by chance
because it happens on the 2nd attempt instead of 3rd one.

> +		return;
> +	}
> +
> +	get_random_bytes(&ptr_key, sizeof(ptr_key));
> +
> +	/* Pairs with smp_rmb() before reading ptr_key. */
> +	smp_wmb();
> +	WRITE_ONCE(filled_random_ptr_key, true);
> +}

With "system_wq":

Reviewed-by: Petr Mladek <pmladek@suse.com>

I could replace "system_unbound_wq" with "system_wq" when
pushing. Is anybody against it, please?

I am sorry that I have missed it when looking at the previous
version.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready.
  2022-09-27 16:40   ` Petr Mladek
@ 2022-09-28  9:11     ` Sebastian Andrzej Siewior
  2022-09-28  9:21       ` Jason A. Donenfeld
  2022-09-30 10:31     ` Sergey Senozhatsky
  1 sibling, 1 reply; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-09-28  9:11 UTC (permalink / raw)
  To: Petr Mladek
  Cc: linux-kernel, Theodore Ts'o, Andy Shevchenko,
	Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Rasmus Villemoes,
	Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

On 2022-09-27 18:40:15 [+0200], Petr Mladek wrote:
> Another advantage is that it removes a nested lock from the printk()
> code path. A deadlock was partly prevented by the trylock. But there was
> still a risk of a deadlock when printk() was called under base_crng.lock.

Okay.

> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index bce63cbf23779..44b39ba56b796 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> I have tried it and system_wq works well here. It actually
> even initializes the hash earlier here. But it is only by chance
> because it happens on the 2nd attempt instead of 3rd one.

Yeah. I added a reschedule of two seconds since it looked okay and I
didn't want to do very often. I have an old box where it takes ~12 secs
to setup and here it is the fifth attempt on average. (Before the rework
it needed way longer to initialize).

> > +		return;
> > +	}
> > +
> > +	get_random_bytes(&ptr_key, sizeof(ptr_key));
> > +
> > +	/* Pairs with smp_rmb() before reading ptr_key. */
> > +	smp_wmb();
> > +	WRITE_ONCE(filled_random_ptr_key, true);
> > +}
> 
> With "system_wq":
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> I could replace "system_unbound_wq" with "system_wq" when
> pushing. Is anybody against it, please?

so schedule_delayed_work() then?
I don't mind at all. I used that one just because serialisation is not
needed and neither is the CPU important.

If you are going to replace it, then I am not going to send an update
(unless I'm old otherwise).

> I am sorry that I have missed it when looking at the previous
> version.

No worries.

> Best Regards,
> Petr

Sebastian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready.
  2022-09-28  9:11     ` Sebastian Andrzej Siewior
@ 2022-09-28  9:21       ` Jason A. Donenfeld
  2022-09-29  8:52         ` Petr Mladek
  0 siblings, 1 reply; 12+ messages in thread
From: Jason A. Donenfeld @ 2022-09-28  9:21 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Petr Mladek, linux-kernel, Theodore Ts'o, Andy Shevchenko,
	John Ogness, Mike Galbraith, Peter Zijlstra, Rasmus Villemoes,
	Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

On 9/28/22, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>
>> I could replace "system_unbound_wq" with "system_wq" when
>> pushing. Is anybody against it, please?
>
> so schedule_delayed_work() then?
> I don't mind at all. I used that one just because serialisation is not
> needed and neither is the CPU important.

Indeed, given that this very much is unbound, I think Sebastian's
original patch makes most sense.

Jason

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready.
  2022-09-28  9:21       ` Jason A. Donenfeld
@ 2022-09-29  8:52         ` Petr Mladek
  0 siblings, 0 replies; 12+ messages in thread
From: Petr Mladek @ 2022-09-29  8:52 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Sebastian Andrzej Siewior, linux-kernel, Theodore Ts'o,
	Andy Shevchenko, John Ogness, Mike Galbraith, Peter Zijlstra,
	Rasmus Villemoes, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner, Tejun Heo

On Wed 2022-09-28 11:21:05, Jason A. Donenfeld wrote:
> On 9/28/22, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> >
> >> I could replace "system_unbound_wq" with "system_wq" when
> >> pushing. Is anybody against it, please?
> >
> > so schedule_delayed_work() then?

yup.

> > I don't mind at all. I used that one just because serialisation is not
> > needed and neither is the CPU important.
> 
> Indeed, given that this very much is unbound, I think Sebastian's
> original patch makes most sense.

Yes, the work does not need any specific CPU. The thing is that the
normal system_wq is the preferred one. Any other workqueues should
be used only when there is a particular reason for it.

The unbound_wq should be used only when:

    + the work needs a lot of CPU time.

    + there are waves on related (sleeping) work items that might be
      triggered from different CPUs.

In our case, the work is only one and short. The preferred
system_wq is perfectly fine.

Best Regards,
Petr

PS: It is not obvious. Tejun told me this when I converted a kthread
    into the workqueue API. Also I spent quite some time understanding
    the workqueue code recently.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2 v4] Init the hashed pointer from a worker.
  2022-09-27 10:49 [PATCH 0/2 v4] Init the hashed pointer from a worker Sebastian Andrzej Siewior
  2022-09-27 10:49 ` [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval() Sebastian Andrzej Siewior
  2022-09-27 10:49 ` [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready Sebastian Andrzej Siewior
@ 2022-09-29 12:43 ` Petr Mladek
  2 siblings, 0 replies; 12+ messages in thread
From: Petr Mladek @ 2022-09-29 12:43 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Theodore Ts'o, Andy Shevchenko,
	Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Rasmus Villemoes,
	Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

On Tue 2022-09-27 12:49:10, Sebastian Andrzej Siewior wrote:
> This is a mini series to initialize the random value, needed for the %p
> format argument, upfront during boot instead on demand. The latter is
> problematic on PREEMPT_RT if the first user happens to be in an atomic
> region.
> 
> v3…v4:
>     - Added a __read_mostly.
>     - Added Jason's Acked-by for 2/2 after talking to him at Plumbers.
>       While we were discussion several ways of tackling this differently
>       and the possible problems/ side effects that this may cause we
>       happen to notice that the current way of doing things is also a
>       problem if the first printk("%p\n") user happens to be in NMI
>       context.
>       Therefore I leave it to the vsprintf/ printk maintainer to decide
>       if this is -stable material or not. I'm not aware of any NMI code
>       path using %p but then it is not officially forbidden.
>       Assuming unknown_nmi_error() contains %p format the string, then
>       the backtrace at the end of the email will be printed.

JFYI, both patches are committed into printk/linux.git,
branch for-6.1-hash-pointer-init

I have just added a note into the 2nd patch about that it also
prevents deadlock when printk("%p", ptr) is called under the lock
used by get_random_bytes(), see
https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/commit/?h=for-6.1-hash-pointer-init&id=6f0ac3b52a9075b7291a72fb338d08491c1f0a64

I did not modify the code. The system_unbound_wq can and should be
fixed separately.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval().
  2022-09-27 10:49 ` [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval() Sebastian Andrzej Siewior
  2022-09-27 14:49   ` Petr Mladek
@ 2022-09-30 10:27   ` Sergey Senozhatsky
  1 sibling, 0 replies; 12+ messages in thread
From: Sergey Senozhatsky @ 2022-09-30 10:27 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Theodore Ts'o, Andy Shevchenko,
	Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Petr Mladek,
	Rasmus Villemoes, Sergey Senozhatsky, Steven Rostedt,
	Thomas Gleixner

On (22/09/27 12:49), Sebastian Andrzej Siewior wrote:
> Using static_branch_likely() to signal that ptr_key has been filled is a
> bit much given that it is not a fast path.
> 
> Replace static_branch_likely() with bool for condition and a memory
> barrier for ptr_key.
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready.
  2022-09-27 16:40   ` Petr Mladek
  2022-09-28  9:11     ` Sebastian Andrzej Siewior
@ 2022-09-30 10:31     ` Sergey Senozhatsky
  1 sibling, 0 replies; 12+ messages in thread
From: Sergey Senozhatsky @ 2022-09-30 10:31 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Sebastian Andrzej Siewior, linux-kernel, Theodore Ts'o,
	Andy Shevchenko, Jason A . Donenfeld ,
	John Ogness, Mike Galbraith, Peter Zijlstra, Rasmus Villemoes,
	Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner

On (22/09/27 18:40), Petr Mladek wrote:
> With "system_wq":
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-09-30 10:58 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-27 10:49 [PATCH 0/2 v4] Init the hashed pointer from a worker Sebastian Andrzej Siewior
2022-09-27 10:49 ` [PATCH v4 1/2] lib/vsprintf: Remove static_branch_likely() from __ptr_to_hashval() Sebastian Andrzej Siewior
2022-09-27 14:49   ` Petr Mladek
2022-09-30 10:27   ` Sergey Senozhatsky
2022-09-27 10:49 ` [PATCH v4 2/2] lib/vsprintf: Initialize vsprintf's pointer hash once the random core is ready Sebastian Andrzej Siewior
2022-09-27 11:20   ` Jason A. Donenfeld
2022-09-27 16:40   ` Petr Mladek
2022-09-28  9:11     ` Sebastian Andrzej Siewior
2022-09-28  9:21       ` Jason A. Donenfeld
2022-09-29  8:52         ` Petr Mladek
2022-09-30 10:31     ` Sergey Senozhatsky
2022-09-29 12:43 ` [PATCH 0/2 v4] Init the hashed pointer from a worker Petr Mladek

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).