* [PATCH v3 1/7] locking: Introduce local_lock()
2020-05-27 20:11 [PATCH v3 0/7] Introduce local_lock() Sebastian Andrzej Siewior
@ 2020-05-27 20:11 ` Sebastian Andrzej Siewior
2020-06-01 9:52 ` [tip: locking/core] " tip-bot2 for Thomas Gleixner
2020-05-27 20:11 ` [PATCH v3 2/7] radix-tree: Use local_lock for protection Sebastian Andrzej Siewior
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-27 20:11 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Steven Rostedt, Will Deacon,
Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Sebastian Andrzej Siewior
From: Thomas Gleixner <tglx@linutronix.de>
preempt_disable() and local_irq_disable/save() are in principle per CPU big
kernel locks. This has several downsides:
- The protection scope is unknown
- Violation of protection rules is hard to detect by instrumentation
- For PREEMPT_RT such sections, unless in low level critical code, can
violate the preemptability constraints.
To address this PREEMPT_RT introduced the concept of local_locks which are
strictly per CPU.
The lock operations map to preempt_disable(), local_irq_disable/save() and
the enabling counterparts on non RT enabled kernels.
If lockdep is enabled local locks gain a lock map which tracks the usage
context. This will catch cases where an area is protected by
preempt_disable() but the access also happens from interrupt context. local
locks have identified quite a few such issues over the years, the most
recent example is:
b7d5dc21072cd ("random: add a spinlock_t to struct batched_entropy")
Aside of the lockdep coverage this also improves code readability as it
precisely annotates the protection scope.
PREEMPT_RT substitutes these local locks with 'sleeping' spinlocks to
protect such sections while maintaining preemtability and CPU locality.
local locks can replace:
- preempt_enable()/disable() pairs
- local_irq_disable/enable() pairs
- local_irq_save/restore() pairs
They are also used to replace code which implicitly disables preemption
like:
- get_cpu()/put_cpu()
- get_cpu_var()/put_cpu_var()
with PREEMPT_RT friendly constructs.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Documentation/locking/locktypes.rst | 215 ++++++++++++++++++++++++++--
include/linux/local_lock.h | 54 +++++++
include/linux/local_lock_internal.h | 90 ++++++++++++
3 files changed, 348 insertions(+), 11 deletions(-)
create mode 100644 include/linux/local_lock.h
create mode 100644 include/linux/local_lock_internal.h
diff --git a/Documentation/locking/locktypes.rst b/Documentation/locking/locktypes.rst
index 09f45ce38d262..1b577a8bf9829 100644
--- a/Documentation/locking/locktypes.rst
+++ b/Documentation/locking/locktypes.rst
@@ -13,6 +13,7 @@ The kernel provides a variety of locking primitives which can be divided
into two categories:
- Sleeping locks
+ - CPU local locks
- Spinning locks
This document conceptually describes these lock types and provides rules
@@ -44,9 +45,23 @@ other contexts unless there is no other option.
On PREEMPT_RT kernels, these lock types are converted to sleeping locks:
+ - local_lock
- spinlock_t
- rwlock_t
+
+CPU local locks
+---------------
+
+ - local_lock
+
+On non-PREEMPT_RT kernels, local_lock functions are wrappers around
+preemption and interrupt disabling primitives. Contrary to other locking
+mechanisms, disabling preemption or interrupts are pure CPU local
+concurrency control mechanisms and not suited for inter-CPU concurrency
+control.
+
+
Spinning locks
--------------
@@ -67,6 +82,7 @@ Spinning locks implicitly disable preemption and the lock / unlock functions
_irqsave/restore() Save and disable / restore interrupt disabled state
=================== ====================================================
+
Owner semantics
===============
@@ -139,6 +155,56 @@ PREEMPT_RT kernels map rw_semaphore to a separate rt_mutex-based
writer from starving readers.
+local_lock
+==========
+
+local_lock provides a named scope to critical sections which are protected
+by disabling preemption or interrupts.
+
+On non-PREEMPT_RT kernels local_lock operations map to the preemption and
+interrupt disabling and enabling primitives:
+
+ =========================== ======================
+ local_lock(&llock) preempt_disable()
+ local_unlock(&llock) preempt_enable()
+ local_lock_irq(&llock) local_irq_disable()
+ local_unlock_irq(&llock) local_irq_enable()
+ local_lock_save(&llock) local_irq_save()
+ local_lock_restore(&llock) local_irq_save()
+ =========================== ======================
+
+The named scope of local_lock has two advantages over the regular
+primitives:
+
+ - The lock name allows static analysis and is also a clear documentation
+ of the protection scope while the regular primitives are scopeless and
+ opaque.
+
+ - If lockdep is enabled the local_lock gains a lockmap which allows to
+ validate the correctness of the protection. This can detect cases where
+ e.g. a function using preempt_disable() as protection mechanism is
+ invoked from interrupt or soft-interrupt context. Aside of that
+ lockdep_assert_held(&llock) works as with any other locking primitive.
+
+local_lock and PREEMPT_RT
+-------------------------
+
+PREEMPT_RT kernels map local_lock to a per-CPU spinlock_t, thus changing
+semantics:
+
+ - All spinlock_t changes also apply to local_lock.
+
+local_lock usage
+----------------
+
+local_lock should be used in situations where disabling preemption or
+interrupts is the appropriate form of concurrency control to protect
+per-CPU data structures on a non PREEMPT_RT kernel.
+
+local_lock is not suitable to protect against preemption or interrupts on a
+PREEMPT_RT kernel due to the PREEMPT_RT specific spinlock_t semantics.
+
+
raw_spinlock_t and spinlock_t
=============================
@@ -258,10 +324,82 @@ PREEMPT_RT kernels map rwlock_t to a separate rt_mutex-based
PREEMPT_RT caveats
==================
+local_lock on RT
+----------------
+
+The mapping of local_lock to spinlock_t on PREEMPT_RT kernels has a few
+implications. For example, on a non-PREEMPT_RT kernel the following code
+sequence works as expected::
+
+ local_lock_irq(&local_lock);
+ raw_spin_lock(&lock);
+
+and is fully equivalent to::
+
+ raw_spin_lock_irq(&lock);
+
+On a PREEMPT_RT kernel this code sequence breaks because local_lock_irq()
+is mapped to a per-CPU spinlock_t which neither disables interrupts nor
+preemption. The following code sequence works perfectly correct on both
+PREEMPT_RT and non-PREEMPT_RT kernels::
+
+ local_lock_irq(&local_lock);
+ spin_lock(&lock);
+
+Another caveat with local locks is that each local_lock has a specific
+protection scope. So the following substitution is wrong::
+
+ func1()
+ {
+ local_irq_save(flags); -> local_lock_irqsave(&local_lock_1, flags);
+ func3();
+ local_irq_restore(flags); -> local_lock_irqrestore(&local_lock_1, flags);
+ }
+
+ func2()
+ {
+ local_irq_save(flags); -> local_lock_irqsave(&local_lock_2, flags);
+ func3();
+ local_irq_restore(flags); -> local_lock_irqrestore(&local_lock_2, flags);
+ }
+
+ func3()
+ {
+ lockdep_assert_irqs_disabled();
+ access_protected_data();
+ }
+
+On a non-PREEMPT_RT kernel this works correctly, but on a PREEMPT_RT kernel
+local_lock_1 and local_lock_2 are distinct and cannot serialize the callers
+of func3(). Also the lockdep assert will trigger on a PREEMPT_RT kernel
+because local_lock_irqsave() does not disable interrupts due to the
+PREEMPT_RT-specific semantics of spinlock_t. The correct substitution is::
+
+ func1()
+ {
+ local_irq_save(flags); -> local_lock_irqsave(&local_lock, flags);
+ func3();
+ local_irq_restore(flags); -> local_lock_irqrestore(&local_lock, flags);
+ }
+
+ func2()
+ {
+ local_irq_save(flags); -> local_lock_irqsave(&local_lock, flags);
+ func3();
+ local_irq_restore(flags); -> local_lock_irqrestore(&local_lock, flags);
+ }
+
+ func3()
+ {
+ lockdep_assert_held(&local_lock);
+ access_protected_data();
+ }
+
+
spinlock_t and rwlock_t
-----------------------
-These changes in spinlock_t and rwlock_t semantics on PREEMPT_RT kernels
+The changes in spinlock_t and rwlock_t semantics on PREEMPT_RT kernels
have a few implications. For example, on a non-PREEMPT_RT kernel the
following code sequence works as expected::
@@ -282,9 +420,61 @@ local_lock mechanism. Acquiring the local_lock pins the task to a CPU,
allowing things like per-CPU interrupt disabled locks to be acquired.
However, this approach should be used only where absolutely necessary.
+A typical scenario is protection of per-CPU variables in thread context::
-raw_spinlock_t
---------------
+ struct foo *p = get_cpu_ptr(&var1);
+
+ spin_lock(&p->lock);
+ p->count += this_cpu_read(var2);
+
+This is correct code on a non-PREEMPT_RT kernel, but on a PREEMPT_RT kernel
+this breaks. The PREEMPT_RT-specific change of spinlock_t semantics does
+not allow to acquire p->lock because get_cpu_ptr() implicitly disables
+preemption. The following substitution works on both kernels::
+
+ struct foo *p;
+
+ migrate_disable();
+ p = this_cpu_ptr(&var1);
+ spin_lock(&p->lock);
+ p->count += this_cpu_read(var2);
+
+On a non-PREEMPT_RT kernel migrate_disable() maps to preempt_disable()
+which makes the above code fully equivalent. On a PREEMPT_RT kernel
+migrate_disable() ensures that the task is pinned on the current CPU which
+in turn guarantees that the per-CPU access to var1 and var2 are staying on
+the same CPU.
+
+The migrate_disable() substitution is not valid for the following
+scenario::
+
+ func()
+ {
+ struct foo *p;
+
+ migrate_disable();
+ p = this_cpu_ptr(&var1);
+ p->val = func2();
+
+While correct on a non-PREEMPT_RT kernel, this breaks on PREEMPT_RT because
+here migrate_disable() does not protect against reentrancy from a
+preempting task. A correct substitution for this case is::
+
+ func()
+ {
+ struct foo *p;
+
+ local_lock(&foo_lock);
+ p = this_cpu_ptr(&var1);
+ p->val = func2();
+
+On a non-PREEMPT_RT kernel this protects against reentrancy by disabling
+preemption. On a PREEMPT_RT kernel this is achieved by acquiring the
+underlying per-CPU spinlock.
+
+
+raw_spinlock_t on RT
+--------------------
Acquiring a raw_spinlock_t disables preemption and possibly also
interrupts, so the critical section must avoid acquiring a regular
@@ -325,22 +515,25 @@ Lock type nesting rules
The most basic rules are:
- - Lock types of the same lock category (sleeping, spinning) can nest
- arbitrarily as long as they respect the general lock ordering rules to
- prevent deadlocks.
+ - Lock types of the same lock category (sleeping, CPU local, spinning)
+ can nest arbitrarily as long as they respect the general lock ordering
+ rules to prevent deadlocks.
- - Sleeping lock types cannot nest inside spinning lock types.
+ - Sleeping lock types cannot nest inside CPU local and spinning lock types.
- - Spinning lock types can nest inside sleeping lock types.
+ - CPU local and spinning lock types can nest inside sleeping lock types.
+
+ - Spinning lock types can nest inside all lock types
These constraints apply both in PREEMPT_RT and otherwise.
The fact that PREEMPT_RT changes the lock category of spinlock_t and
-rwlock_t from spinning to sleeping means that they cannot be acquired while
-holding a raw spinlock. This results in the following nesting ordering:
+rwlock_t from spinning to sleeping and substitutes local_lock with a
+per-CPU spinlock_t means that they cannot be acquired while holding a raw
+spinlock. This results in the following nesting ordering:
1) Sleeping locks
- 2) spinlock_t and rwlock_t
+ 2) spinlock_t, rwlock_t, local_lock
3) raw_spinlock_t and bit spinlocks
Lockdep will complain if these constraints are violated, both in
diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h
new file mode 100644
index 0000000000000..e55010fa73296
--- /dev/null
+++ b/include/linux/local_lock.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_LOCAL_LOCK_H
+#define _LINUX_LOCAL_LOCK_H
+
+#include <linux/local_lock_internal.h>
+
+/**
+ * local_lock_init - Runtime initialize a lock instance
+ */
+#define local_lock_init(lock) __local_lock_init(lock)
+
+/**
+ * local_lock - Acquire a per CPU local lock
+ * @lock: The lock variable
+ */
+#define local_lock(lock) __local_lock(lock)
+
+/**
+ * local_lock_irq - Acquire a per CPU local lock and disable interrupts
+ * @lock: The lock variable
+ */
+#define local_lock_irq(lock) __local_lock_irq(lock)
+
+/**
+ * local_lock_irqsave - Acquire a per CPU local lock, save and disable
+ * interrupts
+ * @lock: The lock variable
+ * @flags: Storage for interrupt flags
+ */
+#define local_lock_irqsave(lock, flags) \
+ __local_lock_irqsave(lock, flags)
+
+/**
+ * local_unlock - Release a per CPU local lock
+ * @lock: The lock variable
+ */
+#define local_unlock(lock) __local_unlock(lock)
+
+/**
+ * local_unlock_irq - Release a per CPU local lock and enable interrupts
+ * @lock: The lock variable
+ */
+#define local_unlock_irq(lock) __local_unlock_irq(lock)
+
+/**
+ * local_unlock_irqrestore - Release a per CPU local lock and restore
+ * interrupt flags
+ * @lock: The lock variable
+ * @flags: Interrupt flags to restore
+ */
+#define local_unlock_irqrestore(lock, flags) \
+ __local_unlock_irqrestore(lock, flags)
+
+#endif
diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
new file mode 100644
index 0000000000000..4a8795b21d774
--- /dev/null
+++ b/include/linux/local_lock_internal.h
@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_LOCAL_LOCK_H
+# error "Do not include directly, include linux/local_lock.h"
+#endif
+
+#include <linux/percpu-defs.h>
+#include <linux/lockdep.h>
+
+typedef struct {
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ struct lockdep_map dep_map;
+ struct task_struct *owner;
+#endif
+} local_lock_t;
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# define LL_DEP_MAP_INIT(lockname) \
+ .dep_map = { \
+ .name = #lockname, \
+ .wait_type_inner = LD_WAIT_CONFIG, \
+ }
+#else
+# define LL_DEP_MAP_INIT(lockname)
+#endif
+
+#define INIT_LOCAL_LOCK(lockname) { LL_DEP_MAP_INIT(lockname) }
+
+#define __local_lock_init(lock) \
+do { \
+ static struct lock_class_key __key; \
+ \
+ debug_check_no_locks_freed((void *)lock, sizeof(*lock));\
+ lockdep_init_map_wait(&(lock)->dep_map, #lock, &__key, 0, LD_WAIT_CONFIG);\
+} while (0)
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+static inline void local_lock_acquire(local_lock_t *l)
+{
+ lock_map_acquire(&l->dep_map);
+ DEBUG_LOCKS_WARN_ON(l->owner);
+ l->owner = current;
+}
+
+static inline void local_lock_release(local_lock_t *l)
+{
+ DEBUG_LOCKS_WARN_ON(l->owner != current);
+ l->owner = NULL;
+ lock_map_release(&l->dep_map);
+}
+
+#else /* CONFIG_DEBUG_LOCK_ALLOC */
+static inline void local_lock_acquire(local_lock_t *l) { }
+static inline void local_lock_release(local_lock_t *l) { }
+#endif /* !CONFIG_DEBUG_LOCK_ALLOC */
+
+#define __local_lock(lock) \
+ do { \
+ preempt_disable(); \
+ local_lock_acquire(this_cpu_ptr(lock)); \
+ } while (0)
+
+#define __local_lock_irq(lock) \
+ do { \
+ local_irq_disable(); \
+ local_lock_acquire(this_cpu_ptr(lock)); \
+ } while (0)
+
+#define __local_lock_irqsave(lock, flags) \
+ do { \
+ local_irq_save(flags); \
+ local_lock_acquire(this_cpu_ptr(lock)); \
+ } while (0)
+
+#define __local_unlock(lock) \
+ do { \
+ local_lock_release(this_cpu_ptr(lock)); \
+ preempt_enable(); \
+ } while (0)
+
+#define __local_unlock_irq(lock) \
+ do { \
+ local_lock_release(this_cpu_ptr(lock)); \
+ local_irq_enable(); \
+ } while (0)
+
+#define __local_unlock_irqrestore(lock, flags) \
+ do { \
+ local_lock_release(this_cpu_ptr(lock)); \
+ local_irq_restore(flags); \
+ } while (0)
--
2.27.0.rc0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [tip: locking/core] locking: Introduce local_lock()
2020-05-27 20:11 ` [PATCH v3 1/7] locking: " Sebastian Andrzej Siewior
@ 2020-06-01 9:52 ` tip-bot2 for Thomas Gleixner
0 siblings, 0 replies; 20+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2020-06-01 9:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Thomas Gleixner, Sebastian Andrzej Siewior, Ingo Molnar,
Peter Zijlstra, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 91710728d1725de51d06b40674abf6e860d592c7
Gitweb: https://git.kernel.org/tip/91710728d1725de51d06b40674abf6e860d592c7
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 27 May 2020 22:11:13 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 May 2020 10:31:09 +02:00
locking: Introduce local_lock()
preempt_disable() and local_irq_disable/save() are in principle per CPU big
kernel locks. This has several downsides:
- The protection scope is unknown
- Violation of protection rules is hard to detect by instrumentation
- For PREEMPT_RT such sections, unless in low level critical code, can
violate the preemptability constraints.
To address this PREEMPT_RT introduced the concept of local_locks which are
strictly per CPU.
The lock operations map to preempt_disable(), local_irq_disable/save() and
the enabling counterparts on non RT enabled kernels.
If lockdep is enabled local locks gain a lock map which tracks the usage
context. This will catch cases where an area is protected by
preempt_disable() but the access also happens from interrupt context. local
locks have identified quite a few such issues over the years, the most
recent example is:
b7d5dc21072cd ("random: add a spinlock_t to struct batched_entropy")
Aside of the lockdep coverage this also improves code readability as it
precisely annotates the protection scope.
PREEMPT_RT substitutes these local locks with 'sleeping' spinlocks to
protect such sections while maintaining preemtability and CPU locality.
local locks can replace:
- preempt_enable()/disable() pairs
- local_irq_disable/enable() pairs
- local_irq_save/restore() pairs
They are also used to replace code which implicitly disables preemption
like:
- get_cpu()/put_cpu()
- get_cpu_var()/put_cpu_var()
with PREEMPT_RT friendly constructs.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20200527201119.1692513-2-bigeasy@linutronix.de
---
Documentation/locking/locktypes.rst | 215 +++++++++++++++++++++++++--
include/linux/local_lock.h | 54 +++++++-
include/linux/local_lock_internal.h | 90 +++++++++++-
3 files changed, 348 insertions(+), 11 deletions(-)
create mode 100644 include/linux/local_lock.h
create mode 100644 include/linux/local_lock_internal.h
diff --git a/Documentation/locking/locktypes.rst b/Documentation/locking/locktypes.rst
index 09f45ce..1b577a8 100644
--- a/Documentation/locking/locktypes.rst
+++ b/Documentation/locking/locktypes.rst
@@ -13,6 +13,7 @@ The kernel provides a variety of locking primitives which can be divided
into two categories:
- Sleeping locks
+ - CPU local locks
- Spinning locks
This document conceptually describes these lock types and provides rules
@@ -44,9 +45,23 @@ Sleeping lock types:
On PREEMPT_RT kernels, these lock types are converted to sleeping locks:
+ - local_lock
- spinlock_t
- rwlock_t
+
+CPU local locks
+---------------
+
+ - local_lock
+
+On non-PREEMPT_RT kernels, local_lock functions are wrappers around
+preemption and interrupt disabling primitives. Contrary to other locking
+mechanisms, disabling preemption or interrupts are pure CPU local
+concurrency control mechanisms and not suited for inter-CPU concurrency
+control.
+
+
Spinning locks
--------------
@@ -67,6 +82,7 @@ can have suffixes which apply further protections:
_irqsave/restore() Save and disable / restore interrupt disabled state
=================== ====================================================
+
Owner semantics
===============
@@ -139,6 +155,56 @@ implementation, thus changing the fairness:
writer from starving readers.
+local_lock
+==========
+
+local_lock provides a named scope to critical sections which are protected
+by disabling preemption or interrupts.
+
+On non-PREEMPT_RT kernels local_lock operations map to the preemption and
+interrupt disabling and enabling primitives:
+
+ =========================== ======================
+ local_lock(&llock) preempt_disable()
+ local_unlock(&llock) preempt_enable()
+ local_lock_irq(&llock) local_irq_disable()
+ local_unlock_irq(&llock) local_irq_enable()
+ local_lock_save(&llock) local_irq_save()
+ local_lock_restore(&llock) local_irq_save()
+ =========================== ======================
+
+The named scope of local_lock has two advantages over the regular
+primitives:
+
+ - The lock name allows static analysis and is also a clear documentation
+ of the protection scope while the regular primitives are scopeless and
+ opaque.
+
+ - If lockdep is enabled the local_lock gains a lockmap which allows to
+ validate the correctness of the protection. This can detect cases where
+ e.g. a function using preempt_disable() as protection mechanism is
+ invoked from interrupt or soft-interrupt context. Aside of that
+ lockdep_assert_held(&llock) works as with any other locking primitive.
+
+local_lock and PREEMPT_RT
+-------------------------
+
+PREEMPT_RT kernels map local_lock to a per-CPU spinlock_t, thus changing
+semantics:
+
+ - All spinlock_t changes also apply to local_lock.
+
+local_lock usage
+----------------
+
+local_lock should be used in situations where disabling preemption or
+interrupts is the appropriate form of concurrency control to protect
+per-CPU data structures on a non PREEMPT_RT kernel.
+
+local_lock is not suitable to protect against preemption or interrupts on a
+PREEMPT_RT kernel due to the PREEMPT_RT specific spinlock_t semantics.
+
+
raw_spinlock_t and spinlock_t
=============================
@@ -258,10 +324,82 @@ implementation, thus changing semantics:
PREEMPT_RT caveats
==================
+local_lock on RT
+----------------
+
+The mapping of local_lock to spinlock_t on PREEMPT_RT kernels has a few
+implications. For example, on a non-PREEMPT_RT kernel the following code
+sequence works as expected::
+
+ local_lock_irq(&local_lock);
+ raw_spin_lock(&lock);
+
+and is fully equivalent to::
+
+ raw_spin_lock_irq(&lock);
+
+On a PREEMPT_RT kernel this code sequence breaks because local_lock_irq()
+is mapped to a per-CPU spinlock_t which neither disables interrupts nor
+preemption. The following code sequence works perfectly correct on both
+PREEMPT_RT and non-PREEMPT_RT kernels::
+
+ local_lock_irq(&local_lock);
+ spin_lock(&lock);
+
+Another caveat with local locks is that each local_lock has a specific
+protection scope. So the following substitution is wrong::
+
+ func1()
+ {
+ local_irq_save(flags); -> local_lock_irqsave(&local_lock_1, flags);
+ func3();
+ local_irq_restore(flags); -> local_lock_irqrestore(&local_lock_1, flags);
+ }
+
+ func2()
+ {
+ local_irq_save(flags); -> local_lock_irqsave(&local_lock_2, flags);
+ func3();
+ local_irq_restore(flags); -> local_lock_irqrestore(&local_lock_2, flags);
+ }
+
+ func3()
+ {
+ lockdep_assert_irqs_disabled();
+ access_protected_data();
+ }
+
+On a non-PREEMPT_RT kernel this works correctly, but on a PREEMPT_RT kernel
+local_lock_1 and local_lock_2 are distinct and cannot serialize the callers
+of func3(). Also the lockdep assert will trigger on a PREEMPT_RT kernel
+because local_lock_irqsave() does not disable interrupts due to the
+PREEMPT_RT-specific semantics of spinlock_t. The correct substitution is::
+
+ func1()
+ {
+ local_irq_save(flags); -> local_lock_irqsave(&local_lock, flags);
+ func3();
+ local_irq_restore(flags); -> local_lock_irqrestore(&local_lock, flags);
+ }
+
+ func2()
+ {
+ local_irq_save(flags); -> local_lock_irqsave(&local_lock, flags);
+ func3();
+ local_irq_restore(flags); -> local_lock_irqrestore(&local_lock, flags);
+ }
+
+ func3()
+ {
+ lockdep_assert_held(&local_lock);
+ access_protected_data();
+ }
+
+
spinlock_t and rwlock_t
-----------------------
-These changes in spinlock_t and rwlock_t semantics on PREEMPT_RT kernels
+The changes in spinlock_t and rwlock_t semantics on PREEMPT_RT kernels
have a few implications. For example, on a non-PREEMPT_RT kernel the
following code sequence works as expected::
@@ -282,9 +420,61 @@ local_lock mechanism. Acquiring the local_lock pins the task to a CPU,
allowing things like per-CPU interrupt disabled locks to be acquired.
However, this approach should be used only where absolutely necessary.
+A typical scenario is protection of per-CPU variables in thread context::
-raw_spinlock_t
---------------
+ struct foo *p = get_cpu_ptr(&var1);
+
+ spin_lock(&p->lock);
+ p->count += this_cpu_read(var2);
+
+This is correct code on a non-PREEMPT_RT kernel, but on a PREEMPT_RT kernel
+this breaks. The PREEMPT_RT-specific change of spinlock_t semantics does
+not allow to acquire p->lock because get_cpu_ptr() implicitly disables
+preemption. The following substitution works on both kernels::
+
+ struct foo *p;
+
+ migrate_disable();
+ p = this_cpu_ptr(&var1);
+ spin_lock(&p->lock);
+ p->count += this_cpu_read(var2);
+
+On a non-PREEMPT_RT kernel migrate_disable() maps to preempt_disable()
+which makes the above code fully equivalent. On a PREEMPT_RT kernel
+migrate_disable() ensures that the task is pinned on the current CPU which
+in turn guarantees that the per-CPU access to var1 and var2 are staying on
+the same CPU.
+
+The migrate_disable() substitution is not valid for the following
+scenario::
+
+ func()
+ {
+ struct foo *p;
+
+ migrate_disable();
+ p = this_cpu_ptr(&var1);
+ p->val = func2();
+
+While correct on a non-PREEMPT_RT kernel, this breaks on PREEMPT_RT because
+here migrate_disable() does not protect against reentrancy from a
+preempting task. A correct substitution for this case is::
+
+ func()
+ {
+ struct foo *p;
+
+ local_lock(&foo_lock);
+ p = this_cpu_ptr(&var1);
+ p->val = func2();
+
+On a non-PREEMPT_RT kernel this protects against reentrancy by disabling
+preemption. On a PREEMPT_RT kernel this is achieved by acquiring the
+underlying per-CPU spinlock.
+
+
+raw_spinlock_t on RT
+--------------------
Acquiring a raw_spinlock_t disables preemption and possibly also
interrupts, so the critical section must avoid acquiring a regular
@@ -325,22 +515,25 @@ Lock type nesting rules
The most basic rules are:
- - Lock types of the same lock category (sleeping, spinning) can nest
- arbitrarily as long as they respect the general lock ordering rules to
- prevent deadlocks.
+ - Lock types of the same lock category (sleeping, CPU local, spinning)
+ can nest arbitrarily as long as they respect the general lock ordering
+ rules to prevent deadlocks.
+
+ - Sleeping lock types cannot nest inside CPU local and spinning lock types.
- - Sleeping lock types cannot nest inside spinning lock types.
+ - CPU local and spinning lock types can nest inside sleeping lock types.
- - Spinning lock types can nest inside sleeping lock types.
+ - Spinning lock types can nest inside all lock types
These constraints apply both in PREEMPT_RT and otherwise.
The fact that PREEMPT_RT changes the lock category of spinlock_t and
-rwlock_t from spinning to sleeping means that they cannot be acquired while
-holding a raw spinlock. This results in the following nesting ordering:
+rwlock_t from spinning to sleeping and substitutes local_lock with a
+per-CPU spinlock_t means that they cannot be acquired while holding a raw
+spinlock. This results in the following nesting ordering:
1) Sleeping locks
- 2) spinlock_t and rwlock_t
+ 2) spinlock_t, rwlock_t, local_lock
3) raw_spinlock_t and bit spinlocks
Lockdep will complain if these constraints are violated, both in
diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h
new file mode 100644
index 0000000..e55010f
--- /dev/null
+++ b/include/linux/local_lock.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_LOCAL_LOCK_H
+#define _LINUX_LOCAL_LOCK_H
+
+#include <linux/local_lock_internal.h>
+
+/**
+ * local_lock_init - Runtime initialize a lock instance
+ */
+#define local_lock_init(lock) __local_lock_init(lock)
+
+/**
+ * local_lock - Acquire a per CPU local lock
+ * @lock: The lock variable
+ */
+#define local_lock(lock) __local_lock(lock)
+
+/**
+ * local_lock_irq - Acquire a per CPU local lock and disable interrupts
+ * @lock: The lock variable
+ */
+#define local_lock_irq(lock) __local_lock_irq(lock)
+
+/**
+ * local_lock_irqsave - Acquire a per CPU local lock, save and disable
+ * interrupts
+ * @lock: The lock variable
+ * @flags: Storage for interrupt flags
+ */
+#define local_lock_irqsave(lock, flags) \
+ __local_lock_irqsave(lock, flags)
+
+/**
+ * local_unlock - Release a per CPU local lock
+ * @lock: The lock variable
+ */
+#define local_unlock(lock) __local_unlock(lock)
+
+/**
+ * local_unlock_irq - Release a per CPU local lock and enable interrupts
+ * @lock: The lock variable
+ */
+#define local_unlock_irq(lock) __local_unlock_irq(lock)
+
+/**
+ * local_unlock_irqrestore - Release a per CPU local lock and restore
+ * interrupt flags
+ * @lock: The lock variable
+ * @flags: Interrupt flags to restore
+ */
+#define local_unlock_irqrestore(lock, flags) \
+ __local_unlock_irqrestore(lock, flags)
+
+#endif
diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
new file mode 100644
index 0000000..4a8795b
--- /dev/null
+++ b/include/linux/local_lock_internal.h
@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_LOCAL_LOCK_H
+# error "Do not include directly, include linux/local_lock.h"
+#endif
+
+#include <linux/percpu-defs.h>
+#include <linux/lockdep.h>
+
+typedef struct {
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ struct lockdep_map dep_map;
+ struct task_struct *owner;
+#endif
+} local_lock_t;
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# define LL_DEP_MAP_INIT(lockname) \
+ .dep_map = { \
+ .name = #lockname, \
+ .wait_type_inner = LD_WAIT_CONFIG, \
+ }
+#else
+# define LL_DEP_MAP_INIT(lockname)
+#endif
+
+#define INIT_LOCAL_LOCK(lockname) { LL_DEP_MAP_INIT(lockname) }
+
+#define __local_lock_init(lock) \
+do { \
+ static struct lock_class_key __key; \
+ \
+ debug_check_no_locks_freed((void *)lock, sizeof(*lock));\
+ lockdep_init_map_wait(&(lock)->dep_map, #lock, &__key, 0, LD_WAIT_CONFIG);\
+} while (0)
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+static inline void local_lock_acquire(local_lock_t *l)
+{
+ lock_map_acquire(&l->dep_map);
+ DEBUG_LOCKS_WARN_ON(l->owner);
+ l->owner = current;
+}
+
+static inline void local_lock_release(local_lock_t *l)
+{
+ DEBUG_LOCKS_WARN_ON(l->owner != current);
+ l->owner = NULL;
+ lock_map_release(&l->dep_map);
+}
+
+#else /* CONFIG_DEBUG_LOCK_ALLOC */
+static inline void local_lock_acquire(local_lock_t *l) { }
+static inline void local_lock_release(local_lock_t *l) { }
+#endif /* !CONFIG_DEBUG_LOCK_ALLOC */
+
+#define __local_lock(lock) \
+ do { \
+ preempt_disable(); \
+ local_lock_acquire(this_cpu_ptr(lock)); \
+ } while (0)
+
+#define __local_lock_irq(lock) \
+ do { \
+ local_irq_disable(); \
+ local_lock_acquire(this_cpu_ptr(lock)); \
+ } while (0)
+
+#define __local_lock_irqsave(lock, flags) \
+ do { \
+ local_irq_save(flags); \
+ local_lock_acquire(this_cpu_ptr(lock)); \
+ } while (0)
+
+#define __local_unlock(lock) \
+ do { \
+ local_lock_release(this_cpu_ptr(lock)); \
+ preempt_enable(); \
+ } while (0)
+
+#define __local_unlock_irq(lock) \
+ do { \
+ local_lock_release(this_cpu_ptr(lock)); \
+ local_irq_enable(); \
+ } while (0)
+
+#define __local_unlock_irqrestore(lock, flags) \
+ do { \
+ local_lock_release(this_cpu_ptr(lock)); \
+ local_irq_restore(flags); \
+ } while (0)
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/7] radix-tree: Use local_lock for protection
2020-05-27 20:11 [PATCH v3 0/7] Introduce local_lock() Sebastian Andrzej Siewior
2020-05-27 20:11 ` [PATCH v3 1/7] locking: " Sebastian Andrzej Siewior
@ 2020-05-27 20:11 ` Sebastian Andrzej Siewior
2020-06-01 9:52 ` [tip: locking/core] " tip-bot2 for Sebastian Andrzej Siewior
2020-05-27 20:11 ` [PATCH v3 3/7] mm/swap: " Sebastian Andrzej Siewior
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-27 20:11 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Steven Rostedt, Will Deacon,
Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Sebastian Andrzej Siewior, linux-fsdevel
The radix-tree and idr preload mechanisms use preempt_disable() to protect
the complete operation between xxx_preload() and xxx_preload_end().
As the code inside the preempt disabled section acquires regular spinlocks,
which are converted to 'sleeping' spinlocks on a PREEMPT_RT kernel and
eventually calls into a memory allocator, this conflicts with the RT
semantics.
Convert it to a local_lock which allows RT kernels to substitute them with
a real per CPU lock. On non RT kernels this maps to preempt_disable() as
before, but provides also lockdep coverage of the critical region.
No functional change.
Cc: Matthew Wilcox <willy@infradead.org>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/idr.h | 2 +-
include/linux/radix-tree.h | 11 ++++++++++-
lib/radix-tree.c | 20 +++++++++-----------
3 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index ac6e946b6767b..3ade03e5c7af3 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -171,7 +171,7 @@ static inline bool idr_is_empty(const struct idr *idr)
*/
static inline void idr_preload_end(void)
{
- preempt_enable();
+ local_unlock(&radix_tree_preloads.lock);
}
/**
diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
index 63e62372443a5..c2a9f7c907273 100644
--- a/include/linux/radix-tree.h
+++ b/include/linux/radix-tree.h
@@ -16,11 +16,20 @@
#include <linux/spinlock.h>
#include <linux/types.h>
#include <linux/xarray.h>
+#include <linux/local_lock.h>
/* Keep unconverted code working */
#define radix_tree_root xarray
#define radix_tree_node xa_node
+struct radix_tree_preload {
+ local_lock_t lock;
+ unsigned nr;
+ /* nodes->parent points to next preallocated node */
+ struct radix_tree_node *nodes;
+};
+DECLARE_PER_CPU(struct radix_tree_preload, radix_tree_preloads);
+
/*
* The bottom two bits of the slot determine how the remaining bits in the
* slot are interpreted:
@@ -245,7 +254,7 @@ int radix_tree_tagged(const struct radix_tree_root *, unsigned int tag);
static inline void radix_tree_preload_end(void)
{
- preempt_enable();
+ local_unlock(&radix_tree_preloads.lock);
}
void __rcu **idr_get_free(struct radix_tree_root *root,
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 2ee6ae3b0ade0..34e406fe561fe 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -20,6 +20,7 @@
#include <linux/kernel.h>
#include <linux/kmemleak.h>
#include <linux/percpu.h>
+#include <linux/local_lock.h>
#include <linux/preempt.h> /* in_interrupt() */
#include <linux/radix-tree.h>
#include <linux/rcupdate.h>
@@ -27,7 +28,6 @@
#include <linux/string.h>
#include <linux/xarray.h>
-
/*
* Radix tree node cache.
*/
@@ -58,12 +58,10 @@ struct kmem_cache *radix_tree_node_cachep;
/*
* Per-cpu pool of preloaded nodes
*/
-struct radix_tree_preload {
- unsigned nr;
- /* nodes->parent points to next preallocated node */
- struct radix_tree_node *nodes;
+DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = {
+ .lock = INIT_LOCAL_LOCK(lock),
};
-static DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = { 0, };
+EXPORT_PER_CPU_SYMBOL_GPL(radix_tree_preloads);
static inline struct radix_tree_node *entry_to_node(void *ptr)
{
@@ -332,14 +330,14 @@ static __must_check int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
*/
gfp_mask &= ~__GFP_ACCOUNT;
- preempt_disable();
+ local_lock(&radix_tree_preloads.lock);
rtp = this_cpu_ptr(&radix_tree_preloads);
while (rtp->nr < nr) {
- preempt_enable();
+ local_unlock(&radix_tree_preloads.lock);
node = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
if (node == NULL)
goto out;
- preempt_disable();
+ local_lock(&radix_tree_preloads.lock);
rtp = this_cpu_ptr(&radix_tree_preloads);
if (rtp->nr < nr) {
node->parent = rtp->nodes;
@@ -381,7 +379,7 @@ int radix_tree_maybe_preload(gfp_t gfp_mask)
if (gfpflags_allow_blocking(gfp_mask))
return __radix_tree_preload(gfp_mask, RADIX_TREE_PRELOAD_SIZE);
/* Preloading doesn't help anything with this gfp mask, skip it */
- preempt_disable();
+ local_lock(&radix_tree_preloads.lock);
return 0;
}
EXPORT_SYMBOL(radix_tree_maybe_preload);
@@ -1470,7 +1468,7 @@ EXPORT_SYMBOL(radix_tree_tagged);
void idr_preload(gfp_t gfp_mask)
{
if (__radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE))
- preempt_disable();
+ local_lock(&radix_tree_preloads.lock);
}
EXPORT_SYMBOL(idr_preload);
--
2.27.0.rc0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [tip: locking/core] radix-tree: Use local_lock for protection
2020-05-27 20:11 ` [PATCH v3 2/7] radix-tree: Use local_lock for protection Sebastian Andrzej Siewior
@ 2020-06-01 9:52 ` tip-bot2 for Sebastian Andrzej Siewior
0 siblings, 0 replies; 20+ messages in thread
From: tip-bot2 for Sebastian Andrzej Siewior @ 2020-06-01 9:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Sebastian Andrzej Siewior, Ingo Molnar, Peter Zijlstra, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: cfa6705d89b6562f79c40c249f8d94073c4276e4
Gitweb: https://git.kernel.org/tip/cfa6705d89b6562f79c40c249f8d94073c4276e4
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Wed, 27 May 2020 22:11:14 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 May 2020 10:31:09 +02:00
radix-tree: Use local_lock for protection
The radix-tree and idr preload mechanisms use preempt_disable() to protect
the complete operation between xxx_preload() and xxx_preload_end().
As the code inside the preempt disabled section acquires regular spinlocks,
which are converted to 'sleeping' spinlocks on a PREEMPT_RT kernel and
eventually calls into a memory allocator, this conflicts with the RT
semantics.
Convert it to a local_lock which allows RT kernels to substitute them with
a real per CPU lock. On non RT kernels this maps to preempt_disable() as
before, but provides also lockdep coverage of the critical region.
No functional change.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20200527201119.1692513-3-bigeasy@linutronix.de
---
include/linux/idr.h | 2 +-
include/linux/radix-tree.h | 11 ++++++++++-
lib/radix-tree.c | 20 +++++++++-----------
3 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index ac6e946..3ade03e 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -171,7 +171,7 @@ static inline bool idr_is_empty(const struct idr *idr)
*/
static inline void idr_preload_end(void)
{
- preempt_enable();
+ local_unlock(&radix_tree_preloads.lock);
}
/**
diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
index 63e6237..c2a9f7c 100644
--- a/include/linux/radix-tree.h
+++ b/include/linux/radix-tree.h
@@ -16,11 +16,20 @@
#include <linux/spinlock.h>
#include <linux/types.h>
#include <linux/xarray.h>
+#include <linux/local_lock.h>
/* Keep unconverted code working */
#define radix_tree_root xarray
#define radix_tree_node xa_node
+struct radix_tree_preload {
+ local_lock_t lock;
+ unsigned nr;
+ /* nodes->parent points to next preallocated node */
+ struct radix_tree_node *nodes;
+};
+DECLARE_PER_CPU(struct radix_tree_preload, radix_tree_preloads);
+
/*
* The bottom two bits of the slot determine how the remaining bits in the
* slot are interpreted:
@@ -245,7 +254,7 @@ int radix_tree_tagged(const struct radix_tree_root *, unsigned int tag);
static inline void radix_tree_preload_end(void)
{
- preempt_enable();
+ local_unlock(&radix_tree_preloads.lock);
}
void __rcu **idr_get_free(struct radix_tree_root *root,
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 2ee6ae3..34e406f 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -20,6 +20,7 @@
#include <linux/kernel.h>
#include <linux/kmemleak.h>
#include <linux/percpu.h>
+#include <linux/local_lock.h>
#include <linux/preempt.h> /* in_interrupt() */
#include <linux/radix-tree.h>
#include <linux/rcupdate.h>
@@ -27,7 +28,6 @@
#include <linux/string.h>
#include <linux/xarray.h>
-
/*
* Radix tree node cache.
*/
@@ -58,12 +58,10 @@ struct kmem_cache *radix_tree_node_cachep;
/*
* Per-cpu pool of preloaded nodes
*/
-struct radix_tree_preload {
- unsigned nr;
- /* nodes->parent points to next preallocated node */
- struct radix_tree_node *nodes;
+DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = {
+ .lock = INIT_LOCAL_LOCK(lock),
};
-static DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = { 0, };
+EXPORT_PER_CPU_SYMBOL_GPL(radix_tree_preloads);
static inline struct radix_tree_node *entry_to_node(void *ptr)
{
@@ -332,14 +330,14 @@ static __must_check int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
*/
gfp_mask &= ~__GFP_ACCOUNT;
- preempt_disable();
+ local_lock(&radix_tree_preloads.lock);
rtp = this_cpu_ptr(&radix_tree_preloads);
while (rtp->nr < nr) {
- preempt_enable();
+ local_unlock(&radix_tree_preloads.lock);
node = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
if (node == NULL)
goto out;
- preempt_disable();
+ local_lock(&radix_tree_preloads.lock);
rtp = this_cpu_ptr(&radix_tree_preloads);
if (rtp->nr < nr) {
node->parent = rtp->nodes;
@@ -381,7 +379,7 @@ int radix_tree_maybe_preload(gfp_t gfp_mask)
if (gfpflags_allow_blocking(gfp_mask))
return __radix_tree_preload(gfp_mask, RADIX_TREE_PRELOAD_SIZE);
/* Preloading doesn't help anything with this gfp mask, skip it */
- preempt_disable();
+ local_lock(&radix_tree_preloads.lock);
return 0;
}
EXPORT_SYMBOL(radix_tree_maybe_preload);
@@ -1470,7 +1468,7 @@ EXPORT_SYMBOL(radix_tree_tagged);
void idr_preload(gfp_t gfp_mask)
{
if (__radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE))
- preempt_disable();
+ local_lock(&radix_tree_preloads.lock);
}
EXPORT_SYMBOL(idr_preload);
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 3/7] mm/swap: Use local_lock for protection
2020-05-27 20:11 [PATCH v3 0/7] Introduce local_lock() Sebastian Andrzej Siewior
2020-05-27 20:11 ` [PATCH v3 1/7] locking: " Sebastian Andrzej Siewior
2020-05-27 20:11 ` [PATCH v3 2/7] radix-tree: Use local_lock for protection Sebastian Andrzej Siewior
@ 2020-05-27 20:11 ` Sebastian Andrzej Siewior
2020-06-01 9:52 ` [tip: locking/core] " tip-bot2 for Ingo Molnar
2020-05-27 20:11 ` [PATCH v3 4/7] squashfs: make use of local lock in multi_cpu decompressor Sebastian Andrzej Siewior
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-27 20:11 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Steven Rostedt, Will Deacon,
Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Andrew Morton, linux-mm,
Sebastian Andrzej Siewior
From: Ingo Molnar <mingo@kernel.org>
The various struct pagevec per CPU variables are protected by disabling
either preemption or interrupts across the critical sections. Inside
these sections spinlocks have to be acquired.
These spinlocks are regular spinlock_t types which are converted to
"sleeping" spinlocks on PREEMPT_RT enabled kernels. Obviously sleeping
locks cannot be acquired in preemption or interrupt disabled sections.
local locks provide a trivial way to substitute preempt and interrupt
disable instances. On a non PREEMPT_RT enabled kernel local_lock() maps
to preempt_disable() and local_lock_irq() to local_irq_disable().
Create lru_rotate_pvecs containing the pagevec and the locallock.
Create lru_pvecs containing the remaining pagevecs and the locallock.
Add lru_add_drain_cpu_zone() which is used from compact_zone() to avoid
exporting the pvec structure.
Change the relevant call sites to acquire these locks instead of using
preempt_disable() / get_cpu() / get_cpu_var() and local_irq_disable() /
local_irq_save().
There is neither a functional change nor a change in the generated
binary code for non PREEMPT_RT enabled non-debug kernels.
When lockdep is enabled local locks have lockdep maps embedded. These
allow lockdep to validate the protections, i.e. inappropriate usage of a
preemption only protected sections would result in a lockdep warning
while the same problem would not be noticed with a plain
preempt_disable() based protection.
local locks also improve readability as they provide a named scope for
the protections while preempt/interrupt disable are opaque scopeless.
Finally local locks allow PREEMPT_RT to substitute them with real
locking primitives to ensure the correctness of operation in a fully
preemptible kernel.
[ bigeasy: Adopted to use local_lock ]
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/swap.h | 1 +
mm/compaction.c | 6 +--
mm/swap.c | 118 +++++++++++++++++++++++++++++--------------
3 files changed, 82 insertions(+), 43 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index e1bbf7a16b276..25181d2dd0b9f 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -337,6 +337,7 @@ extern void activate_page(struct page *);
extern void mark_page_accessed(struct page *);
extern void lru_add_drain(void);
extern void lru_add_drain_cpu(int cpu);
+extern void lru_add_drain_cpu_zone(struct zone *zone);
extern void lru_add_drain_all(void);
extern void rotate_reclaimable_page(struct page *page);
extern void deactivate_file_page(struct page *page);
diff --git a/mm/compaction.c b/mm/compaction.c
index 46f0fcc93081e..c9d659e6a02c5 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2243,15 +2243,11 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
* would succeed.
*/
if (cc->order > 0 && last_migrated_pfn) {
- int cpu;
unsigned long current_block_start =
block_start_pfn(cc->migrate_pfn, cc->order);
if (last_migrated_pfn < current_block_start) {
- cpu = get_cpu();
- lru_add_drain_cpu(cpu);
- drain_local_pages(cc->zone);
- put_cpu();
+ lru_add_drain_cpu_zone(cc->zone);
/* No more flushing until we migrate again */
last_migrated_pfn = 0;
}
diff --git a/mm/swap.c b/mm/swap.c
index bf9a79fed62d7..0ac463d44cff4 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -35,6 +35,7 @@
#include <linux/uio.h>
#include <linux/hugetlb.h>
#include <linux/page_idle.h>
+#include <linux/local_lock.h>
#include "internal.h"
@@ -44,14 +45,32 @@
/* How many pages do we try to swap or page in/out together? */
int page_cluster;
-static DEFINE_PER_CPU(struct pagevec, lru_add_pvec);
-static DEFINE_PER_CPU(struct pagevec, lru_rotate_pvecs);
-static DEFINE_PER_CPU(struct pagevec, lru_deactivate_file_pvecs);
-static DEFINE_PER_CPU(struct pagevec, lru_deactivate_pvecs);
-static DEFINE_PER_CPU(struct pagevec, lru_lazyfree_pvecs);
+/* Protecting only lru_rotate.pvec which requires disabling interrupts */
+struct lru_rotate {
+ local_lock_t lock;
+ struct pagevec pvec;
+};
+static DEFINE_PER_CPU(struct lru_rotate, lru_rotate) = {
+ .lock = INIT_LOCAL_LOCK(lock),
+};
+
+/*
+ * The following struct pagevec are grouped together because they are protected
+ * by disabling preemption (and interrupts remain enabled).
+ */
+struct lru_pvecs {
+ local_lock_t lock;
+ struct pagevec lru_add;
+ struct pagevec lru_deactivate_file;
+ struct pagevec lru_deactivate;
+ struct pagevec lru_lazyfree;
#ifdef CONFIG_SMP
-static DEFINE_PER_CPU(struct pagevec, activate_page_pvecs);
+ struct pagevec activate_page;
#endif
+};
+static DEFINE_PER_CPU(struct lru_pvecs, lru_pvecs) = {
+ .lock = INIT_LOCAL_LOCK(lock),
+};
/*
* This path almost never happens for VM activity - pages are normally
@@ -254,11 +273,11 @@ void rotate_reclaimable_page(struct page *page)
unsigned long flags;
get_page(page);
- local_irq_save(flags);
- pvec = this_cpu_ptr(&lru_rotate_pvecs);
+ local_lock_irqsave(&lru_rotate.lock, flags);
+ pvec = this_cpu_ptr(&lru_rotate.pvec);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_move_tail(pvec);
- local_irq_restore(flags);
+ local_unlock_irqrestore(&lru_rotate.lock, flags);
}
}
@@ -293,7 +312,7 @@ static void __activate_page(struct page *page, struct lruvec *lruvec,
#ifdef CONFIG_SMP
static void activate_page_drain(int cpu)
{
- struct pagevec *pvec = &per_cpu(activate_page_pvecs, cpu);
+ struct pagevec *pvec = &per_cpu(lru_pvecs.activate_page, cpu);
if (pagevec_count(pvec))
pagevec_lru_move_fn(pvec, __activate_page, NULL);
@@ -301,19 +320,21 @@ static void activate_page_drain(int cpu)
static bool need_activate_page_drain(int cpu)
{
- return pagevec_count(&per_cpu(activate_page_pvecs, cpu)) != 0;
+ return pagevec_count(&per_cpu(lru_pvecs.activate_page, cpu)) != 0;
}
void activate_page(struct page *page)
{
page = compound_head(page);
if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
- struct pagevec *pvec = &get_cpu_var(activate_page_pvecs);
+ struct pagevec *pvec;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.activate_page);
get_page(page);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_lru_move_fn(pvec, __activate_page, NULL);
- put_cpu_var(activate_page_pvecs);
+ local_unlock(&lru_pvecs.lock);
}
}
@@ -335,9 +356,12 @@ void activate_page(struct page *page)
static void __lru_cache_activate_page(struct page *page)
{
- struct pagevec *pvec = &get_cpu_var(lru_add_pvec);
+ struct pagevec *pvec;
int i;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_add);
+
/*
* Search backwards on the optimistic assumption that the page being
* activated has just been added to this pagevec. Note that only
@@ -357,7 +381,7 @@ static void __lru_cache_activate_page(struct page *page)
}
}
- put_cpu_var(lru_add_pvec);
+ local_unlock(&lru_pvecs.lock);
}
/*
@@ -385,7 +409,7 @@ void mark_page_accessed(struct page *page)
} else if (!PageActive(page)) {
/*
* If the page is on the LRU, queue it for activation via
- * activate_page_pvecs. Otherwise, assume the page is on a
+ * lru_pvecs.activate_page. Otherwise, assume the page is on a
* pagevec, mark it active and it'll be moved to the active
* LRU on the next drain.
*/
@@ -404,12 +428,14 @@ EXPORT_SYMBOL(mark_page_accessed);
static void __lru_cache_add(struct page *page)
{
- struct pagevec *pvec = &get_cpu_var(lru_add_pvec);
+ struct pagevec *pvec;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_add);
get_page(page);
if (!pagevec_add(pvec, page) || PageCompound(page))
__pagevec_lru_add(pvec);
- put_cpu_var(lru_add_pvec);
+ local_unlock(&lru_pvecs.lock);
}
/**
@@ -593,30 +619,30 @@ static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec,
*/
void lru_add_drain_cpu(int cpu)
{
- struct pagevec *pvec = &per_cpu(lru_add_pvec, cpu);
+ struct pagevec *pvec = &per_cpu(lru_pvecs.lru_add, cpu);
if (pagevec_count(pvec))
__pagevec_lru_add(pvec);
- pvec = &per_cpu(lru_rotate_pvecs, cpu);
+ pvec = &per_cpu(lru_rotate.pvec, cpu);
if (pagevec_count(pvec)) {
unsigned long flags;
/* No harm done if a racing interrupt already did this */
- local_irq_save(flags);
+ local_lock_irqsave(&lru_rotate.lock, flags);
pagevec_move_tail(pvec);
- local_irq_restore(flags);
+ local_unlock_irqrestore(&lru_rotate.lock, flags);
}
- pvec = &per_cpu(lru_deactivate_file_pvecs, cpu);
+ pvec = &per_cpu(lru_pvecs.lru_deactivate_file, cpu);
if (pagevec_count(pvec))
pagevec_lru_move_fn(pvec, lru_deactivate_file_fn, NULL);
- pvec = &per_cpu(lru_deactivate_pvecs, cpu);
+ pvec = &per_cpu(lru_pvecs.lru_deactivate, cpu);
if (pagevec_count(pvec))
pagevec_lru_move_fn(pvec, lru_deactivate_fn, NULL);
- pvec = &per_cpu(lru_lazyfree_pvecs, cpu);
+ pvec = &per_cpu(lru_pvecs.lru_lazyfree, cpu);
if (pagevec_count(pvec))
pagevec_lru_move_fn(pvec, lru_lazyfree_fn, NULL);
@@ -641,11 +667,14 @@ void deactivate_file_page(struct page *page)
return;
if (likely(get_page_unless_zero(page))) {
- struct pagevec *pvec = &get_cpu_var(lru_deactivate_file_pvecs);
+ struct pagevec *pvec;
+
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_deactivate_file);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_lru_move_fn(pvec, lru_deactivate_file_fn, NULL);
- put_cpu_var(lru_deactivate_file_pvecs);
+ local_unlock(&lru_pvecs.lock);
}
}
@@ -660,12 +689,14 @@ void deactivate_file_page(struct page *page)
void deactivate_page(struct page *page)
{
if (PageLRU(page) && PageActive(page) && !PageUnevictable(page)) {
- struct pagevec *pvec = &get_cpu_var(lru_deactivate_pvecs);
+ struct pagevec *pvec;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_deactivate);
get_page(page);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_lru_move_fn(pvec, lru_deactivate_fn, NULL);
- put_cpu_var(lru_deactivate_pvecs);
+ local_unlock(&lru_pvecs.lock);
}
}
@@ -680,19 +711,30 @@ void mark_page_lazyfree(struct page *page)
{
if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) &&
!PageSwapCache(page) && !PageUnevictable(page)) {
- struct pagevec *pvec = &get_cpu_var(lru_lazyfree_pvecs);
+ struct pagevec *pvec;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_lazyfree);
get_page(page);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_lru_move_fn(pvec, lru_lazyfree_fn, NULL);
- put_cpu_var(lru_lazyfree_pvecs);
+ local_unlock(&lru_pvecs.lock);
}
}
void lru_add_drain(void)
{
- lru_add_drain_cpu(get_cpu());
- put_cpu();
+ local_lock(&lru_pvecs.lock);
+ lru_add_drain_cpu(smp_processor_id());
+ local_unlock(&lru_pvecs.lock);
+}
+
+void lru_add_drain_cpu_zone(struct zone *zone)
+{
+ local_lock(&lru_pvecs.lock);
+ lru_add_drain_cpu(smp_processor_id());
+ drain_local_pages(zone);
+ local_unlock(&lru_pvecs.lock);
}
#ifdef CONFIG_SMP
@@ -743,11 +785,11 @@ void lru_add_drain_all(void)
for_each_online_cpu(cpu) {
struct work_struct *work = &per_cpu(lru_add_drain_work, cpu);
- if (pagevec_count(&per_cpu(lru_add_pvec, cpu)) ||
- pagevec_count(&per_cpu(lru_rotate_pvecs, cpu)) ||
- pagevec_count(&per_cpu(lru_deactivate_file_pvecs, cpu)) ||
- pagevec_count(&per_cpu(lru_deactivate_pvecs, cpu)) ||
- pagevec_count(&per_cpu(lru_lazyfree_pvecs, cpu)) ||
+ if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
+ pagevec_count(&per_cpu(lru_rotate.pvec, cpu)) ||
+ pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
+ pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||
+ pagevec_count(&per_cpu(lru_pvecs.lru_lazyfree, cpu)) ||
need_activate_page_drain(cpu)) {
INIT_WORK(work, lru_add_drain_per_cpu);
queue_work_on(cpu, mm_percpu_wq, work);
--
2.27.0.rc0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [tip: locking/core] mm/swap: Use local_lock for protection
2020-05-27 20:11 ` [PATCH v3 3/7] mm/swap: " Sebastian Andrzej Siewior
@ 2020-06-01 9:52 ` tip-bot2 for Ingo Molnar
0 siblings, 0 replies; 20+ messages in thread
From: tip-bot2 for Ingo Molnar @ 2020-06-01 9:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Ingo Molnar, Sebastian Andrzej Siewior, Peter Zijlstra, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: b01b2141999936ac3e4746b7f76c0f204ae4b445
Gitweb: https://git.kernel.org/tip/b01b2141999936ac3e4746b7f76c0f204ae4b445
Author: Ingo Molnar <mingo@kernel.org>
AuthorDate: Wed, 27 May 2020 22:11:15 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 May 2020 10:31:10 +02:00
mm/swap: Use local_lock for protection
The various struct pagevec per CPU variables are protected by disabling
either preemption or interrupts across the critical sections. Inside
these sections spinlocks have to be acquired.
These spinlocks are regular spinlock_t types which are converted to
"sleeping" spinlocks on PREEMPT_RT enabled kernels. Obviously sleeping
locks cannot be acquired in preemption or interrupt disabled sections.
local locks provide a trivial way to substitute preempt and interrupt
disable instances. On a non PREEMPT_RT enabled kernel local_lock() maps
to preempt_disable() and local_lock_irq() to local_irq_disable().
Create lru_rotate_pvecs containing the pagevec and the locallock.
Create lru_pvecs containing the remaining pagevecs and the locallock.
Add lru_add_drain_cpu_zone() which is used from compact_zone() to avoid
exporting the pvec structure.
Change the relevant call sites to acquire these locks instead of using
preempt_disable() / get_cpu() / get_cpu_var() and local_irq_disable() /
local_irq_save().
There is neither a functional change nor a change in the generated
binary code for non PREEMPT_RT enabled non-debug kernels.
When lockdep is enabled local locks have lockdep maps embedded. These
allow lockdep to validate the protections, i.e. inappropriate usage of a
preemption only protected sections would result in a lockdep warning
while the same problem would not be noticed with a plain
preempt_disable() based protection.
local locks also improve readability as they provide a named scope for
the protections while preempt/interrupt disable are opaque scopeless.
Finally local locks allow PREEMPT_RT to substitute them with real
locking primitives to ensure the correctness of operation in a fully
preemptible kernel.
[ bigeasy: Adopted to use local_lock ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20200527201119.1692513-4-bigeasy@linutronix.de
---
include/linux/swap.h | 1 +-
mm/compaction.c | 6 +--
mm/swap.c | 118 ++++++++++++++++++++++++++++--------------
3 files changed, 82 insertions(+), 43 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index e1bbf7a..25181d2 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -337,6 +337,7 @@ extern void activate_page(struct page *);
extern void mark_page_accessed(struct page *);
extern void lru_add_drain(void);
extern void lru_add_drain_cpu(int cpu);
+extern void lru_add_drain_cpu_zone(struct zone *zone);
extern void lru_add_drain_all(void);
extern void rotate_reclaimable_page(struct page *page);
extern void deactivate_file_page(struct page *page);
diff --git a/mm/compaction.c b/mm/compaction.c
index 46f0fcc..c9d659e 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2243,15 +2243,11 @@ check_drain:
* would succeed.
*/
if (cc->order > 0 && last_migrated_pfn) {
- int cpu;
unsigned long current_block_start =
block_start_pfn(cc->migrate_pfn, cc->order);
if (last_migrated_pfn < current_block_start) {
- cpu = get_cpu();
- lru_add_drain_cpu(cpu);
- drain_local_pages(cc->zone);
- put_cpu();
+ lru_add_drain_cpu_zone(cc->zone);
/* No more flushing until we migrate again */
last_migrated_pfn = 0;
}
diff --git a/mm/swap.c b/mm/swap.c
index bf9a79f..0ac463d 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -35,6 +35,7 @@
#include <linux/uio.h>
#include <linux/hugetlb.h>
#include <linux/page_idle.h>
+#include <linux/local_lock.h>
#include "internal.h"
@@ -44,14 +45,32 @@
/* How many pages do we try to swap or page in/out together? */
int page_cluster;
-static DEFINE_PER_CPU(struct pagevec, lru_add_pvec);
-static DEFINE_PER_CPU(struct pagevec, lru_rotate_pvecs);
-static DEFINE_PER_CPU(struct pagevec, lru_deactivate_file_pvecs);
-static DEFINE_PER_CPU(struct pagevec, lru_deactivate_pvecs);
-static DEFINE_PER_CPU(struct pagevec, lru_lazyfree_pvecs);
+/* Protecting only lru_rotate.pvec which requires disabling interrupts */
+struct lru_rotate {
+ local_lock_t lock;
+ struct pagevec pvec;
+};
+static DEFINE_PER_CPU(struct lru_rotate, lru_rotate) = {
+ .lock = INIT_LOCAL_LOCK(lock),
+};
+
+/*
+ * The following struct pagevec are grouped together because they are protected
+ * by disabling preemption (and interrupts remain enabled).
+ */
+struct lru_pvecs {
+ local_lock_t lock;
+ struct pagevec lru_add;
+ struct pagevec lru_deactivate_file;
+ struct pagevec lru_deactivate;
+ struct pagevec lru_lazyfree;
#ifdef CONFIG_SMP
-static DEFINE_PER_CPU(struct pagevec, activate_page_pvecs);
+ struct pagevec activate_page;
#endif
+};
+static DEFINE_PER_CPU(struct lru_pvecs, lru_pvecs) = {
+ .lock = INIT_LOCAL_LOCK(lock),
+};
/*
* This path almost never happens for VM activity - pages are normally
@@ -254,11 +273,11 @@ void rotate_reclaimable_page(struct page *page)
unsigned long flags;
get_page(page);
- local_irq_save(flags);
- pvec = this_cpu_ptr(&lru_rotate_pvecs);
+ local_lock_irqsave(&lru_rotate.lock, flags);
+ pvec = this_cpu_ptr(&lru_rotate.pvec);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_move_tail(pvec);
- local_irq_restore(flags);
+ local_unlock_irqrestore(&lru_rotate.lock, flags);
}
}
@@ -293,7 +312,7 @@ static void __activate_page(struct page *page, struct lruvec *lruvec,
#ifdef CONFIG_SMP
static void activate_page_drain(int cpu)
{
- struct pagevec *pvec = &per_cpu(activate_page_pvecs, cpu);
+ struct pagevec *pvec = &per_cpu(lru_pvecs.activate_page, cpu);
if (pagevec_count(pvec))
pagevec_lru_move_fn(pvec, __activate_page, NULL);
@@ -301,19 +320,21 @@ static void activate_page_drain(int cpu)
static bool need_activate_page_drain(int cpu)
{
- return pagevec_count(&per_cpu(activate_page_pvecs, cpu)) != 0;
+ return pagevec_count(&per_cpu(lru_pvecs.activate_page, cpu)) != 0;
}
void activate_page(struct page *page)
{
page = compound_head(page);
if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
- struct pagevec *pvec = &get_cpu_var(activate_page_pvecs);
+ struct pagevec *pvec;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.activate_page);
get_page(page);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_lru_move_fn(pvec, __activate_page, NULL);
- put_cpu_var(activate_page_pvecs);
+ local_unlock(&lru_pvecs.lock);
}
}
@@ -335,9 +356,12 @@ void activate_page(struct page *page)
static void __lru_cache_activate_page(struct page *page)
{
- struct pagevec *pvec = &get_cpu_var(lru_add_pvec);
+ struct pagevec *pvec;
int i;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_add);
+
/*
* Search backwards on the optimistic assumption that the page being
* activated has just been added to this pagevec. Note that only
@@ -357,7 +381,7 @@ static void __lru_cache_activate_page(struct page *page)
}
}
- put_cpu_var(lru_add_pvec);
+ local_unlock(&lru_pvecs.lock);
}
/*
@@ -385,7 +409,7 @@ void mark_page_accessed(struct page *page)
} else if (!PageActive(page)) {
/*
* If the page is on the LRU, queue it for activation via
- * activate_page_pvecs. Otherwise, assume the page is on a
+ * lru_pvecs.activate_page. Otherwise, assume the page is on a
* pagevec, mark it active and it'll be moved to the active
* LRU on the next drain.
*/
@@ -404,12 +428,14 @@ EXPORT_SYMBOL(mark_page_accessed);
static void __lru_cache_add(struct page *page)
{
- struct pagevec *pvec = &get_cpu_var(lru_add_pvec);
+ struct pagevec *pvec;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_add);
get_page(page);
if (!pagevec_add(pvec, page) || PageCompound(page))
__pagevec_lru_add(pvec);
- put_cpu_var(lru_add_pvec);
+ local_unlock(&lru_pvecs.lock);
}
/**
@@ -593,30 +619,30 @@ static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec,
*/
void lru_add_drain_cpu(int cpu)
{
- struct pagevec *pvec = &per_cpu(lru_add_pvec, cpu);
+ struct pagevec *pvec = &per_cpu(lru_pvecs.lru_add, cpu);
if (pagevec_count(pvec))
__pagevec_lru_add(pvec);
- pvec = &per_cpu(lru_rotate_pvecs, cpu);
+ pvec = &per_cpu(lru_rotate.pvec, cpu);
if (pagevec_count(pvec)) {
unsigned long flags;
/* No harm done if a racing interrupt already did this */
- local_irq_save(flags);
+ local_lock_irqsave(&lru_rotate.lock, flags);
pagevec_move_tail(pvec);
- local_irq_restore(flags);
+ local_unlock_irqrestore(&lru_rotate.lock, flags);
}
- pvec = &per_cpu(lru_deactivate_file_pvecs, cpu);
+ pvec = &per_cpu(lru_pvecs.lru_deactivate_file, cpu);
if (pagevec_count(pvec))
pagevec_lru_move_fn(pvec, lru_deactivate_file_fn, NULL);
- pvec = &per_cpu(lru_deactivate_pvecs, cpu);
+ pvec = &per_cpu(lru_pvecs.lru_deactivate, cpu);
if (pagevec_count(pvec))
pagevec_lru_move_fn(pvec, lru_deactivate_fn, NULL);
- pvec = &per_cpu(lru_lazyfree_pvecs, cpu);
+ pvec = &per_cpu(lru_pvecs.lru_lazyfree, cpu);
if (pagevec_count(pvec))
pagevec_lru_move_fn(pvec, lru_lazyfree_fn, NULL);
@@ -641,11 +667,14 @@ void deactivate_file_page(struct page *page)
return;
if (likely(get_page_unless_zero(page))) {
- struct pagevec *pvec = &get_cpu_var(lru_deactivate_file_pvecs);
+ struct pagevec *pvec;
+
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_deactivate_file);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_lru_move_fn(pvec, lru_deactivate_file_fn, NULL);
- put_cpu_var(lru_deactivate_file_pvecs);
+ local_unlock(&lru_pvecs.lock);
}
}
@@ -660,12 +689,14 @@ void deactivate_file_page(struct page *page)
void deactivate_page(struct page *page)
{
if (PageLRU(page) && PageActive(page) && !PageUnevictable(page)) {
- struct pagevec *pvec = &get_cpu_var(lru_deactivate_pvecs);
+ struct pagevec *pvec;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_deactivate);
get_page(page);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_lru_move_fn(pvec, lru_deactivate_fn, NULL);
- put_cpu_var(lru_deactivate_pvecs);
+ local_unlock(&lru_pvecs.lock);
}
}
@@ -680,19 +711,30 @@ void mark_page_lazyfree(struct page *page)
{
if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) &&
!PageSwapCache(page) && !PageUnevictable(page)) {
- struct pagevec *pvec = &get_cpu_var(lru_lazyfree_pvecs);
+ struct pagevec *pvec;
+ local_lock(&lru_pvecs.lock);
+ pvec = this_cpu_ptr(&lru_pvecs.lru_lazyfree);
get_page(page);
if (!pagevec_add(pvec, page) || PageCompound(page))
pagevec_lru_move_fn(pvec, lru_lazyfree_fn, NULL);
- put_cpu_var(lru_lazyfree_pvecs);
+ local_unlock(&lru_pvecs.lock);
}
}
void lru_add_drain(void)
{
- lru_add_drain_cpu(get_cpu());
- put_cpu();
+ local_lock(&lru_pvecs.lock);
+ lru_add_drain_cpu(smp_processor_id());
+ local_unlock(&lru_pvecs.lock);
+}
+
+void lru_add_drain_cpu_zone(struct zone *zone)
+{
+ local_lock(&lru_pvecs.lock);
+ lru_add_drain_cpu(smp_processor_id());
+ drain_local_pages(zone);
+ local_unlock(&lru_pvecs.lock);
}
#ifdef CONFIG_SMP
@@ -743,11 +785,11 @@ void lru_add_drain_all(void)
for_each_online_cpu(cpu) {
struct work_struct *work = &per_cpu(lru_add_drain_work, cpu);
- if (pagevec_count(&per_cpu(lru_add_pvec, cpu)) ||
- pagevec_count(&per_cpu(lru_rotate_pvecs, cpu)) ||
- pagevec_count(&per_cpu(lru_deactivate_file_pvecs, cpu)) ||
- pagevec_count(&per_cpu(lru_deactivate_pvecs, cpu)) ||
- pagevec_count(&per_cpu(lru_lazyfree_pvecs, cpu)) ||
+ if (pagevec_count(&per_cpu(lru_pvecs.lru_add, cpu)) ||
+ pagevec_count(&per_cpu(lru_rotate.pvec, cpu)) ||
+ pagevec_count(&per_cpu(lru_pvecs.lru_deactivate_file, cpu)) ||
+ pagevec_count(&per_cpu(lru_pvecs.lru_deactivate, cpu)) ||
+ pagevec_count(&per_cpu(lru_pvecs.lru_lazyfree, cpu)) ||
need_activate_page_drain(cpu)) {
INIT_WORK(work, lru_add_drain_per_cpu);
queue_work_on(cpu, mm_percpu_wq, work);
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 4/7] squashfs: make use of local lock in multi_cpu decompressor
2020-05-27 20:11 [PATCH v3 0/7] Introduce local_lock() Sebastian Andrzej Siewior
` (2 preceding siblings ...)
2020-05-27 20:11 ` [PATCH v3 3/7] mm/swap: " Sebastian Andrzej Siewior
@ 2020-05-27 20:11 ` Sebastian Andrzej Siewior
2020-06-01 9:52 ` [tip: locking/core] squashfs: Make " tip-bot2 for Julia Cartwright
2020-05-27 20:11 ` [PATCH v3 5/7] connector/cn_proc: Protect send_msg() with a local lock Sebastian Andrzej Siewior
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-27 20:11 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Steven Rostedt, Will Deacon,
Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Julia Cartwright, Phillip Lougher,
Alexander Stein, Sebastian Andrzej Siewior
From: Julia Cartwright <julia@ni.com>
The squashfs multi CPU decompressor makes use of get_cpu_ptr() to
acquire a pointer to per-CPU data. get_cpu_ptr() implicitly disables
preemption which serializes the access to the per-CPU data.
But decompression can take quite some time depending on the size. The
observed preempt disabled times in real world scenarios went up to 8ms,
causing massive wakeup latencies. This happens on all CPUs as the
decompression is fully parallelized.
Replace the implicit preemption control with an explicit local lock.
This allows RT kernels to substitute it with a real per CPU lock, which
serializes the access but keeps the code section preemptible. On non RT
kernels this maps to preempt_disable() as before, i.e. no functional
change.
[ bigeasy: Use local_lock(), patch description]
Cc: Phillip Lougher <phillip@squashfs.org.uk>
Reported-by: Alexander Stein <alexander.stein@systec-electronic.com>
Signed-off-by: Julia Cartwright <julia@ni.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Tested-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
fs/squashfs/decompressor_multi_percpu.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/fs/squashfs/decompressor_multi_percpu.c b/fs/squashfs/decompressor_multi_percpu.c
index 2a2a2d106440e..e206ebfe003ca 100644
--- a/fs/squashfs/decompressor_multi_percpu.c
+++ b/fs/squashfs/decompressor_multi_percpu.c
@@ -8,6 +8,7 @@
#include <linux/slab.h>
#include <linux/percpu.h>
#include <linux/buffer_head.h>
+#include <linux/local_lock.h>
#include "squashfs_fs.h"
#include "squashfs_fs_sb.h"
@@ -20,7 +21,8 @@
*/
struct squashfs_stream {
- void *stream;
+ void *stream;
+ local_lock_t lock;
};
void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
@@ -41,6 +43,7 @@ void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
err = PTR_ERR(stream->stream);
goto out;
}
+ local_lock_init(&stream->lock);
}
kfree(comp_opts);
@@ -75,12 +78,16 @@ void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
int squashfs_decompress(struct squashfs_sb_info *msblk, struct buffer_head **bh,
int b, int offset, int length, struct squashfs_page_actor *output)
{
- struct squashfs_stream __percpu *percpu =
- (struct squashfs_stream __percpu *) msblk->stream;
- struct squashfs_stream *stream = get_cpu_ptr(percpu);
- int res = msblk->decompressor->decompress(msblk, stream->stream, bh, b,
- offset, length, output);
- put_cpu_ptr(stream);
+ struct squashfs_stream *stream;
+ int res;
+
+ local_lock(&msblk->stream->lock);
+ stream = this_cpu_ptr(msblk->stream);
+
+ res = msblk->decompressor->decompress(msblk, stream->stream, bh, b,
+ offset, length, output);
+
+ local_unlock(&msblk->stream->lock);
if (res < 0)
ERROR("%s decompression failed, data probably corrupt\n",
--
2.27.0.rc0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [tip: locking/core] squashfs: Make use of local lock in multi_cpu decompressor
2020-05-27 20:11 ` [PATCH v3 4/7] squashfs: make use of local lock in multi_cpu decompressor Sebastian Andrzej Siewior
@ 2020-06-01 9:52 ` tip-bot2 for Julia Cartwright
0 siblings, 0 replies; 20+ messages in thread
From: tip-bot2 for Julia Cartwright @ 2020-06-01 9:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Alexander Stein, Julia Cartwright, Sebastian Andrzej Siewior,
Ingo Molnar, Peter Zijlstra, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: fd56200a16c72c7c3ec3e54e06160dfaa5b8dee8
Gitweb: https://git.kernel.org/tip/fd56200a16c72c7c3ec3e54e06160dfaa5b8dee8
Author: Julia Cartwright <julia@ni.com>
AuthorDate: Wed, 27 May 2020 22:11:16 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 May 2020 10:31:10 +02:00
squashfs: Make use of local lock in multi_cpu decompressor
The squashfs multi CPU decompressor makes use of get_cpu_ptr() to
acquire a pointer to per-CPU data. get_cpu_ptr() implicitly disables
preemption which serializes the access to the per-CPU data.
But decompression can take quite some time depending on the size. The
observed preempt disabled times in real world scenarios went up to 8ms,
causing massive wakeup latencies. This happens on all CPUs as the
decompression is fully parallelized.
Replace the implicit preemption control with an explicit local lock.
This allows RT kernels to substitute it with a real per CPU lock, which
serializes the access but keeps the code section preemptible. On non RT
kernels this maps to preempt_disable() as before, i.e. no functional
change.
[ bigeasy: Use local_lock(), patch description]
Reported-by: Alexander Stein <alexander.stein@systec-electronic.com>
Signed-off-by: Julia Cartwright <julia@ni.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Alexander Stein <alexander.stein@systec-electronic.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20200527201119.1692513-5-bigeasy@linutronix.de
---
fs/squashfs/decompressor_multi_percpu.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/fs/squashfs/decompressor_multi_percpu.c b/fs/squashfs/decompressor_multi_percpu.c
index 2a2a2d1..e206ebf 100644
--- a/fs/squashfs/decompressor_multi_percpu.c
+++ b/fs/squashfs/decompressor_multi_percpu.c
@@ -8,6 +8,7 @@
#include <linux/slab.h>
#include <linux/percpu.h>
#include <linux/buffer_head.h>
+#include <linux/local_lock.h>
#include "squashfs_fs.h"
#include "squashfs_fs_sb.h"
@@ -20,7 +21,8 @@
*/
struct squashfs_stream {
- void *stream;
+ void *stream;
+ local_lock_t lock;
};
void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
@@ -41,6 +43,7 @@ void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
err = PTR_ERR(stream->stream);
goto out;
}
+ local_lock_init(&stream->lock);
}
kfree(comp_opts);
@@ -75,12 +78,16 @@ void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
int squashfs_decompress(struct squashfs_sb_info *msblk, struct buffer_head **bh,
int b, int offset, int length, struct squashfs_page_actor *output)
{
- struct squashfs_stream __percpu *percpu =
- (struct squashfs_stream __percpu *) msblk->stream;
- struct squashfs_stream *stream = get_cpu_ptr(percpu);
- int res = msblk->decompressor->decompress(msblk, stream->stream, bh, b,
- offset, length, output);
- put_cpu_ptr(stream);
+ struct squashfs_stream *stream;
+ int res;
+
+ local_lock(&msblk->stream->lock);
+ stream = this_cpu_ptr(msblk->stream);
+
+ res = msblk->decompressor->decompress(msblk, stream->stream, bh, b,
+ offset, length, output);
+
+ local_unlock(&msblk->stream->lock);
if (res < 0)
ERROR("%s decompression failed, data probably corrupt\n",
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 5/7] connector/cn_proc: Protect send_msg() with a local lock
2020-05-27 20:11 [PATCH v3 0/7] Introduce local_lock() Sebastian Andrzej Siewior
` (3 preceding siblings ...)
2020-05-27 20:11 ` [PATCH v3 4/7] squashfs: make use of local lock in multi_cpu decompressor Sebastian Andrzej Siewior
@ 2020-05-27 20:11 ` Sebastian Andrzej Siewior
2020-06-01 9:52 ` [tip: locking/core] " tip-bot2 for Mike Galbraith
2020-05-27 20:11 ` [PATCH v3 6/7] zram: Allocate struct zcomp_strm as per-CPU memory Sebastian Andrzej Siewior
2020-05-27 20:11 ` [PATCH v3 7/7] zram: Use local lock to protect per-CPU data Sebastian Andrzej Siewior
6 siblings, 1 reply; 20+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-27 20:11 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Steven Rostedt, Will Deacon,
Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Mike Galbraith, Evgeniy Polyakov, netdev,
Sebastian Andrzej Siewior
From: Mike Galbraith <umgwanakikbuti@gmail.com>
send_msg() disables preemption to avoid out-of-order messages. As the
code inside the preempt disabled section acquires regular spinlocks,
which are converted to 'sleeping' spinlocks on a PREEMPT_RT kernel and
eventually calls into a memory allocator, this conflicts with the RT
semantics.
Convert it to a local_lock which allows RT kernels to substitute them with
a real per CPU lock. On non RT kernels this maps to preempt_disable() as
before. No functional change.
[bigeasy: Patch description]
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/connector/cn_proc.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index d58ce664da843..646ad385e4904 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -18,6 +18,7 @@
#include <linux/pid_namespace.h>
#include <linux/cn_proc.h>
+#include <linux/local_lock.h>
/*
* Size of a cn_msg followed by a proc_event structure. Since the
@@ -38,25 +39,31 @@ static inline struct cn_msg *buffer_to_cn_msg(__u8 *buffer)
static atomic_t proc_event_num_listeners = ATOMIC_INIT(0);
static struct cb_id cn_proc_event_id = { CN_IDX_PROC, CN_VAL_PROC };
-/* proc_event_counts is used as the sequence number of the netlink message */
-static DEFINE_PER_CPU(__u32, proc_event_counts) = { 0 };
+/* local_event.count is used as the sequence number of the netlink message */
+struct local_event {
+ local_lock_t lock;
+ __u32 count;
+};
+static DEFINE_PER_CPU(struct local_event, local_event) = {
+ .lock = INIT_LOCAL_LOCK(lock),
+};
static inline void send_msg(struct cn_msg *msg)
{
- preempt_disable();
+ local_lock(&local_event.lock);
- msg->seq = __this_cpu_inc_return(proc_event_counts) - 1;
+ msg->seq = __this_cpu_inc_return(local_event.count) - 1;
((struct proc_event *)msg->data)->cpu = smp_processor_id();
/*
- * Preemption remains disabled during send to ensure the messages are
- * ordered according to their sequence numbers.
+ * local_lock() disables preemption during send to ensure the messages
+ * are ordered according to their sequence numbers.
*
* If cn_netlink_send() fails, the data is not sent.
*/
cn_netlink_send(msg, 0, CN_IDX_PROC, GFP_NOWAIT);
- preempt_enable();
+ local_unlock(&local_event.lock);
}
void proc_fork_connector(struct task_struct *task)
--
2.27.0.rc0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [tip: locking/core] connector/cn_proc: Protect send_msg() with a local lock
2020-05-27 20:11 ` [PATCH v3 5/7] connector/cn_proc: Protect send_msg() with a local lock Sebastian Andrzej Siewior
@ 2020-06-01 9:52 ` tip-bot2 for Mike Galbraith
0 siblings, 0 replies; 20+ messages in thread
From: tip-bot2 for Mike Galbraith @ 2020-06-01 9:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Mike Galbraith, Sebastian Andrzej Siewior, Ingo Molnar,
Peter Zijlstra, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 3e92fd7bd2b8418b53cb7304855b8b69bedbe2b4
Gitweb: https://git.kernel.org/tip/3e92fd7bd2b8418b53cb7304855b8b69bedbe2b4
Author: Mike Galbraith <umgwanakikbuti@gmail.com>
AuthorDate: Wed, 27 May 2020 22:11:17 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 May 2020 10:31:10 +02:00
connector/cn_proc: Protect send_msg() with a local lock
send_msg() disables preemption to avoid out-of-order messages. As the
code inside the preempt disabled section acquires regular spinlocks,
which are converted to 'sleeping' spinlocks on a PREEMPT_RT kernel and
eventually calls into a memory allocator, this conflicts with the RT
semantics.
Convert it to a local_lock which allows RT kernels to substitute them with
a real per CPU lock. On non RT kernels this maps to preempt_disable() as
before. No functional change.
[bigeasy: Patch description]
Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20200527201119.1692513-6-bigeasy@linutronix.de
---
drivers/connector/cn_proc.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index d58ce66..646ad38 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -18,6 +18,7 @@
#include <linux/pid_namespace.h>
#include <linux/cn_proc.h>
+#include <linux/local_lock.h>
/*
* Size of a cn_msg followed by a proc_event structure. Since the
@@ -38,25 +39,31 @@ static inline struct cn_msg *buffer_to_cn_msg(__u8 *buffer)
static atomic_t proc_event_num_listeners = ATOMIC_INIT(0);
static struct cb_id cn_proc_event_id = { CN_IDX_PROC, CN_VAL_PROC };
-/* proc_event_counts is used as the sequence number of the netlink message */
-static DEFINE_PER_CPU(__u32, proc_event_counts) = { 0 };
+/* local_event.count is used as the sequence number of the netlink message */
+struct local_event {
+ local_lock_t lock;
+ __u32 count;
+};
+static DEFINE_PER_CPU(struct local_event, local_event) = {
+ .lock = INIT_LOCAL_LOCK(lock),
+};
static inline void send_msg(struct cn_msg *msg)
{
- preempt_disable();
+ local_lock(&local_event.lock);
- msg->seq = __this_cpu_inc_return(proc_event_counts) - 1;
+ msg->seq = __this_cpu_inc_return(local_event.count) - 1;
((struct proc_event *)msg->data)->cpu = smp_processor_id();
/*
- * Preemption remains disabled during send to ensure the messages are
- * ordered according to their sequence numbers.
+ * local_lock() disables preemption during send to ensure the messages
+ * are ordered according to their sequence numbers.
*
* If cn_netlink_send() fails, the data is not sent.
*/
cn_netlink_send(msg, 0, CN_IDX_PROC, GFP_NOWAIT);
- preempt_enable();
+ local_unlock(&local_event.lock);
}
void proc_fork_connector(struct task_struct *task)
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 6/7] zram: Allocate struct zcomp_strm as per-CPU memory
2020-05-27 20:11 [PATCH v3 0/7] Introduce local_lock() Sebastian Andrzej Siewior
` (4 preceding siblings ...)
2020-05-27 20:11 ` [PATCH v3 5/7] connector/cn_proc: Protect send_msg() with a local lock Sebastian Andrzej Siewior
@ 2020-05-27 20:11 ` Sebastian Andrzej Siewior
2020-05-29 20:51 ` Minchan Kim
2020-06-01 9:52 ` [tip: locking/core] " tip-bot2 for Sebastian Andrzej Siewior
2020-05-27 20:11 ` [PATCH v3 7/7] zram: Use local lock to protect per-CPU data Sebastian Andrzej Siewior
6 siblings, 2 replies; 20+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-27 20:11 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Steven Rostedt, Will Deacon,
Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Sebastian Andrzej Siewior, Minchan Kim,
Nitin Gupta, Sergey Senozhatsky
zcomp::stream is a per-CPU pointer, pointing to struct zcomp_strm
which contains two pointers. Having struct zcomp_strm allocated
directly as per-CPU memory would avoid one additional memory
allocation and a pointer dereference. This also simplifies the
addition of a local_lock to struct zcomp_strm.
Allocate zcomp::stream directly as per-CPU memory.
Cc: Minchan Kim <minchan@kernel.org>
Cc: Nitin Gupta <ngupta@vflare.org>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/block/zram/zcomp.c | 41 +++++++++++++++-----------------------
drivers/block/zram/zcomp.h | 2 +-
2 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 1a8564a79d8dc..912e3e63d8a09 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -37,19 +37,16 @@ static void zcomp_strm_free(struct zcomp_strm *zstrm)
if (!IS_ERR_OR_NULL(zstrm->tfm))
crypto_free_comp(zstrm->tfm);
free_pages((unsigned long)zstrm->buffer, 1);
- kfree(zstrm);
+ zstrm->tfm = NULL;
+ zstrm->buffer = NULL;
}
/*
- * allocate new zcomp_strm structure with ->tfm initialized by
- * backend, return NULL on error
+ * Initialize zcomp_strm structure with ->tfm initialized by backend, and
+ * ->buffer. Return a negative value on error.
*/
-static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
+static int zcomp_strm_init(struct zcomp_strm *zstrm, struct zcomp *comp)
{
- struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_KERNEL);
- if (!zstrm)
- return NULL;
-
zstrm->tfm = crypto_alloc_comp(comp->name, 0, 0);
/*
* allocate 2 pages. 1 for compressed data, plus 1 extra for the
@@ -58,9 +55,9 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
zstrm->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
if (IS_ERR_OR_NULL(zstrm->tfm) || !zstrm->buffer) {
zcomp_strm_free(zstrm);
- zstrm = NULL;
+ return -ENOMEM;
}
- return zstrm;
+ return 0;
}
bool zcomp_available_algorithm(const char *comp)
@@ -113,7 +110,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
{
- return *get_cpu_ptr(comp->stream);
+ return get_cpu_ptr(comp->stream);
}
void zcomp_stream_put(struct zcomp *comp)
@@ -159,17 +156,13 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node)
{
struct zcomp *comp = hlist_entry(node, struct zcomp, node);
struct zcomp_strm *zstrm;
+ int ret;
- if (WARN_ON(*per_cpu_ptr(comp->stream, cpu)))
- return 0;
-
- zstrm = zcomp_strm_alloc(comp);
- if (IS_ERR_OR_NULL(zstrm)) {
+ zstrm = per_cpu_ptr(comp->stream, cpu);
+ ret = zcomp_strm_init(zstrm, comp);
+ if (ret)
pr_err("Can't allocate a compression stream\n");
- return -ENOMEM;
- }
- *per_cpu_ptr(comp->stream, cpu) = zstrm;
- return 0;
+ return ret;
}
int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node)
@@ -177,10 +170,8 @@ int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node)
struct zcomp *comp = hlist_entry(node, struct zcomp, node);
struct zcomp_strm *zstrm;
- zstrm = *per_cpu_ptr(comp->stream, cpu);
- if (!IS_ERR_OR_NULL(zstrm))
- zcomp_strm_free(zstrm);
- *per_cpu_ptr(comp->stream, cpu) = NULL;
+ zstrm = per_cpu_ptr(comp->stream, cpu);
+ zcomp_strm_free(zstrm);
return 0;
}
@@ -188,7 +179,7 @@ static int zcomp_init(struct zcomp *comp)
{
int ret;
- comp->stream = alloc_percpu(struct zcomp_strm *);
+ comp->stream = alloc_percpu(struct zcomp_strm);
if (!comp->stream)
return -ENOMEM;
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 1806475b919df..72c2ee4d843ed 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -14,7 +14,7 @@ struct zcomp_strm {
/* dynamic per-device compression frontend */
struct zcomp {
- struct zcomp_strm * __percpu *stream;
+ struct zcomp_strm __percpu *stream;
const char *name;
struct hlist_node node;
};
--
2.27.0.rc0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 6/7] zram: Allocate struct zcomp_strm as per-CPU memory
2020-05-27 20:11 ` [PATCH v3 6/7] zram: Allocate struct zcomp_strm as per-CPU memory Sebastian Andrzej Siewior
@ 2020-05-29 20:51 ` Minchan Kim
2020-06-01 9:52 ` [tip: locking/core] " tip-bot2 for Sebastian Andrzej Siewior
1 sibling, 0 replies; 20+ messages in thread
From: Minchan Kim @ 2020-05-29 20:51 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Steven Rostedt,
Will Deacon, Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Nitin Gupta, Sergey Senozhatsky
On Wed, May 27, 2020 at 10:11:18PM +0200, Sebastian Andrzej Siewior wrote:
> zcomp::stream is a per-CPU pointer, pointing to struct zcomp_strm
> which contains two pointers. Having struct zcomp_strm allocated
> directly as per-CPU memory would avoid one additional memory
> allocation and a pointer dereference. This also simplifies the
> addition of a local_lock to struct zcomp_strm.
>
> Allocate zcomp::stream directly as per-CPU memory.
>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Nitin Gupta <ngupta@vflare.org>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Minchan Kim <minchan@kernel.org>
Looks good to me.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tip: locking/core] zram: Allocate struct zcomp_strm as per-CPU memory
2020-05-27 20:11 ` [PATCH v3 6/7] zram: Allocate struct zcomp_strm as per-CPU memory Sebastian Andrzej Siewior
2020-05-29 20:51 ` Minchan Kim
@ 2020-06-01 9:52 ` tip-bot2 for Sebastian Andrzej Siewior
1 sibling, 0 replies; 20+ messages in thread
From: tip-bot2 for Sebastian Andrzej Siewior @ 2020-06-01 9:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Sebastian Andrzej Siewior, Ingo Molnar, Peter Zijlstra, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: ed19f19256be2949af1ab5634e62178d30a355c2
Gitweb: https://git.kernel.org/tip/ed19f19256be2949af1ab5634e62178d30a355c2
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Wed, 27 May 2020 22:11:18 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 May 2020 10:31:10 +02:00
zram: Allocate struct zcomp_strm as per-CPU memory
zcomp::stream is a per-CPU pointer, pointing to struct zcomp_strm
which contains two pointers. Having struct zcomp_strm allocated
directly as per-CPU memory would avoid one additional memory
allocation and a pointer dereference. This also simplifies the
addition of a local_lock to struct zcomp_strm.
Allocate zcomp::stream directly as per-CPU memory.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20200527201119.1692513-7-bigeasy@linutronix.de
---
drivers/block/zram/zcomp.c | 41 ++++++++++++++-----------------------
drivers/block/zram/zcomp.h | 2 +-
2 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 1a8564a..912e3e6 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -37,19 +37,16 @@ static void zcomp_strm_free(struct zcomp_strm *zstrm)
if (!IS_ERR_OR_NULL(zstrm->tfm))
crypto_free_comp(zstrm->tfm);
free_pages((unsigned long)zstrm->buffer, 1);
- kfree(zstrm);
+ zstrm->tfm = NULL;
+ zstrm->buffer = NULL;
}
/*
- * allocate new zcomp_strm structure with ->tfm initialized by
- * backend, return NULL on error
+ * Initialize zcomp_strm structure with ->tfm initialized by backend, and
+ * ->buffer. Return a negative value on error.
*/
-static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
+static int zcomp_strm_init(struct zcomp_strm *zstrm, struct zcomp *comp)
{
- struct zcomp_strm *zstrm = kmalloc(sizeof(*zstrm), GFP_KERNEL);
- if (!zstrm)
- return NULL;
-
zstrm->tfm = crypto_alloc_comp(comp->name, 0, 0);
/*
* allocate 2 pages. 1 for compressed data, plus 1 extra for the
@@ -58,9 +55,9 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
zstrm->buffer = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
if (IS_ERR_OR_NULL(zstrm->tfm) || !zstrm->buffer) {
zcomp_strm_free(zstrm);
- zstrm = NULL;
+ return -ENOMEM;
}
- return zstrm;
+ return 0;
}
bool zcomp_available_algorithm(const char *comp)
@@ -113,7 +110,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
{
- return *get_cpu_ptr(comp->stream);
+ return get_cpu_ptr(comp->stream);
}
void zcomp_stream_put(struct zcomp *comp)
@@ -159,17 +156,13 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node)
{
struct zcomp *comp = hlist_entry(node, struct zcomp, node);
struct zcomp_strm *zstrm;
+ int ret;
- if (WARN_ON(*per_cpu_ptr(comp->stream, cpu)))
- return 0;
-
- zstrm = zcomp_strm_alloc(comp);
- if (IS_ERR_OR_NULL(zstrm)) {
+ zstrm = per_cpu_ptr(comp->stream, cpu);
+ ret = zcomp_strm_init(zstrm, comp);
+ if (ret)
pr_err("Can't allocate a compression stream\n");
- return -ENOMEM;
- }
- *per_cpu_ptr(comp->stream, cpu) = zstrm;
- return 0;
+ return ret;
}
int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node)
@@ -177,10 +170,8 @@ int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node)
struct zcomp *comp = hlist_entry(node, struct zcomp, node);
struct zcomp_strm *zstrm;
- zstrm = *per_cpu_ptr(comp->stream, cpu);
- if (!IS_ERR_OR_NULL(zstrm))
- zcomp_strm_free(zstrm);
- *per_cpu_ptr(comp->stream, cpu) = NULL;
+ zstrm = per_cpu_ptr(comp->stream, cpu);
+ zcomp_strm_free(zstrm);
return 0;
}
@@ -188,7 +179,7 @@ static int zcomp_init(struct zcomp *comp)
{
int ret;
- comp->stream = alloc_percpu(struct zcomp_strm *);
+ comp->stream = alloc_percpu(struct zcomp_strm);
if (!comp->stream)
return -ENOMEM;
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 1806475..72c2ee4 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -14,7 +14,7 @@ struct zcomp_strm {
/* dynamic per-device compression frontend */
struct zcomp {
- struct zcomp_strm * __percpu *stream;
+ struct zcomp_strm __percpu *stream;
const char *name;
struct hlist_node node;
};
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
2020-05-27 20:11 [PATCH v3 0/7] Introduce local_lock() Sebastian Andrzej Siewior
` (5 preceding siblings ...)
2020-05-27 20:11 ` [PATCH v3 6/7] zram: Allocate struct zcomp_strm as per-CPU memory Sebastian Andrzej Siewior
@ 2020-05-27 20:11 ` Sebastian Andrzej Siewior
2020-05-29 20:57 ` Minchan Kim
` (2 more replies)
6 siblings, 3 replies; 20+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-05-27 20:11 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Steven Rostedt, Will Deacon,
Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Mike Galbraith, Minchan Kim, Nitin Gupta,
Sergey Senozhatsky, Sebastian Andrzej Siewior
From: Mike Galbraith <umgwanakikbuti@gmail.com>
The zcomp driver uses per-CPU compression. The per-CPU data pointer is
acquired with get_cpu_ptr() which implicitly disables preemption.
It allocates memory inside the preempt disabled region which conflicts
with the PREEMPT_RT semantics.
Replace the implicit preemption control with an explicit local lock.
This allows RT kernels to substitute it with a real per CPU lock, which
serializes the access but keeps the code section preemptible. On non RT
kernels this maps to preempt_disable() as before, i.e. no functional
change.
[bigeasy: Use local_lock(), description, drop reordering]
Cc: Minchan Kim <minchan@kernel.org>
Cc: Nitin Gupta <ngupta@vflare.org>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/block/zram/zcomp.c | 7 +++++--
drivers/block/zram/zcomp.h | 3 +++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 912e3e63d8a09..5ee8e3fae5516 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -110,12 +110,13 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
{
- return get_cpu_ptr(comp->stream);
+ local_lock(&comp->stream->lock);
+ return this_cpu_ptr(comp->stream);
}
void zcomp_stream_put(struct zcomp *comp)
{
- put_cpu_ptr(comp->stream);
+ local_unlock(&comp->stream->lock);
}
int zcomp_compress(struct zcomp_strm *zstrm,
@@ -159,6 +160,8 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node)
int ret;
zstrm = per_cpu_ptr(comp->stream, cpu);
+ local_lock_init(&zstrm->lock);
+
ret = zcomp_strm_init(zstrm, comp);
if (ret)
pr_err("Can't allocate a compression stream\n");
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 72c2ee4d843ed..40f6420f4b2e9 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -5,8 +5,11 @@
#ifndef _ZCOMP_H_
#define _ZCOMP_H_
+#include <linux/local_lock.h>
struct zcomp_strm {
+ /* The members ->buffer and ->tfm are protected by ->lock. */
+ local_lock_t lock;
/* compression/decompression buffer */
void *buffer;
struct crypto_comp *tfm;
--
2.27.0.rc0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
2020-05-27 20:11 ` [PATCH v3 7/7] zram: Use local lock to protect per-CPU data Sebastian Andrzej Siewior
@ 2020-05-29 20:57 ` Minchan Kim
2020-06-01 9:52 ` [tip: locking/core] " tip-bot2 for Mike Galbraith
2020-10-19 1:52 ` [PATCH v3 7/7] " Yu Zhao
2 siblings, 0 replies; 20+ messages in thread
From: Minchan Kim @ 2020-05-29 20:57 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Steven Rostedt,
Will Deacon, Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Mike Galbraith, Nitin Gupta, Sergey Senozhatsky
On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote:
> From: Mike Galbraith <umgwanakikbuti@gmail.com>
>
> The zcomp driver uses per-CPU compression. The per-CPU data pointer is
> acquired with get_cpu_ptr() which implicitly disables preemption.
> It allocates memory inside the preempt disabled region which conflicts
> with the PREEMPT_RT semantics.
>
> Replace the implicit preemption control with an explicit local lock.
> This allows RT kernels to substitute it with a real per CPU lock, which
> serializes the access but keeps the code section preemptible. On non RT
> kernels this maps to preempt_disable() as before, i.e. no functional
> change.
>
> [bigeasy: Use local_lock(), description, drop reordering]
>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Nitin Gupta <ngupta@vflare.org>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Minchan Kim <minchan@kernel.org>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tip: locking/core] zram: Use local lock to protect per-CPU data
2020-05-27 20:11 ` [PATCH v3 7/7] zram: Use local lock to protect per-CPU data Sebastian Andrzej Siewior
2020-05-29 20:57 ` Minchan Kim
@ 2020-06-01 9:52 ` tip-bot2 for Mike Galbraith
2020-10-19 1:52 ` [PATCH v3 7/7] " Yu Zhao
2 siblings, 0 replies; 20+ messages in thread
From: tip-bot2 for Mike Galbraith @ 2020-06-01 9:52 UTC (permalink / raw)
To: linux-tip-commits
Cc: Mike Galbraith, Sebastian Andrzej Siewior, Ingo Molnar,
Peter Zijlstra, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 19f545b6e07f753c4dc639c2f0ab52345733b6a8
Gitweb: https://git.kernel.org/tip/19f545b6e07f753c4dc639c2f0ab52345733b6a8
Author: Mike Galbraith <umgwanakikbuti@gmail.com>
AuthorDate: Wed, 27 May 2020 22:11:19 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 May 2020 10:31:10 +02:00
zram: Use local lock to protect per-CPU data
The zcomp driver uses per-CPU compression. The per-CPU data pointer is
acquired with get_cpu_ptr() which implicitly disables preemption.
It allocates memory inside the preempt disabled region which conflicts
with the PREEMPT_RT semantics.
Replace the implicit preemption control with an explicit local lock.
This allows RT kernels to substitute it with a real per CPU lock, which
serializes the access but keeps the code section preemptible. On non RT
kernels this maps to preempt_disable() as before, i.e. no functional
change.
[bigeasy: Use local_lock(), description, drop reordering]
Signed-off-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20200527201119.1692513-8-bigeasy@linutronix.de
---
drivers/block/zram/zcomp.c | 7 +++++--
drivers/block/zram/zcomp.h | 3 +++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 912e3e6..5ee8e3f 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -110,12 +110,13 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
{
- return get_cpu_ptr(comp->stream);
+ local_lock(&comp->stream->lock);
+ return this_cpu_ptr(comp->stream);
}
void zcomp_stream_put(struct zcomp *comp)
{
- put_cpu_ptr(comp->stream);
+ local_unlock(&comp->stream->lock);
}
int zcomp_compress(struct zcomp_strm *zstrm,
@@ -159,6 +160,8 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node)
int ret;
zstrm = per_cpu_ptr(comp->stream, cpu);
+ local_lock_init(&zstrm->lock);
+
ret = zcomp_strm_init(zstrm, comp);
if (ret)
pr_err("Can't allocate a compression stream\n");
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index 72c2ee4..40f6420 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -5,8 +5,11 @@
#ifndef _ZCOMP_H_
#define _ZCOMP_H_
+#include <linux/local_lock.h>
struct zcomp_strm {
+ /* The members ->buffer and ->tfm are protected by ->lock. */
+ local_lock_t lock;
/* compression/decompression buffer */
void *buffer;
struct crypto_comp *tfm;
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
2020-05-27 20:11 ` [PATCH v3 7/7] zram: Use local lock to protect per-CPU data Sebastian Andrzej Siewior
2020-05-29 20:57 ` Minchan Kim
2020-06-01 9:52 ` [tip: locking/core] " tip-bot2 for Mike Galbraith
@ 2020-10-19 1:52 ` Yu Zhao
2020-10-19 2:33 ` Hugh Dickins
2020-10-19 2:46 ` Mike Galbraith
2 siblings, 2 replies; 20+ messages in thread
From: Yu Zhao @ 2020-10-19 1:52 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Steven Rostedt,
Will Deacon, Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Mike Galbraith, Minchan Kim, Nitin Gupta,
Sergey Senozhatsky, linux-mm
On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote:
> From: Mike Galbraith <umgwanakikbuti@gmail.com>
>
> The zcomp driver uses per-CPU compression. The per-CPU data pointer is
> acquired with get_cpu_ptr() which implicitly disables preemption.
> It allocates memory inside the preempt disabled region which conflicts
> with the PREEMPT_RT semantics.
>
> Replace the implicit preemption control with an explicit local lock.
> This allows RT kernels to substitute it with a real per CPU lock, which
> serializes the access but keeps the code section preemptible. On non RT
> kernels this maps to preempt_disable() as before, i.e. no functional
> change.
Hi,
This change seems to have introduced a potential deadlock. Can you
please take a look?
Thank you.
[ 40.030778] ======================================================
[ 40.037706] WARNING: possible circular locking dependency detected
[ 40.044637] 5.9.0-74216-g5c9472ed6825 #1 Tainted: G W
[ 40.051759] ------------------------------------------------------
[ 40.058685] swapon/586 is trying to acquire lock:
[ 40.063950] ffffe8ffffc0ee60 (&zstrm->lock){+.+.}-{2:2}, at: local_lock_acquire+0x5/0x70 [zram]
[ 40.073739]
[ 40.073739] but task is already holding lock:
[ 40.080277] ffff888101a1f438 (&zspage->lock){.+.+}-{2:2}, at: zs_map_object+0x73/0x28d
[ 40.089182]
[ 40.089182] which lock already depends on the new lock.
[ 40.089182]
[ 40.098344]
[ 40.098344] the existing dependency chain (in reverse order) is:
[ 40.106715]
[ 40.106715] -> #1 (&zspage->lock){.+.+}-{2:2}:
[ 40.113386] lock_acquire+0x1cd/0x2c3
[ 40.118083] _raw_read_lock+0x44/0x78
[ 40.122781] zs_map_object+0x73/0x28d
[ 40.127479] zram_bvec_rw+0x42e/0x75d [zram]
[ 40.132855] zram_submit_bio+0x1fc/0x2d7 [zram]
[ 40.138526] submit_bio_noacct+0x11b/0x372
[ 40.143709] submit_bio+0xfd/0x1b5
[ 40.148113] __block_write_full_page+0x302/0x56f
[ 40.153877] __writepage+0x1e/0x74
[ 40.158281] write_cache_pages+0x404/0x59a
[ 40.163461] generic_writepages+0x53/0x82
[ 40.168545] do_writepages+0x33/0x74
[ 40.173145] __filemap_fdatawrite_range+0x91/0xac
[ 40.179005] file_write_and_wait_range+0x39/0x87
[ 40.184769] blkdev_fsync+0x19/0x3e
[ 40.189272] do_fsync+0x39/0x5c
[ 40.193384] __x64_sys_fsync+0x13/0x17
[ 40.198178] do_syscall_64+0x37/0x45
[ 40.202776] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 40.209022]
[ 40.209022] -> #0 (&zstrm->lock){+.+.}-{2:2}:
[ 40.215589] validate_chain+0x1966/0x21a8
[ 40.220673] __lock_acquire+0x941/0xbba
[ 40.225552] lock_acquire+0x1cd/0x2c3
[ 40.230250] local_lock_acquire+0x21/0x70 [zram]
[ 40.236015] zcomp_stream_get+0x33/0x4d [zram]
[ 40.241585] zram_bvec_rw+0x476/0x75d [zram]
[ 40.246963] zram_rw_page+0xd8/0x17c [zram]
[ 40.252240] bdev_read_page+0x7a/0x9d
[ 40.256933] do_mpage_readpage+0x6b2/0x860
[ 40.262101] mpage_readahead+0x136/0x245
[ 40.267089] read_pages+0x60/0x1f9
[ 40.271492] page_cache_ra_unbounded+0x211/0x27b
[ 40.277251] generic_file_buffered_read+0x188/0xd4d
[ 40.283296] new_sync_read+0x10c/0x143
[ 40.288088] vfs_read+0xf4/0x1a5
[ 40.292285] ksys_read+0x73/0xd3
[ 40.296483] do_syscall_64+0x37/0x45
[ 40.301072] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 40.307319]
[ 40.307319] other info that might help us debug this:
[ 40.307319]
[ 40.316285] Possible unsafe locking scenario:
[ 40.316285]
[ 40.322907] CPU0 CPU1
[ 40.327972] ---- ----
[ 40.333041] lock(&zspage->lock);
[ 40.336874] lock(&zstrm->lock);
[ 40.343424] lock(&zspage->lock);
[ 40.350071] lock(&zstrm->lock);
[ 40.353803]
[ 40.353803] *** DEADLOCK ***
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
2020-10-19 1:52 ` [PATCH v3 7/7] " Yu Zhao
@ 2020-10-19 2:33 ` Hugh Dickins
2020-10-19 2:46 ` Mike Galbraith
1 sibling, 0 replies; 20+ messages in thread
From: Hugh Dickins @ 2020-10-19 2:33 UTC (permalink / raw)
To: Yu Zhao
Cc: Sebastian Andrzej Siewior, linux-kernel, Peter Zijlstra,
Ingo Molnar, Steven Rostedt, Will Deacon, Thomas Gleixner,
Paul E . McKenney, Linus Torvalds, Matthew Wilcox,
Mike Galbraith, Minchan Kim, Nitin Gupta, Sergey Senozhatsky,
linux-mm
On Sun, Oct 18, 2020 at 6:53 PM Yu Zhao <yuzhao@google.com> wrote:
>
> On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote:
> > From: Mike Galbraith <umgwanakikbuti@gmail.com>
> >
> > The zcomp driver uses per-CPU compression. The per-CPU data pointer is
> > acquired with get_cpu_ptr() which implicitly disables preemption.
> > It allocates memory inside the preempt disabled region which conflicts
> > with the PREEMPT_RT semantics.
> >
> > Replace the implicit preemption control with an explicit local lock.
> > This allows RT kernels to substitute it with a real per CPU lock, which
> > serializes the access but keeps the code section preemptible. On non RT
> > kernels this maps to preempt_disable() as before, i.e. no functional
> > change.
>
> Hi,
>
> This change seems to have introduced a potential deadlock. Can you
> please take a look?
Probably needs Peter's fix
https://lore.kernel.org/lkml/20201016124009.GQ2611@hirez.programming.kicks-ass.net/
>
> Thank you.
>
> [ 40.030778] ======================================================
> [ 40.037706] WARNING: possible circular locking dependency detected
> [ 40.044637] 5.9.0-74216-g5c9472ed6825 #1 Tainted: G W
> [ 40.051759] ------------------------------------------------------
> [ 40.058685] swapon/586 is trying to acquire lock:
> [ 40.063950] ffffe8ffffc0ee60 (&zstrm->lock){+.+.}-{2:2}, at: local_lock_acquire+0x5/0x70 [zram]
> [ 40.073739]
> [ 40.073739] but task is already holding lock:
> [ 40.080277] ffff888101a1f438 (&zspage->lock){.+.+}-{2:2}, at: zs_map_object+0x73/0x28d
> [ 40.089182]
> [ 40.089182] which lock already depends on the new lock.
> [ 40.089182]
> [ 40.098344]
> [ 40.098344] the existing dependency chain (in reverse order) is:
> [ 40.106715]
> [ 40.106715] -> #1 (&zspage->lock){.+.+}-{2:2}:
> [ 40.113386] lock_acquire+0x1cd/0x2c3
> [ 40.118083] _raw_read_lock+0x44/0x78
> [ 40.122781] zs_map_object+0x73/0x28d
> [ 40.127479] zram_bvec_rw+0x42e/0x75d [zram]
> [ 40.132855] zram_submit_bio+0x1fc/0x2d7 [zram]
> [ 40.138526] submit_bio_noacct+0x11b/0x372
> [ 40.143709] submit_bio+0xfd/0x1b5
> [ 40.148113] __block_write_full_page+0x302/0x56f
> [ 40.153877] __writepage+0x1e/0x74
> [ 40.158281] write_cache_pages+0x404/0x59a
> [ 40.163461] generic_writepages+0x53/0x82
> [ 40.168545] do_writepages+0x33/0x74
> [ 40.173145] __filemap_fdatawrite_range+0x91/0xac
> [ 40.179005] file_write_and_wait_range+0x39/0x87
> [ 40.184769] blkdev_fsync+0x19/0x3e
> [ 40.189272] do_fsync+0x39/0x5c
> [ 40.193384] __x64_sys_fsync+0x13/0x17
> [ 40.198178] do_syscall_64+0x37/0x45
> [ 40.202776] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 40.209022]
> [ 40.209022] -> #0 (&zstrm->lock){+.+.}-{2:2}:
> [ 40.215589] validate_chain+0x1966/0x21a8
> [ 40.220673] __lock_acquire+0x941/0xbba
> [ 40.225552] lock_acquire+0x1cd/0x2c3
> [ 40.230250] local_lock_acquire+0x21/0x70 [zram]
> [ 40.236015] zcomp_stream_get+0x33/0x4d [zram]
> [ 40.241585] zram_bvec_rw+0x476/0x75d [zram]
> [ 40.246963] zram_rw_page+0xd8/0x17c [zram]
> [ 40.252240] bdev_read_page+0x7a/0x9d
> [ 40.256933] do_mpage_readpage+0x6b2/0x860
> [ 40.262101] mpage_readahead+0x136/0x245
> [ 40.267089] read_pages+0x60/0x1f9
> [ 40.271492] page_cache_ra_unbounded+0x211/0x27b
> [ 40.277251] generic_file_buffered_read+0x188/0xd4d
> [ 40.283296] new_sync_read+0x10c/0x143
> [ 40.288088] vfs_read+0xf4/0x1a5
> [ 40.292285] ksys_read+0x73/0xd3
> [ 40.296483] do_syscall_64+0x37/0x45
> [ 40.301072] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 40.307319]
> [ 40.307319] other info that might help us debug this:
> [ 40.307319]
> [ 40.316285] Possible unsafe locking scenario:
> [ 40.316285]
> [ 40.322907] CPU0 CPU1
> [ 40.327972] ---- ----
> [ 40.333041] lock(&zspage->lock);
> [ 40.336874] lock(&zstrm->lock);
> [ 40.343424] lock(&zspage->lock);
> [ 40.350071] lock(&zstrm->lock);
> [ 40.353803]
> [ 40.353803] *** DEADLOCK ***
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 7/7] zram: Use local lock to protect per-CPU data
2020-10-19 1:52 ` [PATCH v3 7/7] " Yu Zhao
2020-10-19 2:33 ` Hugh Dickins
@ 2020-10-19 2:46 ` Mike Galbraith
1 sibling, 0 replies; 20+ messages in thread
From: Mike Galbraith @ 2020-10-19 2:46 UTC (permalink / raw)
To: Yu Zhao, Sebastian Andrzej Siewior
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Steven Rostedt,
Will Deacon, Thomas Gleixner, Paul E . McKenney, Linus Torvalds,
Matthew Wilcox, Minchan Kim, Nitin Gupta, Sergey Senozhatsky,
linux-mm
On Sun, 2020-10-18 at 19:52 -0600, Yu Zhao wrote:
> On Wed, May 27, 2020 at 10:11:19PM +0200, Sebastian Andrzej Siewior wrote:
> > From: Mike Galbraith <umgwanakikbuti@gmail.com>
> >
> > The zcomp driver uses per-CPU compression. The per-CPU data pointer is
> > acquired with get_cpu_ptr() which implicitly disables preemption.
> > It allocates memory inside the preempt disabled region which conflicts
> > with the PREEMPT_RT semantics.
> >
> > Replace the implicit preemption control with an explicit local lock.
> > This allows RT kernels to substitute it with a real per CPU lock, which
> > serializes the access but keeps the code section preemptible. On non RT
> > kernels this maps to preempt_disable() as before, i.e. no functional
> > change.
>
> Hi,
>
> This change seems to have introduced a potential deadlock. Can you
> please take a look?
Hm, looks like I'm getting undeserved credit for uncovering a locking
bug. In reality, Sebastian was generous with attribution of derivative
work, so he should ge credit.. and it looks like peterz fixed it.
Date: Fri, 16 Oct 2020 14:40:09 +0200
From: Peter Zijlstra <peterz@infradead.org>
---
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9100ac36670a..c1e2c2e1cde8 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1216,10 +1216,11 @@ static void zram_free_page(struct zram *zram, size_t index)
static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
struct bio *bio, bool partial_io)
{
- int ret;
+ struct zcomp_strm *zstrm;
unsigned long handle;
unsigned int size;
void *src, *dst;
+ int ret;
zram_slot_lock(zram, index);
if (zram_test_flag(zram, index, ZRAM_WB)) {
@@ -1250,6 +1251,9 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
size = zram_get_obj_size(zram, index);
+ if (size != PAGE_SIZE)
+ zstrm = zcomp_stream_get(zram->comp);
+
src = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
if (size == PAGE_SIZE) {
dst = kmap_atomic(page);
@@ -1257,8 +1261,6 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
kunmap_atomic(dst);
ret = 0;
} else {
- struct zcomp_strm *zstrm = zcomp_stream_get(zram->comp);
-
dst = kmap_atomic(page);
ret = zcomp_decompress(zstrm, src, size, dst);
kunmap_atomic(dst);
^ permalink raw reply related [flat|nested] 20+ messages in thread