* [PATCH v7 08/16] lockdep: Avoid adding redundant direct links of crosslocks
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
In-Reply-To: <1495616389-29772-1-git-send-email-byungchul.park@lge.com>
We can skip adding a dependency 'AX -> B', in case that we ensure 'AX ->
the previous of B in hlocks' to be created, where AX is a crosslock and
B is a typical lock. Remember that two adjacent locks in hlocks generate
a dependency like 'prev -> next', that is, 'the previous of B in hlocks
-> B' in this case.
For example:
in hlocks[]
------------
^ A (gen_id: 4) --+
| | previous gen_id
| B (gen_id: 3) <-+
| C (gen_id: 3)
| D (gen_id: 2)
oldest | E (gen_id: 1)
in xhlocks[]
------------
^ A (gen_id: 4, prev_gen_id: 3(B's gen id))
| B (gen_id: 3, prev_gen_id: 3(C's gen id))
| C (gen_id: 3, prev_gen_id: 2(D's gen id))
| D (gen_id: 2, prev_gen_id: 1(E's gen id))
oldest | E (gen_id: 1, prev_gen_id: NA)
On commit for a crosslock AX(gen_id = 3), it's engough to add 'AX -> C',
but adding 'AX -> B' and 'AX -> A' is unnecessary since 'AX -> C', 'C ->
B' and 'B -> A' cover them, which are guaranteed to be generated.
This patch intoduces a variable, prev_gen_id, to avoid adding this kind
of redundant dependencies. In other words, the previous in hlocks will
anyway handle it if the previous's gen_id >= the crosslock's gen_id.
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
include/linux/lockdep.h | 11 +++++++++++
kernel/locking/lockdep.c | 32 ++++++++++++++++++++++++++++++--
2 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index f7c730a..e5c5cc4 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -284,6 +284,17 @@ struct held_lock {
*/
struct hist_lock {
/*
+ * We can skip adding a dependency 'a target crosslock -> this
+ * lock', in case that we ensure 'the target crosslock -> the
+ * previous lock in held_locks' to be created. Remember that
+ * 'the previous lock in held_locks -> this lock' is guaranteed
+ * to be created, and 'A -> B' and 'B -> C' cover 'A -> C'.
+ *
+ * Keep the previous's gen_id to make the decision.
+ */
+ unsigned int prev_gen_id;
+
+ /*
* Id for each entry in the ring buffer. This is used to
* decide whether the ring buffer was overwritten or not.
*
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 09f5eec..a14d2ca 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4778,7 +4778,7 @@ static inline int xhlock_valid(struct hist_lock *xhlock)
*
* Irq disable is only required.
*/
-static void add_xhlock(struct held_lock *hlock)
+static void add_xhlock(struct held_lock *hlock, unsigned int prev_gen_id)
{
unsigned int idx = ++current->xhlock_idx;
struct hist_lock *xhlock = &xhlock(idx);
@@ -4793,6 +4793,11 @@ static void add_xhlock(struct held_lock *hlock)
/* Initialize hist_lock's members */
xhlock->hlock = *hlock;
+ /*
+ * prev_gen_id is used to skip adding redundant dependencies,
+ * which can be covered by the previous lock in held_locks.
+ */
+ xhlock->prev_gen_id = prev_gen_id;
xhlock->hist_id = current->hist_id++;
xhlock->trace.nr_entries = 0;
@@ -4813,6 +4818,11 @@ static inline int same_context_xhlock(struct hist_lock *xhlock)
*/
static void check_add_xhlock(struct held_lock *hlock)
{
+ struct held_lock *prev;
+ struct held_lock *start;
+ unsigned int gen_id;
+ unsigned int gen_id_invalid;
+
/*
* Record a hist_lock, only in case that acquisitions ahead
* could depend on the held_lock. For example, if the held_lock
@@ -4822,7 +4832,22 @@ static void check_add_xhlock(struct held_lock *hlock)
if (!current->xhlocks || !depend_before(hlock))
return;
- add_xhlock(hlock);
+ gen_id = (unsigned int)atomic_read(&cross_gen_id);
+ /*
+ * gen_id_invalid should be old enough to be invalid.
+ * Current gen_id - (UINIT_MAX / 4) would be a good
+ * value to meet it.
+ */
+ gen_id_invalid = gen_id - (UINT_MAX / 4);
+ start = current->held_locks;
+
+ for (prev = hlock - 1; prev >= start &&
+ !depend_before(prev); prev--);
+
+ if (prev < start)
+ add_xhlock(hlock, gen_id_invalid);
+ else if (prev->gen_id != gen_id)
+ add_xhlock(hlock, prev->gen_id);
}
/*
@@ -4979,6 +5004,9 @@ static void commit_xhlocks(struct cross_lock *xlock)
prev_hist_id = xhlock->hist_id;
+ if (!before(xhlock->prev_gen_id, xlock->hlock.gen_id))
+ continue;
+
/*
* commit_xhlock() returns 0 with graph_lock already
* released if fail.
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v7 09/16] lockdep: Fix incorrect condition to print bug msgs for MAX_LOCKDEP_CHAIN_HLOCKS
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
In-Reply-To: <1495616389-29772-1-git-send-email-byungchul.park@lge.com>
Bug messages and stack dump for MAX_LOCKDEP_CHAIN_HLOCKS should be
printed only once.
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
kernel/locking/lockdep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index a14d2ca..8173c81 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2267,7 +2267,7 @@ static inline int add_chain_cache(struct task_struct *curr,
* Important for check_no_collision().
*/
if (unlikely(nr_chain_hlocks > MAX_LOCKDEP_CHAIN_HLOCKS)) {
- if (debug_locks_off_graph_unlock())
+ if (!debug_locks_off_graph_unlock())
return 0;
print_lockdep_off("BUG: MAX_LOCKDEP_CHAIN_HLOCKS too low!");
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v7 05/16] lockdep: Implement crossrelease feature
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
In-Reply-To: <1495616389-29772-1-git-send-email-byungchul.park@lge.com>
Lockdep is a runtime locking correctness validator that detects and
reports a deadlock or its possibility by checking dependencies between
locks. It's useful since it does not report just an actual deadlock but
also the possibility of a deadlock that has not actually happened yet.
That enables problems to be fixed before they affect real systems.
However, this facility is only applicable to typical locks, such as
spinlocks and mutexes, which are normally released within the context in
which they were acquired. However, synchronization primitives like page
locks or completions, which are allowed to be released in any context,
also create dependencies and can cause a deadlock. So lockdep should
track these locks to do a better job. The 'crossrelease' implementation
makes these primitives also be tracked.
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
include/linux/irqflags.h | 24 ++-
include/linux/lockdep.h | 111 ++++++++++-
include/linux/sched.h | 8 +
kernel/exit.c | 1 +
kernel/fork.c | 3 +
kernel/locking/lockdep.c | 474 ++++++++++++++++++++++++++++++++++++++++++++---
kernel/workqueue.c | 2 +
lib/Kconfig.debug | 12 ++
8 files changed, 601 insertions(+), 34 deletions(-)
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 5dd1272..c40af8a 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -23,10 +23,26 @@
# define trace_softirq_context(p) ((p)->softirq_context)
# define trace_hardirqs_enabled(p) ((p)->hardirqs_enabled)
# define trace_softirqs_enabled(p) ((p)->softirqs_enabled)
-# define trace_hardirq_enter() do { current->hardirq_context++; } while (0)
-# define trace_hardirq_exit() do { current->hardirq_context--; } while (0)
-# define lockdep_softirq_enter() do { current->softirq_context++; } while (0)
-# define lockdep_softirq_exit() do { current->softirq_context--; } while (0)
+# define trace_hardirq_enter() \
+do { \
+ current->hardirq_context++; \
+ crossrelease_hardirq_start(); \
+} while (0)
+# define trace_hardirq_exit() \
+do { \
+ current->hardirq_context--; \
+ crossrelease_hardirq_end(); \
+} while (0)
+# define lockdep_softirq_enter() \
+do { \
+ current->softirq_context++; \
+ crossrelease_softirq_start(); \
+} while (0)
+# define lockdep_softirq_exit() \
+do { \
+ current->softirq_context--; \
+ crossrelease_softirq_end(); \
+} while (0)
# define INIT_TRACE_IRQFLAGS .softirqs_enabled = 1,
#else
# define trace_hardirqs_on() do { } while (0)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index c1458fe..d531097 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -155,6 +155,12 @@ struct lockdep_map {
int cpu;
unsigned long ip;
#endif
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+ /*
+ * Whether it's a crosslock.
+ */
+ int cross;
+#endif
};
static inline void lockdep_copy_map(struct lockdep_map *to,
@@ -258,7 +264,61 @@ struct held_lock {
unsigned int hardirqs_off:1;
unsigned int references:12; /* 32 bits */
unsigned int pin_count;
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+ /*
+ * Generation id.
+ *
+ * A value of cross_gen_id will be stored when holding this,
+ * which is globally increased whenever each crosslock is held.
+ */
+ unsigned int gen_id;
+#endif
+};
+
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+#define MAX_XHLOCK_TRACE_ENTRIES 5
+
+/*
+ * This is for keeping locks waiting for commit so that true dependencies
+ * can be added at commit step.
+ */
+struct hist_lock {
+ /*
+ * Seperate stack_trace data. This will be used at commit step.
+ */
+ struct stack_trace trace;
+ unsigned long trace_entries[MAX_XHLOCK_TRACE_ENTRIES];
+
+ /*
+ * Seperate hlock instance. This will be used at commit step.
+ *
+ * TODO: Use a smaller data structure containing only necessary
+ * data. However, we should make lockdep code able to handle the
+ * smaller one first.
+ */
+ struct held_lock hlock;
+};
+
+/*
+ * To initialize a lock as crosslock, lockdep_init_map_crosslock() should
+ * be called instead of lockdep_init_map().
+ */
+struct cross_lock {
+ /*
+ * Seperate hlock instance. This will be used at commit step.
+ *
+ * TODO: Use a smaller data structure containing only necessary
+ * data. However, we should make lockdep code able to handle the
+ * smaller one first.
+ */
+ struct held_lock hlock;
+};
+
+struct lockdep_map_cross {
+ struct lockdep_map map;
+ struct cross_lock xlock;
};
+#endif
/*
* Initialization, self-test and debugging-output methods:
@@ -282,13 +342,6 @@ extern void lockdep_init_map(struct lockdep_map *lock, const char *name,
struct lock_class_key *key, int subclass);
/*
- * To initialize a lockdep_map statically use this macro.
- * Note that _name must not be NULL.
- */
-#define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
- { .name = (_name), .key = (void *)(_key), }
-
-/*
* Reinitialize a lock key - for cases where there is special locking or
* special initialization of locks so that the validator gets the scope
* of dependencies wrong: they are either too broad (they need a class-split)
@@ -443,6 +496,50 @@ static inline void lockdep_on(void)
#endif /* !LOCKDEP */
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+extern void lockdep_init_map_crosslock(struct lockdep_map *lock,
+ const char *name,
+ struct lock_class_key *key,
+ int subclass);
+extern void lock_commit_crosslock(struct lockdep_map *lock);
+
+#define STATIC_CROSS_LOCKDEP_MAP_INIT(_name, _key) \
+ { .map.name = (_name), .map.key = (void *)(_key), \
+ .map.cross = 1, }
+
+/*
+ * To initialize a lockdep_map statically use this macro.
+ * Note that _name must not be NULL.
+ */
+#define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
+ { .name = (_name), .key = (void *)(_key), .cross = 0, }
+
+extern void crossrelease_hardirq_start(void);
+extern void crossrelease_hardirq_end(void);
+extern void crossrelease_softirq_start(void);
+extern void crossrelease_softirq_end(void);
+extern void crossrelease_work_start(void);
+extern void crossrelease_work_end(void);
+extern void init_crossrelease_task(struct task_struct *task);
+extern void free_crossrelease_task(struct task_struct *task);
+#else
+/*
+ * To initialize a lockdep_map statically use this macro.
+ * Note that _name must not be NULL.
+ */
+#define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
+ { .name = (_name), .key = (void *)(_key), }
+
+static inline void crossrelease_hardirq_start(void) {}
+static inline void crossrelease_hardirq_end(void) {}
+static inline void crossrelease_softirq_start(void) {}
+static inline void crossrelease_softirq_end(void) {}
+static inline void crossrelease_work_start(void) {}
+static inline void crossrelease_work_end(void) {}
+static inline void init_crossrelease_task(struct task_struct *task) {}
+static inline void free_crossrelease_task(struct task_struct *task) {}
+#endif
+
#ifdef CONFIG_LOCK_STAT
extern void lock_contended(struct lockdep_map *lock, unsigned long ip);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index e9c009d..5f6d6f4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1749,6 +1749,14 @@ struct task_struct {
struct held_lock held_locks[MAX_LOCK_DEPTH];
gfp_t lockdep_reclaim_gfp;
#endif
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+#define MAX_XHLOCKS_NR 64UL
+ struct hist_lock *xhlocks; /* Crossrelease history locks */
+ unsigned int xhlock_idx;
+ unsigned int xhlock_idx_soft; /* For restoring at softirq exit */
+ unsigned int xhlock_idx_hard; /* For restoring at hardirq exit */
+ unsigned int xhlock_idx_work; /* For restoring at work exit */
+#endif
#ifdef CONFIG_UBSAN
unsigned int in_ubsan;
#endif
diff --git a/kernel/exit.c b/kernel/exit.c
index 3076f30..cc56aad 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -883,6 +883,7 @@ void __noreturn do_exit(long code)
exit_rcu();
TASKS_RCU(__srcu_read_unlock(&tasks_rcu_exit_srcu, tasks_rcu_i));
+ free_crossrelease_task(tsk);
do_task_dead();
}
EXPORT_SYMBOL_GPL(do_exit);
diff --git a/kernel/fork.c b/kernel/fork.c
index 997ac1d..f9623a0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -451,6 +451,7 @@ void __init fork_init(void)
for (i = 0; i < UCOUNT_COUNTS; i++) {
init_user_ns.ucount_max[i] = max_threads/2;
}
+ init_crossrelease_task(&init_task);
}
int __weak arch_dup_task_struct(struct task_struct *dst,
@@ -1611,6 +1612,7 @@ static __latent_entropy struct task_struct *copy_process(
p->lockdep_depth = 0; /* no locks held yet */
p->curr_chain_key = 0;
p->lockdep_recursion = 0;
+ init_crossrelease_task(p);
#endif
#ifdef CONFIG_DEBUG_MUTEXES
@@ -1856,6 +1858,7 @@ static __latent_entropy struct task_struct *copy_process(
bad_fork_cleanup_perf:
perf_event_free_task(p);
bad_fork_cleanup_policy:
+ free_crossrelease_task(p);
#ifdef CONFIG_NUMA
mpol_put(p->mempolicy);
bad_fork_cleanup_threadgroup_lock:
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 2847356..63eb04a 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -55,6 +55,10 @@
#define CREATE_TRACE_POINTS
#include <trace/events/lock.h>
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+#include <linux/slab.h>
+#endif
+
#ifdef CONFIG_PROVE_LOCKING
int prove_locking = 1;
module_param(prove_locking, int, 0644);
@@ -709,6 +713,18 @@ static int count_matching_names(struct lock_class *new_class)
return NULL;
}
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+static void cross_init(struct lockdep_map *lock, int cross);
+static int cross_lock(struct lockdep_map *lock);
+static int lock_acquire_crosslock(struct held_lock *hlock);
+static int lock_release_crosslock(struct lockdep_map *lock);
+#else
+static inline void cross_init(struct lockdep_map *lock, int cross) {}
+static inline int cross_lock(struct lockdep_map *lock) { return 0; }
+static inline int lock_acquire_crosslock(struct held_lock *hlock) { return 2; }
+static inline int lock_release_crosslock(struct lockdep_map *lock) { return 2; }
+#endif
+
/*
* Register a lock's class in the hash-table, if the class is not present
* yet. Otherwise we look it up. We cache the result in the lock object
@@ -1768,6 +1784,9 @@ static inline void inc_chains(void)
if (nest)
return 2;
+ if (cross_lock(prev->instance))
+ continue;
+
return print_deadlock_bug(curr, prev, next);
}
return 1;
@@ -1921,30 +1940,36 @@ static inline void inc_chains(void)
int distance = curr->lockdep_depth - depth + 1;
hlock = curr->held_locks + depth - 1;
/*
- * Only non-recursive-read entries get new dependencies
- * added:
+ * Only non-crosslock entries get new dependencies added.
+ * Crosslock entries will be added by commit later:
*/
- if (hlock->read != 2 && hlock->check) {
- int ret = check_prev_add(curr, hlock, next,
- distance, &trace, save);
- if (!ret)
- return 0;
-
+ if (!cross_lock(hlock->instance)) {
/*
- * Stop saving stack_trace if save_trace() was
- * called at least once:
+ * Only non-recursive-read entries get new dependencies
+ * added:
*/
- if (save && ret == 2)
- save = NULL;
+ if (hlock->read != 2 && hlock->check) {
+ int ret = check_prev_add(curr, hlock, next,
+ distance, &trace, save);
+ if (!ret)
+ return 0;
- /*
- * Stop after the first non-trylock entry,
- * as non-trylock entries have added their
- * own direct dependencies already, so this
- * lock is connected to them indirectly:
- */
- if (!hlock->trylock)
- break;
+ /*
+ * Stop saving stack_trace if save_trace() was
+ * called at least once:
+ */
+ if (save && ret == 2)
+ save = NULL;
+
+ /*
+ * Stop after the first non-trylock entry,
+ * as non-trylock entries have added their
+ * own direct dependencies already, so this
+ * lock is connected to them indirectly:
+ */
+ if (!hlock->trylock)
+ break;
+ }
}
depth--;
/*
@@ -3203,7 +3228,7 @@ static int mark_lock(struct task_struct *curr, struct held_lock *this,
/*
* Initialize a lock instance's lock-class mapping info:
*/
-void lockdep_init_map(struct lockdep_map *lock, const char *name,
+static void __lockdep_init_map(struct lockdep_map *lock, const char *name,
struct lock_class_key *key, int subclass)
{
int i;
@@ -3261,8 +3286,25 @@ void lockdep_init_map(struct lockdep_map *lock, const char *name,
raw_local_irq_restore(flags);
}
}
+
+void lockdep_init_map(struct lockdep_map *lock, const char *name,
+ struct lock_class_key *key, int subclass)
+{
+ cross_init(lock, 0);
+ __lockdep_init_map(lock, name, key, subclass);
+}
EXPORT_SYMBOL_GPL(lockdep_init_map);
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+void lockdep_init_map_crosslock(struct lockdep_map *lock, const char *name,
+ struct lock_class_key *key, int subclass)
+{
+ cross_init(lock, 1);
+ __lockdep_init_map(lock, name, key, subclass);
+}
+EXPORT_SYMBOL_GPL(lockdep_init_map_crosslock);
+#endif
+
struct lock_class_key __lockdep_no_validate__;
EXPORT_SYMBOL_GPL(__lockdep_no_validate__);
@@ -3317,6 +3359,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
unsigned int depth;
int chain_head = 0;
int class_idx;
+ int ret;
u64 chain_key;
if (unlikely(!debug_locks))
@@ -3366,7 +3409,8 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
class_idx = class - lock_classes + 1;
- if (depth) {
+ /* TODO: nest_lock is not implemented for crosslock yet. */
+ if (depth && !cross_lock(lock)) {
hlock = curr->held_locks + depth - 1;
if (hlock->class_idx == class_idx && nest_lock) {
if (hlock->references)
@@ -3447,6 +3491,14 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
if (!validate_chain(curr, lock, hlock, chain_head, chain_key))
return 0;
+ ret = lock_acquire_crosslock(hlock);
+ /*
+ * 2 means normal acquire operations are needed. Otherwise, it's
+ * ok just to return with '0:fail, 1:success'.
+ */
+ if (ret != 2)
+ return ret;
+
curr->curr_chain_key = chain_key;
curr->lockdep_depth++;
check_chain_key(curr);
@@ -3610,11 +3662,19 @@ static int match_held_lock(struct held_lock *hlock, struct lockdep_map *lock)
struct task_struct *curr = current;
struct held_lock *hlock, *prev_hlock;
unsigned int depth;
- int i;
+ int ret, i;
if (unlikely(!debug_locks))
return 0;
+ ret = lock_release_crosslock(lock);
+ /*
+ * 2 means normal release operations are needed. Otherwise, it's
+ * ok just to return with '0:fail, 1:success'.
+ */
+ if (ret != 2)
+ return ret;
+
depth = curr->lockdep_depth;
/*
* So we're all set to release this lock.. wait what lock? We don't
@@ -4557,3 +4617,371 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
dump_stack();
}
EXPORT_SYMBOL_GPL(lockdep_rcu_suspicious);
+
+#ifdef CONFIG_LOCKDEP_CROSSRELEASE
+
+#define xhlock(i) (current->xhlocks[(i) % MAX_XHLOCKS_NR])
+
+/*
+ * Whenever a crosslock is held, cross_gen_id will be increased.
+ */
+static atomic_t cross_gen_id; /* Can be wrapped */
+
+void crossrelease_hardirq_start(void)
+{
+ if (current->xhlocks)
+ current->xhlock_idx_hard = current->xhlock_idx;
+}
+
+void crossrelease_hardirq_end(void)
+{
+ if (current->xhlocks)
+ current->xhlock_idx = current->xhlock_idx_hard;
+}
+
+void crossrelease_softirq_start(void)
+{
+ if (current->xhlocks)
+ current->xhlock_idx_soft = current->xhlock_idx;
+}
+
+void crossrelease_softirq_end(void)
+{
+ if (current->xhlocks)
+ current->xhlock_idx = current->xhlock_idx_soft;
+}
+
+/*
+ * Each work of workqueue might run in a different context,
+ * thanks to concurrency support of workqueue. So we have to
+ * distinguish each work to avoid false positive.
+ */
+void crossrelease_work_start(void)
+{
+ if (current->xhlocks)
+ current->xhlock_idx_work = current->xhlock_idx;
+}
+
+void crossrelease_work_end(void)
+{
+ if (current->xhlocks)
+ current->xhlock_idx = current->xhlock_idx_work;
+}
+
+static int cross_lock(struct lockdep_map *lock)
+{
+ return lock ? lock->cross : 0;
+}
+
+/*
+ * This is needed to decide the relationship between wrapable variables.
+ */
+static inline int before(unsigned int a, unsigned int b)
+{
+ return (int)(a - b) < 0;
+}
+
+static inline struct lock_class *xhlock_class(struct hist_lock *xhlock)
+{
+ return hlock_class(&xhlock->hlock);
+}
+
+static inline struct lock_class *xlock_class(struct cross_lock *xlock)
+{
+ return hlock_class(&xlock->hlock);
+}
+
+/*
+ * Should we check a dependency with previous one?
+ */
+static inline int depend_before(struct held_lock *hlock)
+{
+ return hlock->read != 2 && hlock->check && !hlock->trylock;
+}
+
+/*
+ * Should we check a dependency with next one?
+ */
+static inline int depend_after(struct held_lock *hlock)
+{
+ return hlock->read != 2 && hlock->check;
+}
+
+/*
+ * Check if the xhlock is valid, which would be false if,
+ *
+ * 1. Has not used after initializaion yet.
+ *
+ * Remind hist_lock is implemented as a ring buffer.
+ */
+static inline int xhlock_valid(struct hist_lock *xhlock)
+{
+ /*
+ * xhlock->hlock.instance must be !NULL.
+ */
+ return !!xhlock->hlock.instance;
+}
+
+/*
+ * Record a hist_lock entry.
+ *
+ * Irq disable is only required.
+ */
+static void add_xhlock(struct held_lock *hlock)
+{
+ unsigned int idx = ++current->xhlock_idx;
+ struct hist_lock *xhlock = &xhlock(idx);
+
+#ifdef CONFIG_DEBUG_LOCKDEP
+ /*
+ * This can be done locklessly because they are all task-local
+ * state, we must however ensure IRQs are disabled.
+ */
+ WARN_ON_ONCE(!irqs_disabled());
+#endif
+
+ /* Initialize hist_lock's members */
+ xhlock->hlock = *hlock;
+
+ xhlock->trace.nr_entries = 0;
+ xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
+ xhlock->trace.entries = xhlock->trace_entries;
+ xhlock->trace.skip = 3;
+ save_stack_trace(&xhlock->trace);
+}
+
+static inline int same_context_xhlock(struct hist_lock *xhlock)
+{
+ return xhlock->hlock.irq_context == task_irq_context(current);
+}
+
+/*
+ * This should be lockless as far as possible because this would be
+ * called very frequently.
+ */
+static void check_add_xhlock(struct held_lock *hlock)
+{
+ /*
+ * Record a hist_lock, only in case that acquisitions ahead
+ * could depend on the held_lock. For example, if the held_lock
+ * is trylock then acquisitions ahead never depends on that.
+ * In that case, we don't need to record it. Just return.
+ */
+ if (!current->xhlocks || !depend_before(hlock))
+ return;
+
+ add_xhlock(hlock);
+}
+
+/*
+ * For crosslock.
+ */
+static int add_xlock(struct held_lock *hlock)
+{
+ struct cross_lock *xlock;
+ unsigned int gen_id;
+
+ if (!graph_lock())
+ return 0;
+
+ xlock = &((struct lockdep_map_cross *)hlock->instance)->xlock;
+
+ gen_id = (unsigned int)atomic_inc_return(&cross_gen_id);
+ xlock->hlock = *hlock;
+ xlock->hlock.gen_id = gen_id;
+ graph_unlock();
+
+ return 1;
+}
+
+/*
+ * Called for both normal and crosslock acquires. Normal locks will be
+ * pushed on the hist_lock queue. Cross locks will record state and
+ * stop regular lock_acquire() to avoid being placed on the held_lock
+ * stack.
+ *
+ * Return: 0 - failure;
+ * 1 - crosslock, done;
+ * 2 - normal lock, continue to held_lock[] ops.
+ */
+static int lock_acquire_crosslock(struct held_lock *hlock)
+{
+ /*
+ * CONTEXT 1 CONTEXT 2
+ * --------- ---------
+ * lock A (cross)
+ * X = atomic_inc_return(&cross_gen_id)
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * Y = atomic_read_acquire(&cross_gen_id)
+ * lock B
+ *
+ * atomic_read_acquire() is for ordering between A and B,
+ * IOW, A happens before B, when CONTEXT 2 see Y >= X.
+ *
+ * Pairs with atomic_inc_return() in add_xlock().
+ */
+ hlock->gen_id = (unsigned int)atomic_read_acquire(&cross_gen_id);
+
+ if (cross_lock(hlock->instance))
+ return add_xlock(hlock);
+
+ check_add_xhlock(hlock);
+ return 2;
+}
+
+static int copy_trace(struct stack_trace *trace)
+{
+ unsigned long *buf = stack_trace + nr_stack_trace_entries;
+ unsigned int max_nr = MAX_STACK_TRACE_ENTRIES - nr_stack_trace_entries;
+ unsigned int nr = min(max_nr, trace->nr_entries);
+
+ trace->nr_entries = nr;
+ memcpy(buf, trace->entries, nr * sizeof(trace->entries[0]));
+ trace->entries = buf;
+ nr_stack_trace_entries += nr;
+
+ if (nr_stack_trace_entries >= MAX_STACK_TRACE_ENTRIES-1) {
+ if (!debug_locks_off_graph_unlock())
+ return 0;
+
+ print_lockdep_off("BUG: MAX_STACK_TRACE_ENTRIES too low!");
+ dump_stack();
+
+ return 0;
+ }
+
+ return 1;
+}
+
+static int commit_xhlock(struct cross_lock *xlock, struct hist_lock *xhlock)
+{
+ unsigned int xid, pid;
+ u64 chain_key;
+
+ xid = xlock_class(xlock) - lock_classes;
+ chain_key = iterate_chain_key((u64)0, xid);
+ pid = xhlock_class(xhlock) - lock_classes;
+ chain_key = iterate_chain_key(chain_key, pid);
+
+ if (lookup_chain_cache(chain_key))
+ return 1;
+
+ if (!add_chain_cache_classes(xid, pid, xhlock->hlock.irq_context,
+ chain_key))
+ return 0;
+
+ if (!check_prev_add(current, &xlock->hlock, &xhlock->hlock, 1,
+ &xhlock->trace, copy_trace))
+ return 0;
+
+ return 1;
+}
+
+static void commit_xhlocks(struct cross_lock *xlock)
+{
+ unsigned int cur = current->xhlock_idx;
+ unsigned int i;
+
+ if (!graph_lock())
+ return;
+
+ for (i = 0; i < MAX_XHLOCKS_NR; i++) {
+ struct hist_lock *xhlock = &xhlock(cur - i);
+
+ if (!xhlock_valid(xhlock))
+ break;
+
+ if (before(xhlock->hlock.gen_id, xlock->hlock.gen_id))
+ break;
+
+ if (!same_context_xhlock(xhlock))
+ break;
+
+ /*
+ * commit_xhlock() returns 0 with graph_lock already
+ * released if fail.
+ */
+ if (!commit_xhlock(xlock, xhlock))
+ return;
+ }
+
+ graph_unlock();
+}
+
+void lock_commit_crosslock(struct lockdep_map *lock)
+{
+ struct cross_lock *xlock;
+ unsigned long flags;
+
+ if (unlikely(!debug_locks || current->lockdep_recursion))
+ return;
+
+ if (!current->xhlocks)
+ return;
+
+ /*
+ * Do commit hist_locks with the cross_lock, only in case that
+ * the cross_lock could depend on acquisitions after that.
+ *
+ * For example, if the cross_lock does not have the 'check' flag
+ * then we don't need to check dependencies and commit for that.
+ * Just skip it. In that case, of course, the cross_lock does
+ * not depend on acquisitions ahead, either.
+ *
+ * WARNING: Don't do that in add_xlock() in advance. When an
+ * acquisition context is different from the commit context,
+ * invalid(skipped) cross_lock might be accessed.
+ */
+ if (!depend_after(&((struct lockdep_map_cross *)lock)->xlock.hlock))
+ return;
+
+ raw_local_irq_save(flags);
+ check_flags(flags);
+ current->lockdep_recursion = 1;
+ xlock = &((struct lockdep_map_cross *)lock)->xlock;
+ commit_xhlocks(xlock);
+ current->lockdep_recursion = 0;
+ raw_local_irq_restore(flags);
+}
+EXPORT_SYMBOL_GPL(lock_commit_crosslock);
+
+/*
+ * Return: 1 - crosslock, done;
+ * 2 - normal lock, continue to held_lock[] ops.
+ */
+static int lock_release_crosslock(struct lockdep_map *lock)
+{
+ return cross_lock(lock) ? 1 : 2;
+}
+
+static void cross_init(struct lockdep_map *lock, int cross)
+{
+ lock->cross = cross;
+
+ /*
+ * Crossrelease assumes that the ring buffer size of xhlocks
+ * is aligned with power of 2. So force it on build.
+ */
+ BUILD_BUG_ON(MAX_XHLOCKS_NR & (MAX_XHLOCKS_NR - 1));
+}
+
+void init_crossrelease_task(struct task_struct *task)
+{
+ task->xhlock_idx = UINT_MAX;
+ task->xhlock_idx_soft = UINT_MAX;
+ task->xhlock_idx_hard = UINT_MAX;
+ task->xhlock_idx_work = UINT_MAX;
+ task->xhlocks = kzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR,
+ GFP_KERNEL);
+}
+
+void free_crossrelease_task(struct task_struct *task)
+{
+ if (task->xhlocks) {
+ void *tmp = task->xhlocks;
+ /* Diable crossrelease for current */
+ task->xhlocks = NULL;
+ kfree(tmp);
+ }
+}
+#endif
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 479d840..2f43ac1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2092,6 +2092,7 @@ static void process_one_work(struct worker *worker, struct work_struct *work)
lock_map_acquire_read(&pwq->wq->lockdep_map);
lock_map_acquire(&lockdep_map);
+ crossrelease_work_start();
trace_workqueue_execute_start(work);
worker->current_func(work);
/*
@@ -2099,6 +2100,7 @@ static void process_one_work(struct worker *worker, struct work_struct *work)
* point will only record its address.
*/
trace_workqueue_execute_end(work);
+ crossrelease_work_end();
lock_map_release(&lockdep_map);
lock_map_release(&pwq->wq->lockdep_map);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index a6c8db1..e584431 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1042,6 +1042,18 @@ config DEBUG_LOCK_ALLOC
spin_lock_init()/mutex_init()/etc., or whether there is any lock
held during task exit.
+config LOCKDEP_CROSSRELEASE
+ bool "Lock debugging: make lockdep work for crosslocks"
+ depends on PROVE_LOCKING
+ default n
+ help
+ This makes lockdep work for crosslock which is a lock allowed to
+ be released in a different context from the acquisition context.
+ Normally a lock must be released in the context acquiring the lock.
+ However, relexing this constraint helps synchronization primitives
+ such as page locks or completions can use the lock correctness
+ detector, lockdep.
+
config PROVE_LOCKING
bool "Lock debugging: prove locking correctness"
depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v7 06/16] lockdep: Detect and handle hist_lock ring buffer overwrite
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
In-Reply-To: <1495616389-29772-1-git-send-email-byungchul.park@lge.com>
The ring buffer can be overwritten by hardirq/softirq/work contexts.
That cases must be considered on rollback or commit. For example,
|<------ hist_lock ring buffer size ----->|
ppppppppppppiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii
wrapped > iiiiiiiiiiiiiiiiiiiiiii....................
where 'p' represents an acquisition in process context,
'i' represents an acquisition in irq context.
On irq exit, crossrelease tries to rollback idx to original position,
but it should not because the entry already has been invalid by
overwriting 'i'. Avoid rollback or commit for entries overwritten.
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
include/linux/lockdep.h | 20 +++++++++++
include/linux/sched.h | 4 +++
kernel/locking/lockdep.c | 92 +++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 104 insertions(+), 12 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d531097..a03f79d 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -284,6 +284,26 @@ struct held_lock {
*/
struct hist_lock {
/*
+ * Id for each entry in the ring buffer. This is used to
+ * decide whether the ring buffer was overwritten or not.
+ *
+ * For example,
+ *
+ * |<----------- hist_lock ring buffer size ------->|
+ * pppppppppppppppppppppiiiiiiiiiiiiiiiiiiiiiiiiiiiii
+ * wrapped > iiiiiiiiiiiiiiiiiiiiiiiiiii.......................
+ *
+ * where 'p' represents an acquisition in process
+ * context, 'i' represents an acquisition in irq
+ * context.
+ *
+ * In this example, the ring buffer was overwritten by
+ * acquisitions in irq context, that should be detected on
+ * rollback or commit.
+ */
+ unsigned int hist_id;
+
+ /*
* Seperate stack_trace data. This will be used at commit step.
*/
struct stack_trace trace;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5f6d6f4..9e1437c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1756,6 +1756,10 @@ struct task_struct {
unsigned int xhlock_idx_soft; /* For restoring at softirq exit */
unsigned int xhlock_idx_hard; /* For restoring at hardirq exit */
unsigned int xhlock_idx_work; /* For restoring at work exit */
+ unsigned int hist_id;
+ unsigned int hist_id_soft; /* For overwrite check at softirq exit */
+ unsigned int hist_id_hard; /* For overwrite check at hardirq exit */
+ unsigned int hist_id_work; /* For overwrite check at work exit */
#endif
#ifdef CONFIG_UBSAN
unsigned int in_ubsan;
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 63eb04a..26ff205 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4627,28 +4627,65 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
*/
static atomic_t cross_gen_id; /* Can be wrapped */
+/*
+ * Make an entry of the ring buffer invalid.
+ */
+static inline void invalidate_xhlock(struct hist_lock *xhlock)
+{
+ /*
+ * Normally, xhlock->hlock.instance must be !NULL.
+ */
+ xhlock->hlock.instance = NULL;
+}
+
void crossrelease_hardirq_start(void)
{
- if (current->xhlocks)
- current->xhlock_idx_hard = current->xhlock_idx;
+ struct task_struct *cur = current;
+
+ if (cur->xhlocks) {
+ cur->xhlock_idx_hard = cur->xhlock_idx;
+ cur->hist_id_hard = cur->hist_id;
+ }
}
void crossrelease_hardirq_end(void)
{
- if (current->xhlocks)
- current->xhlock_idx = current->xhlock_idx_hard;
+ struct task_struct *cur = current;
+
+ if (cur->xhlocks) {
+ unsigned int idx = cur->xhlock_idx_hard;
+ struct hist_lock *h = &xhlock(idx);
+
+ cur->xhlock_idx = idx;
+ /* Check if the ring was overwritten. */
+ if (h->hist_id != cur->hist_id_hard)
+ invalidate_xhlock(h);
+ }
}
void crossrelease_softirq_start(void)
{
- if (current->xhlocks)
- current->xhlock_idx_soft = current->xhlock_idx;
+ struct task_struct *cur = current;
+
+ if (cur->xhlocks) {
+ cur->xhlock_idx_soft = cur->xhlock_idx;
+ cur->hist_id_soft = cur->hist_id;
+ }
}
void crossrelease_softirq_end(void)
{
- if (current->xhlocks)
- current->xhlock_idx = current->xhlock_idx_soft;
+ struct task_struct *cur = current;
+
+ if (cur->xhlocks) {
+ unsigned int idx = cur->xhlock_idx_soft;
+ struct hist_lock *h = &xhlock(idx);
+
+ cur->xhlock_idx = idx;
+ /* Check if the ring was overwritten. */
+ if (h->hist_id != cur->hist_id_soft)
+ invalidate_xhlock(h);
+ }
}
/*
@@ -4658,14 +4695,27 @@ void crossrelease_softirq_end(void)
*/
void crossrelease_work_start(void)
{
- if (current->xhlocks)
- current->xhlock_idx_work = current->xhlock_idx;
+ struct task_struct *cur = current;
+
+ if (cur->xhlocks) {
+ cur->xhlock_idx_work = cur->xhlock_idx;
+ cur->hist_id_work = cur->hist_id;
+ }
}
void crossrelease_work_end(void)
{
- if (current->xhlocks)
- current->xhlock_idx = current->xhlock_idx_work;
+ struct task_struct *cur = current;
+
+ if (cur->xhlocks) {
+ unsigned int idx = cur->xhlock_idx_work;
+ struct hist_lock *h = &xhlock(idx);
+
+ cur->xhlock_idx = idx;
+ /* Check if the ring was overwritten. */
+ if (h->hist_id != cur->hist_id_work)
+ invalidate_xhlock(h);
+ }
}
static int cross_lock(struct lockdep_map *lock)
@@ -4711,6 +4761,7 @@ static inline int depend_after(struct held_lock *hlock)
* Check if the xhlock is valid, which would be false if,
*
* 1. Has not used after initializaion yet.
+ * 2. Got invalidated.
*
* Remind hist_lock is implemented as a ring buffer.
*/
@@ -4742,6 +4793,7 @@ static void add_xhlock(struct held_lock *hlock)
/* Initialize hist_lock's members */
xhlock->hlock = *hlock;
+ xhlock->hist_id = current->hist_id++;
xhlock->trace.nr_entries = 0;
xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
@@ -4880,6 +4932,7 @@ static int commit_xhlock(struct cross_lock *xlock, struct hist_lock *xhlock)
static void commit_xhlocks(struct cross_lock *xlock)
{
unsigned int cur = current->xhlock_idx;
+ unsigned int prev_hist_id = xhlock(cur).hist_id;
unsigned int i;
if (!graph_lock())
@@ -4898,6 +4951,17 @@ static void commit_xhlocks(struct cross_lock *xlock)
break;
/*
+ * Filter out the cases that the ring buffer was
+ * overwritten and the previous entry has a bigger
+ * hist_id than the following one, which is impossible
+ * otherwise.
+ */
+ if (unlikely(before(xhlock->hist_id, prev_hist_id)))
+ break;
+
+ prev_hist_id = xhlock->hist_id;
+
+ /*
* commit_xhlock() returns 0 with graph_lock already
* released if fail.
*/
@@ -4967,6 +5031,10 @@ static void cross_init(struct lockdep_map *lock, int cross)
void init_crossrelease_task(struct task_struct *task)
{
+ task->hist_id = 0;
+ task->hist_id_soft = 0;
+ task->hist_id_hard = 0;
+ task->hist_id_work = 0;
task->xhlock_idx = UINT_MAX;
task->xhlock_idx_soft = UINT_MAX;
task->xhlock_idx_hard = UINT_MAX;
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v7 02/16] lockdep: Add a function building a chain between two classes
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
In-Reply-To: <1495616389-29772-1-git-send-email-byungchul.park@lge.com>
Crossrelease needs to build a chain between two classes regardless of
their contexts. However, add_chain_cache() cannot be used for that
purpose since it assumes that it's called in the acquisition context
of the hlock. So this patch introduces a new function doing it.
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
kernel/locking/lockdep.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 0c6e6b7..eb39474 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2110,6 +2110,76 @@ static int check_no_collision(struct task_struct *curr,
}
/*
+ * This is for building a chain between just two different classes,
+ * instead of adding a new hlock upon current, which is done by
+ * add_chain_cache().
+ *
+ * This can be called in any context with two classes, while
+ * add_chain_cache() must be done within the lock owener's context
+ * since it uses hlock which might be racy in another context.
+ */
+static inline int add_chain_cache_classes(unsigned int prev,
+ unsigned int next,
+ unsigned int irq_context,
+ u64 chain_key)
+{
+ struct hlist_head *hash_head = chainhashentry(chain_key);
+ struct lock_chain *chain;
+
+ /*
+ * Allocate a new chain entry from the static array, and add
+ * it to the hash:
+ */
+
+ /*
+ * We might need to take the graph lock, ensure we've got IRQs
+ * disabled to make this an IRQ-safe lock.. for recursion reasons
+ * lockdep won't complain about its own locking errors.
+ */
+ if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
+ return 0;
+
+ if (unlikely(nr_lock_chains >= MAX_LOCKDEP_CHAINS)) {
+ if (!debug_locks_off_graph_unlock())
+ return 0;
+
+ print_lockdep_off("BUG: MAX_LOCKDEP_CHAINS too low!");
+ dump_stack();
+ return 0;
+ }
+
+ chain = lock_chains + nr_lock_chains++;
+ chain->chain_key = chain_key;
+ chain->irq_context = irq_context;
+ chain->depth = 2;
+ if (likely(nr_chain_hlocks + chain->depth <= MAX_LOCKDEP_CHAIN_HLOCKS)) {
+ chain->base = nr_chain_hlocks;
+ nr_chain_hlocks += chain->depth;
+ chain_hlocks[chain->base] = prev - 1;
+ chain_hlocks[chain->base + 1] = next -1;
+ }
+#ifdef CONFIG_DEBUG_LOCKDEP
+ /*
+ * Important for check_no_collision().
+ */
+ else {
+ if (!debug_locks_off_graph_unlock())
+ return 0;
+
+ print_lockdep_off("BUG: MAX_LOCKDEP_CHAIN_HLOCKS too low!");
+ dump_stack();
+ return 0;
+ }
+#endif
+
+ hlist_add_head_rcu(&chain->entry, hash_head);
+ debug_atomic_inc(chain_lookup_misses);
+ inc_chains();
+
+ return 1;
+}
+
+/*
* Adds a dependency chain into chain hashtable. And must be called with
* graph_lock held.
*
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v7 04/16] lockdep: Make check_prev_add() able to handle external stack_trace
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
In-Reply-To: <1495616389-29772-1-git-send-email-byungchul.park@lge.com>
Currently, a space for stack_trace is pinned in check_prev_add(), that
makes us not able to use external stack_trace. The simplest way to
achieve it is to pass an external stack_trace as an argument.
A more suitable solution is to pass a callback additionally along with
a stack_trace so that callers can decide the way to save or whether to
save. Actually crossrelease needs to do other than saving a stack_trace.
So pass a stack_trace and callback to handle it, to check_prev_add().
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
kernel/locking/lockdep.c | 40 +++++++++++++++++++---------------------
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 4709110..2847356 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1797,20 +1797,13 @@ static inline void inc_chains(void)
*/
static int
check_prev_add(struct task_struct *curr, struct held_lock *prev,
- struct held_lock *next, int distance, int *stack_saved)
+ struct held_lock *next, int distance, struct stack_trace *trace,
+ int (*save)(struct stack_trace *trace))
{
struct lock_list *entry;
int ret;
struct lock_list this;
struct lock_list *uninitialized_var(target_entry);
- /*
- * Static variable, serialized by the graph_lock().
- *
- * We use this static variable to save the stack trace in case
- * we call into this function multiple times due to encountering
- * trylocks in the held lock stack.
- */
- static struct stack_trace trace;
/*
* Prove that the new <prev> -> <next> dependency would not
@@ -1858,11 +1851,8 @@ static inline void inc_chains(void)
}
}
- if (!*stack_saved) {
- if (!save_trace(&trace))
- return 0;
- *stack_saved = 1;
- }
+ if (save && !save(trace))
+ return 0;
/*
* Ok, all validations passed, add the new lock
@@ -1870,14 +1860,14 @@ static inline void inc_chains(void)
*/
ret = add_lock_to_list(hlock_class(prev), hlock_class(next),
&hlock_class(prev)->locks_after,
- next->acquire_ip, distance, &trace);
+ next->acquire_ip, distance, trace);
if (!ret)
return 0;
ret = add_lock_to_list(hlock_class(next), hlock_class(prev),
&hlock_class(next)->locks_before,
- next->acquire_ip, distance, &trace);
+ next->acquire_ip, distance, trace);
if (!ret)
return 0;
@@ -1885,8 +1875,6 @@ static inline void inc_chains(void)
* Debugging printouts:
*/
if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
- /* We drop graph lock, so another thread can overwrite trace. */
- *stack_saved = 0;
graph_unlock();
printk("\n new dependency: ");
print_lock_name(hlock_class(prev));
@@ -1910,8 +1898,9 @@ static inline void inc_chains(void)
check_prevs_add(struct task_struct *curr, struct held_lock *next)
{
int depth = curr->lockdep_depth;
- int stack_saved = 0;
struct held_lock *hlock;
+ struct stack_trace trace;
+ int (*save)(struct stack_trace *trace) = save_trace;
/*
* Debugging checks.
@@ -1936,9 +1925,18 @@ static inline void inc_chains(void)
* added:
*/
if (hlock->read != 2 && hlock->check) {
- if (!check_prev_add(curr, hlock, next,
- distance, &stack_saved))
+ int ret = check_prev_add(curr, hlock, next,
+ distance, &trace, save);
+ if (!ret)
return 0;
+
+ /*
+ * Stop saving stack_trace if save_trace() was
+ * called at least once:
+ */
+ if (save && ret == 2)
+ save = NULL;
+
/*
* Stop after the first non-trylock entry,
* as non-trylock entries have added their
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v7 03/16] lockdep: Change the meaning of check_prev_add()'s return value
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
In-Reply-To: <1495616389-29772-1-git-send-email-byungchul.park@lge.com>
Firstly, return 1 instead of 2 when 'prev -> next' dependency already
exists. Since the value 2 is not referenced anywhere, just return 1
indicating success in this case.
Secondly, return 2 instead of 1 when successfully added a lock_list
entry with saving stack_trace. With that, a caller can decide whether
to avoid redundant save_trace() on the caller site.
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
kernel/locking/lockdep.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index eb39474..4709110 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1854,7 +1854,7 @@ static inline void inc_chains(void)
if (entry->class == hlock_class(next)) {
if (distance == 1)
entry->distance = 1;
- return 2;
+ return 1;
}
}
@@ -1894,9 +1894,10 @@ static inline void inc_chains(void)
print_lock_name(hlock_class(next));
printk(KERN_CONT "\n");
dump_stack();
- return graph_lock();
+ if (!graph_lock())
+ return 0;
}
- return 1;
+ return 2;
}
/*
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v7 00/16] lockdep: Implement crossrelease feature
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
I checked if crossrelease feature works well on my qemu-i386 machine.
There's no problem at all to work on mine. But I wonder if it's still
true on other machines. Especially, on large system. Could you let me
know if it doesn't work on yours or if crossrelease feature is useful?
-----8<-----
Change from v6
- unwind the ring buffer instead tagging for 'work' context
- introduce hist_id to distinguish every entry of ring buffer
- change the point calling crossrelease_work_start()
- handle cases the ring buffer was overwritten
- change LOCKDEP_CROSSRELEASE config in Kconfig
(select PROVE_LOCKING -> depends on PROVE_LOCKING)
- rename xhlock_used() -> xhlock_valid()
- simplify serveral code (e.g. traversal the ring buffer)
- add/enhance several comments and changelogs
Change from v5
- force XHLOCKS_SIZE to be power of 2 and simplify code
- remove nmi check
- separate an optimization using prev_gen_id with a full changelog
- separate non(multi)-acquisition handling with a full changelog
- replace vmalloc with kmallock(GFP_KERNEL) for xhlocks
- select PROVE_LOCKING when choosing CROSSRELEASE
- clean serveral code (e.g. loose some ifdefferies)
- enhance several comments and changelogs
Change from v4
- rebase on vanilla v4.9 tag
- re-name pend_lock(plock) to hist_lock(xhlock)
- allow overwriting ring buffer for hist_lock
- unwind ring buffer instead of tagging id for each irq
- introduce lockdep_map_cross embedding cross_lock
- make each work of workqueue distinguishable
- enhance comments
(I will update the document at the next spin.)
Change from v3
- reviced document
Change from v2
- rebase on vanilla v4.7 tag
- move lockdep data for page lock from struct page to page_ext
- allocate plocks buffer via vmalloc instead of in struct task
- enhanced comments and document
- optimize performance
- make reporting function crossrelease-aware
Change from v1
- enhanced the document
- removed save_stack_trace() optimizing patch
- made this based on the seperated save_stack_trace patchset
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1182242.html
Can we detect deadlocks below with original lockdep?
Example 1)
PROCESS X PROCESS Y
-------------- --------------
mutext_lock A
lock_page B
lock_page B
mutext_lock A // DEADLOCK
unlock_page B
mutext_unlock A
mutex_unlock A
unlock_page B
where A and B are different lock classes.
No, we cannot.
Example 2)
PROCESS X PROCESS Y PROCESS Z
-------------- -------------- --------------
mutex_lock A
lock_page B
lock_page B
mutext_lock A // DEADLOCK
mutext_unlock A
unlock_page B
(B was held by PROCESS X)
unlock_page B
mutex_unlock A
where A and B are different lock classes.
No, we cannot.
Example 3)
PROCESS X PROCESS Y
-------------- --------------
mutex_lock A
mutex_lock A
wait_for_complete B // DEADLOCK
mutex_unlock A
complete B
mutex_unlock A
where A is a lock class and B is a completion variable.
No, we cannot.
Not only lock operations, but also any operations causing to wait or
spin for something can cause deadlock unless it's eventually *released*
by someone. The important point here is that the waiting or spinning
must be *released* by someone.
Using crossrelease feature, we can check dependency and detect deadlock
possibility not only for typical lock, but also for lock_page(),
wait_for_xxx() and so on, which might be released in any context.
See the last patch including the document for more information.
Byungchul Park (16):
lockdep: Refactor lookup_chain_cache()
lockdep: Add a function building a chain between two classes
lockdep: Change the meaning of check_prev_add()'s return value
lockdep: Make check_prev_add() able to handle external stack_trace
lockdep: Implement crossrelease feature
lockdep: Detect and handle hist_lock ring buffer overwrite
lockdep: Handle non(or multi)-acquisition of a crosslock
lockdep: Avoid adding redundant direct links of crosslocks
lockdep: Fix incorrect condition to print bug msgs for
MAX_LOCKDEP_CHAIN_HLOCKS
lockdep: Make print_circular_bug() aware of crossrelease
lockdep: Apply crossrelease to completions
pagemap.h: Remove trailing white space
lockdep: Apply crossrelease to PG_locked locks
lockdep: Apply lock_acquire(release) on __Set(__Clear)PageLocked
lockdep: Move data of CONFIG_LOCKDEP_PAGELOCK from page to page_ext
lockdep: Crossrelease feature documentation
Documentation/locking/crossrelease.txt | 874 ++++++++++++++++++++++++++++++++
include/linux/completion.h | 118 ++++-
include/linux/irqflags.h | 24 +-
include/linux/lockdep.h | 162 +++++-
include/linux/mm_types.h | 4 +
include/linux/page-flags.h | 43 +-
include/linux/page_ext.h | 4 +
include/linux/pagemap.h | 125 ++++-
include/linux/sched.h | 12 +
kernel/exit.c | 1 +
kernel/fork.c | 3 +
kernel/locking/lockdep.c | 882 +++++++++++++++++++++++++++++----
kernel/sched/completion.c | 54 +-
kernel/workqueue.c | 2 +
lib/Kconfig.debug | 29 ++
mm/filemap.c | 73 ++-
mm/page_ext.c | 4 +
17 files changed, 2262 insertions(+), 152 deletions(-)
create mode 100644 Documentation/locking/crossrelease.txt
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH v7 01/16] lockdep: Refactor lookup_chain_cache()
From: Byungchul Park @ 2017-05-24 8:59 UTC (permalink / raw)
To: peterz, mingo
Cc: tglx, walken, boqun.feng, kirill, linux-kernel, linux-mm, akpm,
willy, npiggin, kernel-team
In-Reply-To: <1495616389-29772-1-git-send-email-byungchul.park@lge.com>
Currently, lookup_chain_cache() provides both 'lookup' and 'add'
functionalities in a function. However, each is useful. So this
patch makes lookup_chain_cache() only do 'lookup' functionality and
makes add_chain_cahce() only do 'add' functionality. And it's more
readable than before.
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
kernel/locking/lockdep.c | 132 ++++++++++++++++++++++++++++++-----------------
1 file changed, 86 insertions(+), 46 deletions(-)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 4d7ffc0..0c6e6b7 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2110,14 +2110,15 @@ static int check_no_collision(struct task_struct *curr,
}
/*
- * Look up a dependency chain. If the key is not present yet then
- * add it and return 1 - in this case the new dependency chain is
- * validated. If the key is already hashed, return 0.
- * (On return with 1 graph_lock is held.)
+ * Adds a dependency chain into chain hashtable. And must be called with
+ * graph_lock held.
+ *
+ * Return 0 if fail, and graph_lock is released.
+ * Return 1 if succeed, with graph_lock held.
*/
-static inline int lookup_chain_cache(struct task_struct *curr,
- struct held_lock *hlock,
- u64 chain_key)
+static inline int add_chain_cache(struct task_struct *curr,
+ struct held_lock *hlock,
+ u64 chain_key)
{
struct lock_class *class = hlock_class(hlock);
struct hlist_head *hash_head = chainhashentry(chain_key);
@@ -2125,49 +2126,18 @@ static inline int lookup_chain_cache(struct task_struct *curr,
int i, j;
/*
+ * Allocate a new chain entry from the static array, and add
+ * it to the hash:
+ */
+
+ /*
* We might need to take the graph lock, ensure we've got IRQs
* disabled to make this an IRQ-safe lock.. for recursion reasons
* lockdep won't complain about its own locking errors.
*/
if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
return 0;
- /*
- * We can walk it lock-free, because entries only get added
- * to the hash:
- */
- hlist_for_each_entry_rcu(chain, hash_head, entry) {
- if (chain->chain_key == chain_key) {
-cache_hit:
- debug_atomic_inc(chain_lookup_hits);
- if (!check_no_collision(curr, hlock, chain))
- return 0;
- if (very_verbose(class))
- printk("\nhash chain already cached, key: "
- "%016Lx tail class: [%p] %s\n",
- (unsigned long long)chain_key,
- class->key, class->name);
- return 0;
- }
- }
- if (very_verbose(class))
- printk("\nnew hash chain, key: %016Lx tail class: [%p] %s\n",
- (unsigned long long)chain_key, class->key, class->name);
- /*
- * Allocate a new chain entry from the static array, and add
- * it to the hash:
- */
- if (!graph_lock())
- return 0;
- /*
- * We have to walk the chain again locked - to avoid duplicates:
- */
- hlist_for_each_entry(chain, hash_head, entry) {
- if (chain->chain_key == chain_key) {
- graph_unlock();
- goto cache_hit;
- }
- }
if (unlikely(nr_lock_chains >= MAX_LOCKDEP_CHAINS)) {
if (!debug_locks_off_graph_unlock())
return 0;
@@ -2219,6 +2189,75 @@ static inline int lookup_chain_cache(struct task_struct *curr,
return 1;
}
+/*
+ * Look up a dependency chain.
+ */
+static inline struct lock_chain *lookup_chain_cache(u64 chain_key)
+{
+ struct hlist_head *hash_head = chainhashentry(chain_key);
+ struct lock_chain *chain;
+
+ /*
+ * We can walk it lock-free, because entries only get added
+ * to the hash:
+ */
+ hlist_for_each_entry_rcu(chain, hash_head, entry) {
+ if (chain->chain_key == chain_key) {
+ debug_atomic_inc(chain_lookup_hits);
+ return chain;
+ }
+ }
+ return NULL;
+}
+
+/*
+ * If the key is not present yet in dependency chain cache then
+ * add it and return 1 - in this case the new dependency chain is
+ * validated. If the key is already hashed, return 0.
+ * (On return with 1 graph_lock is held.)
+ */
+static inline int lookup_chain_cache_add(struct task_struct *curr,
+ struct held_lock *hlock,
+ u64 chain_key)
+{
+ struct lock_class *class = hlock_class(hlock);
+ struct lock_chain *chain = lookup_chain_cache(chain_key);
+
+ if (chain) {
+cache_hit:
+ if (!check_no_collision(curr, hlock, chain))
+ return 0;
+
+ if (very_verbose(class))
+ printk("\nhash chain already cached, key: "
+ "%016Lx tail class: [%p] %s\n",
+ (unsigned long long)chain_key,
+ class->key, class->name);
+ return 0;
+ }
+
+ if (very_verbose(class))
+ printk("\nnew hash chain, key: %016Lx tail class: [%p] %s\n",
+ (unsigned long long)chain_key, class->key, class->name);
+
+ if (!graph_lock())
+ return 0;
+
+ /*
+ * We have to walk the chain again locked - to avoid duplicates:
+ */
+ chain = lookup_chain_cache(chain_key);
+ if (chain) {
+ graph_unlock();
+ goto cache_hit;
+ }
+
+ if (!add_chain_cache(curr, hlock, chain_key))
+ return 0;
+
+ return 1;
+}
+
static int validate_chain(struct task_struct *curr, struct lockdep_map *lock,
struct held_lock *hlock, int chain_head, u64 chain_key)
{
@@ -2229,11 +2268,11 @@ static int validate_chain(struct task_struct *curr, struct lockdep_map *lock,
*
* We look up the chain_key and do the O(N^2) check and update of
* the dependencies only if this is a new dependency chain.
- * (If lookup_chain_cache() returns with 1 it acquires
+ * (If lookup_chain_cache_add() return with 1 it acquires
* graph_lock for us)
*/
if (!hlock->trylock && hlock->check &&
- lookup_chain_cache(curr, hlock, chain_key)) {
+ lookup_chain_cache_add(curr, hlock, chain_key)) {
/*
* Check whether last held lock:
*
@@ -2264,9 +2303,10 @@ static int validate_chain(struct task_struct *curr, struct lockdep_map *lock,
if (!chain_head && ret != 2)
if (!check_prevs_add(curr, hlock))
return 0;
+
graph_unlock();
} else
- /* after lookup_chain_cache(): */
+ /* after lookup_chain_cache_add(): */
if (unlikely(!debug_locks))
return 0;
--
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [-next] memory hotplug regression
From: Michal Hocko @ 2017-05-24 8:39 UTC (permalink / raw)
To: Heiko Carstens; +Cc: Gerald Schaefer, linux-mm, linux-kernel
In-Reply-To: <20170524082022.GC5427@osiris>
On Wed 24-05-17 10:20:22, Heiko Carstens wrote:
> Hello Michal,
>
> I just re-tested linux-next with respect to your memory hotplug changes and
> actually (finally) figured out that your patch ("mm, memory_hotplug: do not
> associate hotadded memory to zones until online)" changes behaviour on
> s390:
>
> before your patch memory blocks that were offline and located behind the
> last online memory block were added by default to ZONE_MOVABLE:
>
> # cat /sys/devices/system/memory/memory16/valid_zones
> Movable Normal
>
> With your patch this changes, so that they will be added to ZONE_NORMAL by
> default instead:
>
> # cat /sys/devices/system/memory/memory16/valid_zones
> Normal Movable
>
> Sorry, that I didn't realize this earlier!
>
> Having the ZONE_MOVABLE default was actually the only point why s390's
> arch_add_memory() was rather complex compared to other architectures.
>
> We always had this behaviour, since we always wanted to be able to offline
> memory after it was brought online. Given that back then "online_movable"
> did not exist, the initial s390 memory hotplug support simply added all
> additional memory to ZONE_MOVABLE.
>
> Keeping the default the same would be quite important.
Hmm, that is really unfortunate because I would _really_ like to get rid
of the previous semantic which was really awkward. The whole point of
the rework is to get rid of the nasty zone shifting.
Is it an option to use `online_movable' rather than `online' in your setup?
Btw. my long term plan is to remove the zone range constrains altogether
so you could online each memblock to the type you want. Would that be
sufficient for you in general?
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [Question] Mlocked count will not be decreased
From: Yisheng Xie @ 2017-05-24 8:32 UTC (permalink / raw)
To: Kefeng Wang, linux-mm, linux-kernel, zhongjiang, Qiuxishi
In-Reply-To: <a61701d8-3dce-51a2-5eaf-14de84425640@huawei.com>
Hi Kefengi 1/4 ?
Could you please try this patch.
Thanks
Yisheng Xie
-------------
^ permalink raw reply
* Re: mm, we use rcu access task_struct in mm_match_cgroup(), but not use rcu free in free_task_struct()
From: Michal Hocko @ 2017-05-24 8:29 UTC (permalink / raw)
To: Xishi Qiu
Cc: Vlastimil Babka, Mel Gorman, Hugh Dickins, Minchan Kim,
wencongyang (A), Johannes Weiner, Dmitry Vyukov, zhong jiang,
Linux MM, LKML
In-Reply-To: <59253E84.6010506@huawei.com>
On Wed 24-05-17 16:04:20, Xishi Qiu wrote:
> On 2017/5/24 15:49, Vlastimil Babka wrote:
>
> > On 05/24/2017 06:40 AM, Xishi Qiu wrote:
> >> On 2017/5/24 9:40, Xishi Qiu wrote:
> >>
> >>> Hi, I find we use rcu access task_struct in mm_match_cgroup(), but not use
> >>> rcu free in free_task_struct(), is it right?
> >>>
> >>> Here is the backtrace.
> >
> > Can you post the whole oops, including kernel version etc? Is it the
> > same 3.10 RH kernel as in the other report?
> >
>
> Hi Vlastimil,
>
> Yes, it's RHEL 7.2
Please contact Redhat for the support of this kernel. Feel free to
report the issue if it persists with the current vanilla kernel. It is
really hard for us to help you with a custom patched and an old kernel.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [-next] memory hotplug regression
From: Heiko Carstens @ 2017-05-24 8:20 UTC (permalink / raw)
To: Michal Hocko; +Cc: Gerald Schaefer, linux-mm, linux-kernel
Hello Michal,
I just re-tested linux-next with respect to your memory hotplug changes and
actually (finally) figured out that your patch ("mm, memory_hotplug: do not
associate hotadded memory to zones until online)" changes behaviour on
s390:
before your patch memory blocks that were offline and located behind the
last online memory block were added by default to ZONE_MOVABLE:
# cat /sys/devices/system/memory/memory16/valid_zones
Movable Normal
With your patch this changes, so that they will be added to ZONE_NORMAL by
default instead:
# cat /sys/devices/system/memory/memory16/valid_zones
Normal Movable
Sorry, that I didn't realize this earlier!
Having the ZONE_MOVABLE default was actually the only point why s390's
arch_add_memory() was rather complex compared to other architectures.
We always had this behaviour, since we always wanted to be able to offline
memory after it was brought online. Given that back then "online_movable"
did not exist, the initial s390 memory hotplug support simply added all
additional memory to ZONE_MOVABLE.
Keeping the default the same would be quite important.
FWIW, and a bit unrelated: we had/have very basic lsmem and chmem tools
which can be used to list memory states and bring memory online and
offline. These tools were part of the s390-tools package and only recently
moved to util-linux.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: mm, we use rcu access task_struct in mm_match_cgroup(), but not use rcu free in free_task_struct()
From: Xishi Qiu @ 2017-05-24 8:04 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Michal Hocko, Mel Gorman, Hugh Dickins, Minchan Kim,
wencongyang (A), Johannes Weiner, Dmitry Vyukov, zhong jiang,
Linux MM, LKML
In-Reply-To: <263518b9-5a39-1af9-ac9e-055da3384aef@suse.cz>
On 2017/5/24 15:49, Vlastimil Babka wrote:
> On 05/24/2017 06:40 AM, Xishi Qiu wrote:
>> On 2017/5/24 9:40, Xishi Qiu wrote:
>>
>>> Hi, I find we use rcu access task_struct in mm_match_cgroup(), but not use
>>> rcu free in free_task_struct(), is it right?
>>>
>>> Here is the backtrace.
>
> Can you post the whole oops, including kernel version etc? Is it the
> same 3.10 RH kernel as in the other report?
>
Hi Vlastimil,
Yes, it's RHEL 7.2
[ 663.687410] Modules linked in: dm_service_time dm_multipath iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi ocfs2_lockfs(OE) ocfs2(OE) ocfs2_adl(OE) jbd2 ocfs2_stack_o2cb(OE) ocfs2_dlm(OE) ocfs2_nodemanager(OE) ocfs2_stackglue(OE) kboxdriver(O) kbox(O) 8021q garp stp mrp llc dev_connlimit(O) vhba(OE) signo_catch(O) hotpatch(OE) bum(O) ip_set nfnetlink prio(O) nat(O) vport_vxlan(O) openvswitch(O) nf_defrag_ipv6 gre ipmi_devintf pmcint(O) ipmi_si ipmi_msghandler coretemp iTCO_wdt kvm_intel(O) iTCO_vendor_support intel_rapl kvm(O) crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel lrw gf128mul glue_helper mei_me ablk_helper cryptd i2c_i801 be2net mei lpc_ich i2c_core vxlan sb_edac pcspkr mfd_core edac_core sg ip6_udp_tunnel udp_tunnel shpchp acpi_power_meter remote_trigger(O) nf_conntrack_ipv4
[ 663.759402] nf_defrag_ipv4 vhost_net(O) tun(O) vhost(O) macvtap macvlan vfio_pci irqbypass vfio_iommu_type1 vfio xt_sctp nf_conntrack_proto_sctp nf_nat_proto_sctp nf_nat nf_conntrack sctp libcrc32c ip_tables ext3 mbcache jbd dm_mod sd_mod lpfc crc_t10dif ahci crct10dif_generic libahci crct10dif_pclmul mpt2sas scsi_transport_fc libata raid_class scsi_tgt crct10dif_common scsi_transport_sas nbd(OE) [last unloaded: kbox]
[ 663.796117] CPU: 2 PID: 2133 Comm: CPU 15/KVM Tainted: G OE ---- ------- 3.10.0-327.49.58.52_13.x86_64 #1
[ 663.807080] Hardware name: Huawei CH121 V3/IT11SGCA1, BIOS 1.51 06/11/2015
[ 663.814107] task: ffff881fe3353300 ti: ffff881fe2768000 task.ti: ffff881fe2768000
[ 663.821854] RIP: 0010:[<ffffffff811db536>] [<ffffffff811db536>] mem_cgroup_from_task+0x16/0x20
[ 663.830895] RSP: 0000:ffff881fe276b810 EFLAGS: 00010286
[ 663.836333] RAX: 6b6b6b6b6b6b6b6b RBX: ffffea007f988880 RCX: 0000000000020000
[ 663.843621] RDX: 00000007fa607d67 RSI: 00000007fa607d67 RDI: ffff880fe36d72c0
[ 663.850878] RBP: ffff881fe276b880 R08: 00000007fa607600 R09: a801fd67b3000000
[ 663.858164] R10: 57fdec98cc59ecc0 R11: ffff880fe2e8dbd0 R12: ffffc9001cb74000
[ 663.865420] R13: ffff881fdb8cfda0 R14: ffff881fe2581570 R15: 00000007fa607d67
[ 663.872696] FS: 00007fdaaba6d700(0000) GS:ffff88103fc80000(0000) knlGS:0000000000000000
[ 663.881092] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 663.886996] CR2: 00007fdaba65cf88 CR3: 0000000fe4919000 CR4: 00000000001427e0
[ 663.894252] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 663.901533] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 663.908788] Stack:
[ 663.910966] ffffffff811a6b8a ffff881fd828d980 00000007fa607d67 ffff881fe276b950
[ 663.918753] 0000000000000000 00000007fa607600 00007fa607600000 000000017f59ece0
[ 663.926541] 00000000675ac78b ffff881fe276bc70 ffffea007f9888a0 ffff881fe276ba40
[ 663.934324] ffffea007f988880 0000000000000001 ffff881fe276b9b8 ffffffff81180994
[ 663.942113] ffff881fe3353300 ffff881fe3353300 ffff881fe3353300 0000000000000000
[ 663.949903] ffff881fe276ba38 ffff881fe276ba30 ffff881fe276ba20 ffff881fe276ba28
[ 663.957691] ffff881fe276ba18 0000000000000000 ffff88207efd6000 0000000000000000
[ 663.965468] ffff881fe3353300 0000000000000000 00ff881000000008 0000000000000000
[ 663.973261] 0000000000000000 0000000000000000 0000000000000000 0000881fe5400410
[ 663.980994] ffffea007f59ece0 ffffea007f691ca0 ffff881fe276b940 ffff881fe276b940
[ 663.988773] 0000000000000000 ffff881fe276b9b8 ffff881fe276bca0 ffff881fe276ba10
[ 663.996563] 0000000000000001 ffff881fe276ba40 ffff881fe5400410 00000000675ac78b
[ 664.004359] ffff881fe276ba40 0000000000000000 ffff881fe276bc70 0000000000000020
[ 664.012139] ffff88207efd6000 ffff881fe276ba80 ffffffff8118166a ffff881fe276ba20
[ 664.019926] ffff881fe276ba30 ffff881fe276ba38 0000001700000000 0000000000000016
[ 664.027718] ffff88207efd6540 00000003ffffffe0 0000000000400410 ffff881fe5400410
[ 664.035497] 0000000000000020 0000000000000000 0000000000000000 0000000000000000
[ 664.043284] 0000000000000000 0000000000000000 ffffea007f625aa0 ffffea007f9888e0
[ 664.051073] 00000000675ac78b 0000000000000000 0000000000000020 ffff881fe5400410
[ 664.058870] 0000000000000020 0000000000000000 ffff881fe276bb80 ffffffff81182135
[ 664.066629] 0000000000011628 ffff881fe276baa8 ffffffff00000003 ffff881fe276ba00
[ 664.074419] 0000000000000020 0000000000000000 ffff881fe276bc70 00000000000002dd
[ 664.082149] 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 664.089933] 000000000000045d 0000000000000000 0000000000000000 0000000000000001
[ 664.097726] Call Trace:
[ 664.100341] [<ffffffff811a6b8a>] ? page_referenced+0x24a/0x350
[ 664.106417] [<ffffffff81180994>] shrink_page_list+0x4b4/0xad0
[ 664.112412] [<ffffffff8118166a>] shrink_inactive_list+0x1ea/0x560
[ 664.118746] [<ffffffff81182135>] shrink_lruvec+0x375/0x760
[ 664.124448] [<ffffffff81182596>] shrink_zone+0x76/0x1a0
[ 664.129911] [<ffffffff81182a90>] do_try_to_free_pages+0xe0/0x3f0
[ 664.136161] [<ffffffff810b4be8>] ? finish_task_switch+0xd8/0x170
[ 664.142410] [<ffffffff81649957>] ? __schedule+0x2b7/0x790
[ 664.148047] [<ffffffff81182fea>] try_to_free_mem_cgroup_pages+0xca/0x160
[ 664.154978] [<ffffffff811dd8de>] mem_cgroup_reclaim+0x4e/0xe0
[ 664.160961] [<ffffffff811ddd9c>] __mem_cgroup_try_charge+0x42c/0x650
[ 664.167526] [<ffffffff811e1295>] ? swap_cgroup_record+0x55/0x80
[ 664.173692] [<ffffffff811df62b>] __mem_cgroup_try_charge_swapin+0x9b/0xd0
[ 664.180719] [<ffffffff8116bd5e>] ? __find_get_page+0x1e/0xa0
[ 664.186620] [<ffffffff811e0537>] mem_cgroup_try_charge_swapin+0x57/0x70
[ 664.193477] [<ffffffff8119abdd>] handle_mm_fault+0x82d/0xf50
[ 664.199381] [<ffffffff816502d6>] __do_page_fault+0x166/0x470
[ 664.205289] [<ffffffff81650603>] do_page_fault+0x23/0x80
[ 664.210813] [<ffffffff811fdc3b>] ? SyS_ioctl+0x8b/0xc0
[ 664.216196] [<ffffffff8164c808>] page_fault+0x28/0x30
[ 664.221483] Code: 00 00 00 00 00 0f 1f 44 00 00 55 48 8b 47 70 48 89 e5 5d c3 90 0f 1f 44 00 00 55 48 85 ff 48 89 e5 74 0d 48 8b 87 40 09 00 00 5d <48> 8b 40 50 c3 31 c0 5d c3 90 0f 1f 44 00 00 55 48 85 ff 48 89
[ 664.242054] RIP [<ffffffff811db536>] mem_cgroup_from_task+0x16/0x20
[ 664.248581] RSP <ffff881fe276b810>
[ 664.252746] ---[ end trace dac00ad920bb710a ]---
>
> .
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2 04/11] x86/mm: Pass flush_tlb_info to flush_tlb_others() etc
From: Ingo Molnar @ 2017-05-24 8:18 UTC (permalink / raw)
To: Andy Lutomirski
Cc: X86 ML, linux-kernel@vger.kernel.org, Borislav Petkov,
Linus Torvalds, Andrew Morton, Mel Gorman, linux-mm@kvack.org,
Nadav Amit, Rik van Riel, Dave Hansen, Nadav Amit, Michal Hocko
In-Reply-To: <3aa98b1199bdcc258706bc8084135b51b52f1ece.1495492063.git.luto@kernel.org>
* Andy Lutomirski <luto@kernel.org> wrote:
> Rather than passing all the contents of flush_tlb_info to
> flush_tlb_others(), pass a pointer to the structure directly. For
> consistency, this also removes the unnecessary cpu parameter from
> uv_flush_tlb_others() to make its signature match the other
> *flush_tlb_others() functions.
>
> This serves two purposes:
>
> - It will dramatically simplify future patches that change struct
> flush_tlb_info, which I'm planning to do.
>
> - struct flush_tlb_info is an adequate description of what to do
> for a local flush, too, so by reusing it we can remove duplicated
> code between local and remove flushes in a future patch.
>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Nadav Amit <namit@vmware.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> arch/x86/include/asm/paravirt.h | 6 ++--
> arch/x86/include/asm/paravirt_types.h | 5 ++-
> arch/x86/include/asm/tlbflush.h | 19 ++++++-----
> arch/x86/include/asm/uv/uv.h | 9 ++---
> arch/x86/mm/tlb.c | 64 +++++++++++++++++------------------
> arch/x86/platform/uv/tlb_uv.c | 10 +++---
> arch/x86/xen/mmu.c | 10 +++---
> 7 files changed, 59 insertions(+), 64 deletions(-)
I've picked up the first three patches, but this patch apparently clashes with
v4.12 changes:
patching file arch/x86/platform/uv/tlb_uv.c
Hunk #1 FAILED at 1109.
Hunk #2 FAILED at 1170.
2 out of 2 hunks FAILED -- rejects in file arch/x86/platform/uv/tlb_uv.c
patching file arch/x86/xen/mmu.c
Hunk #1 FAILED at 1427.
Hunk #2 FAILED at 1440.
Hunk #3 FAILED at 1454.
3 out of 3 hunks FAILED -- rejects in file arch/x86/xen/mmu.c
Patch 10 is broken and patch 11 needs an Ack from the KVM guys so I'll wait for a
new series on top of the new x86/mm branch. (Which I'll push out once it passes
testing.)
Thanks,
Ingo
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm/zsmalloc: fix -Wunneeded-internal-declaration warning
From: Sergey Senozhatsky @ 2017-05-24 8:16 UTC (permalink / raw)
To: Nick Desaulniers
Cc: md, mka, Minchan Kim, Nitin Gupta, Sergey Senozhatsky, linux-mm,
linux-kernel
In-Reply-To: <20170524053859.29059-1-nick.desaulniers@gmail.com>
On (05/23/17 22:38), Nick Desaulniers wrote:
>
> is_first_page() is only called from the macro VM_BUG_ON_PAGE() which is
> only compiled in as a runtime check when CONFIG_DEBUG_VM is set,
> otherwise is checked at compile time and not actually compiled in.
>
> Fixes the following warning, found with Clang:
>
> mm/zsmalloc.c:472:12: warning: function 'is_first_page' is not needed and
> will not be emitted [-Wunneeded-internal-declaration]
> static int is_first_page(struct page *page)
> ^
>
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
well, no objections from my side. MM seems to be getting more and
more `__maybe_unused' annotations because of clang.
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE
From: Vlastimil Babka @ 2017-05-24 7:58 UTC (permalink / raw)
To: Mike Rapoport
Cc: Michal Hocko, Kirill A. Shutemov, Andrew Morton, Arnd Bergmann,
Kirill A. Shutemov, Andrea Arcangeli, Pavel Emelyanov, linux-mm,
lkml, Linux API
In-Reply-To: <20170524075043.GB3063@rapoport-lnx>
On 05/24/2017 09:50 AM, Mike Rapoport wrote:
> On Mon, May 22, 2017 at 05:52:47PM +0200, Vlastimil Babka wrote:
>> On 05/22/2017 04:29 PM, Mike Rapoport wrote:
>>> On Mon, May 22, 2017 at 03:55:48PM +0200, Michal Hocko wrote:
>>>> On Mon 22-05-17 16:36:00, Mike Rapoport wrote:
>>>>> On Mon, May 22, 2017 at 02:42:43PM +0300, Kirill A. Shutemov wrote:
>>>>>> On Mon, May 22, 2017 at 09:12:42AM +0300, Mike Rapoport wrote:
>>>>>>> Currently applications can explicitly enable or disable THP for a memory
>>>>>>> region using MADV_HUGEPAGE or MADV_NOHUGEPAGE. However, once either of
>>>>>>> these advises is used, the region will always have
>>>>>>> VM_HUGEPAGE/VM_NOHUGEPAGE flag set in vma->vm_flags.
>>>>>>> The MADV_CLR_HUGEPAGE resets both these flags and allows managing THP in
>>>>>>> the region according to system-wide settings.
>>>>>>
>>>>>> Seems reasonable. But could you describe an use-case when it's useful in
>>>>>> real world.
>>>>>
>>>>> My use-case was combination of pre- and post-copy migration of containers
>>>>> with CRIU.
>>>>> In this case we populate a part of a memory region with data that was saved
>>>>> during the pre-copy stage. Afterwards, the region is registered with
>>>>> userfaultfd and we expect to get page faults for the parts of the region
>>>>> that were not yet populated. However, khugepaged collapses the pages and
>>>>> the page faults we would expect do not occur.
>>>>
>>>> I am not sure I undestand the problem. Do I get it right that the
>>>> khugepaged will effectivelly corrupt the memory by collapsing a range
>>>> which is not yet fully populated? If yes shouldn't that be fixed in
>>>> khugepaged rather than adding yet another madvise command? Also how do
>>>> you prevent on races? (say you VM_NOHUGEPAGE, khugepaged would be in the
>>>> middle of the operation and sees a collapsable vma and you get the same
>>>> result)
>>>
>>> Probably I didn't explained it too well.
>>>
>>> The range is intentionally not populated. When we combine pre- and
>>> post-copy for process migration, we create memory pre-dump without stopping
>>> the process, then we freeze the process without dumping the pages it has
>>> dirtied between pre-dump and freeze, and then, during restore, we populate
>>> the dirtied pages using userfaultfd.
>>>
>>> When CRIU restores a process in such scenario, it does something like:
>>>
>>> * mmap() memory region
>>> * fill in the pages that were collected during the pre-dump
>>> * do some other stuff
>>> * register memory region with userfaultfd
>>> * populate the missing memory on demand
>>>
>>> khugepaged collapses the pages in the partially populated regions before we
>>> have a chance to register these regions with userfaultfd, which would
>>> prevent the collapse.
>>>
>>> We could have used MADV_NOHUGEPAGE right after the mmap() call, and then
>>> there would be no race because there would be nothing for khugepaged to
>>> collapse at that point. But the problem is that we have no way to reset
>>> *HUGEPAGE flags after the memory restore is complete.
>>
>> Hmm, I wouldn't be that sure if this is indeed race-free. Check that
>> this scenario is indeed impossible?
>>
>> - you do the mmap
>> - khugepaged will choose the process' mm to scan
>> - khugepaged will get to the vma in question, it doesn't have
>> MADV_NOHUGEPAGE yet
>> - you set MADV_NOHUGEPAGE on the vma
>> - you start populating the vma
>> - khugepaged sees the vma is non-empty, collapses
>>
>> unless I'm wrong, the racers will have mmap_sem for reading only when
>> setting/checking the MADV_NOHUGEPAGE? Might be actually considered a bug.
>>
>> However, can't you use prctl(PR_SET_THP_DISABLE) instead? "If arg2 has a
>> nonzero value, the flag is set, otherwise it is cleared." says the
>> manpage. Do it before the mmap and you avoid the race as well?
>
> Unfortunately, prctl(PR_SET_THP_DISABLE) didn't help :(
> When I've tried to use it, I've ended up with VM_NOHUGEPAGE set on all VMAs
> created after prctl(). This returns me to the state when checkpoint-restore
> alters the application vma->vm_flags although it shouldn't and I do not see
> a way to fix it using existing interfaces.
[CC linux-api, should have been done in the initial posting already]
Hm so the prctl does:
if (arg2)
me->mm->def_flags |= VM_NOHUGEPAGE;
else
me->mm->def_flags &= ~VM_NOHUGEPAGE;
That's rather lazy implementation IMHO. Could we change it so the flag
is stored elsewhere in the mm, and the code that decides to (not) use
THP will check both the per-vma flag and the per-mm flag?
> --
> Sincerely yours,
> Mike.
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE
From: Mike Rapoport @ 2017-05-24 7:50 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Michal Hocko, Kirill A. Shutemov, Andrew Morton, Arnd Bergmann,
Kirill A. Shutemov, Andrea Arcangeli, Pavel Emelyanov, linux-mm,
lkml
In-Reply-To: <a9e74c22-1a07-f49a-42b5-497fee85e9c9@suse.cz>
On Mon, May 22, 2017 at 05:52:47PM +0200, Vlastimil Babka wrote:
> On 05/22/2017 04:29 PM, Mike Rapoport wrote:
> > On Mon, May 22, 2017 at 03:55:48PM +0200, Michal Hocko wrote:
> >> On Mon 22-05-17 16:36:00, Mike Rapoport wrote:
> >>> On Mon, May 22, 2017 at 02:42:43PM +0300, Kirill A. Shutemov wrote:
> >>>> On Mon, May 22, 2017 at 09:12:42AM +0300, Mike Rapoport wrote:
> >>>>> Currently applications can explicitly enable or disable THP for a memory
> >>>>> region using MADV_HUGEPAGE or MADV_NOHUGEPAGE. However, once either of
> >>>>> these advises is used, the region will always have
> >>>>> VM_HUGEPAGE/VM_NOHUGEPAGE flag set in vma->vm_flags.
> >>>>> The MADV_CLR_HUGEPAGE resets both these flags and allows managing THP in
> >>>>> the region according to system-wide settings.
> >>>>
> >>>> Seems reasonable. But could you describe an use-case when it's useful in
> >>>> real world.
> >>>
> >>> My use-case was combination of pre- and post-copy migration of containers
> >>> with CRIU.
> >>> In this case we populate a part of a memory region with data that was saved
> >>> during the pre-copy stage. Afterwards, the region is registered with
> >>> userfaultfd and we expect to get page faults for the parts of the region
> >>> that were not yet populated. However, khugepaged collapses the pages and
> >>> the page faults we would expect do not occur.
> >>
> >> I am not sure I undestand the problem. Do I get it right that the
> >> khugepaged will effectivelly corrupt the memory by collapsing a range
> >> which is not yet fully populated? If yes shouldn't that be fixed in
> >> khugepaged rather than adding yet another madvise command? Also how do
> >> you prevent on races? (say you VM_NOHUGEPAGE, khugepaged would be in the
> >> middle of the operation and sees a collapsable vma and you get the same
> >> result)
> >
> > Probably I didn't explained it too well.
> >
> > The range is intentionally not populated. When we combine pre- and
> > post-copy for process migration, we create memory pre-dump without stopping
> > the process, then we freeze the process without dumping the pages it has
> > dirtied between pre-dump and freeze, and then, during restore, we populate
> > the dirtied pages using userfaultfd.
> >
> > When CRIU restores a process in such scenario, it does something like:
> >
> > * mmap() memory region
> > * fill in the pages that were collected during the pre-dump
> > * do some other stuff
> > * register memory region with userfaultfd
> > * populate the missing memory on demand
> >
> > khugepaged collapses the pages in the partially populated regions before we
> > have a chance to register these regions with userfaultfd, which would
> > prevent the collapse.
> >
> > We could have used MADV_NOHUGEPAGE right after the mmap() call, and then
> > there would be no race because there would be nothing for khugepaged to
> > collapse at that point. But the problem is that we have no way to reset
> > *HUGEPAGE flags after the memory restore is complete.
>
> Hmm, I wouldn't be that sure if this is indeed race-free. Check that
> this scenario is indeed impossible?
>
> - you do the mmap
> - khugepaged will choose the process' mm to scan
> - khugepaged will get to the vma in question, it doesn't have
> MADV_NOHUGEPAGE yet
> - you set MADV_NOHUGEPAGE on the vma
> - you start populating the vma
> - khugepaged sees the vma is non-empty, collapses
>
> unless I'm wrong, the racers will have mmap_sem for reading only when
> setting/checking the MADV_NOHUGEPAGE? Might be actually considered a bug.
>
> However, can't you use prctl(PR_SET_THP_DISABLE) instead? "If arg2 has a
> nonzero value, the flag is set, otherwise it is cleared." says the
> manpage. Do it before the mmap and you avoid the race as well?
Unfortunately, prctl(PR_SET_THP_DISABLE) didn't help :(
When I've tried to use it, I've ended up with VM_NOHUGEPAGE set on all VMAs
created after prctl(). This returns me to the state when checkpoint-restore
alters the application vma->vm_flags although it shouldn't and I do not see
a way to fix it using existing interfaces.
--
Sincerely yours,
Mike.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: mm, we use rcu access task_struct in mm_match_cgroup(), but not use rcu free in free_task_struct()
From: Vlastimil Babka @ 2017-05-24 7:49 UTC (permalink / raw)
To: Xishi Qiu, Michal Hocko, Mel Gorman, Hugh Dickins, Minchan Kim,
wencongyang (A), Johannes Weiner, Dmitry Vyukov, zhong jiang
Cc: Linux MM, LKML
In-Reply-To: <59250EA3.60905@huawei.com>
On 05/24/2017 06:40 AM, Xishi Qiu wrote:
> On 2017/5/24 9:40, Xishi Qiu wrote:
>
>> Hi, I find we use rcu access task_struct in mm_match_cgroup(), but not use
>> rcu free in free_task_struct(), is it right?
>>
>> Here is the backtrace.
Can you post the whole oops, including kernel version etc? Is it the
same 3.10 RH kernel as in the other report?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v1 00/11] mm/kasan: support per-page shadow memory to reduce memory consumption
From: Joonsoo Kim @ 2017-05-24 7:45 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrew Morton, Andrey Ryabinin, Alexander Potapenko, kasan-dev,
linux-mm@kvack.org, LKML, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, kernel-team
In-Reply-To: <CACT4Y+YREmHViSMsH84bwtEqbUsqsgzaa76eWzJXqmSgqKbgvg@mail.gmail.com>
On Wed, May 24, 2017 at 08:57:23AM +0200, Dmitry Vyukov wrote:
> On Tue, May 16, 2017 at 10:49 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > On Mon, May 15, 2017 at 11:23 PM, Joonsoo Kim <js1304@gmail.com> wrote:
> >>> >
> >>> > Hello, all.
> >>> >
> >>> > This is an attempt to recude memory consumption of KASAN. Please see
> >>> > following description to get the more information.
> >>> >
> >>> > 1. What is per-page shadow memory
> >>>
> >>> Hi Joonsoo,
> >>
> >> Hello, Dmitry.
> >>
> >>>
> >>> First I need to say that this is great work. I wanted KASAN to consume
> >>
> >> Thanks!
> >>
> >>> 1/8-th of _kernel_ memory rather than total physical memory for a long
> >>> time.
> >>>
> >>> However, this implementation does not work inline instrumentation. And
> >>> the inline instrumentation is the main mode for KASAN. Outline
> >>> instrumentation is merely a rudiment to support gcc 4.9, and it needs
> >>> to be removed as soon as we stop caring about gcc 4.9 (do we at all?
> >>> is it the current compiler in any distro? Ubuntu 12 has 4.8, Ubuntu 14
> >>> already has 5.4. And if you build gcc yourself or get a fresher
> >>> compiler from somewhere else, you hopefully get something better than
> >>> 4.9).
> >>
> >> Hmm... I don't think that outline instrumentation is something to be
> >> removed. In embedded world, there is a fixed partition table and
> >> enlarging the kernel binary would cause the problem. Changing that
> >> table is possible but is really uncomfortable thing for debugging
> >> something. So, I think that outline instrumentation has it's own merit.
> >
> > Fair. Let's consider both as important.
> >
> >> Anyway, I have missed inline instrumentation completely.
> >>
> >> I will attach the fix in the bottom. It doesn't look beautiful
> >> since it breaks layer design (some check will be done at report
> >> function). However, I think that it's a good trade-off.
> >
> >
> > I can confirm that inline works with that patch.
> >
> > I can also confirm that it reduces memory usage. I've booted qemu with
> > 2G ram and run some fixed workload. Before:
> > 31853 dvyukov 20 0 3043200 765464 21312 S 366.0 4.7 2:39.53
> > qemu-system-x86
> > 7528 dvyukov 20 0 3043200 732444 21676 S 333.3 4.5 2:23.19
> > qemu-system-x86
> > After:
> > 6192 dvyukov 20 0 3043200 394244 20636 S 17.9 2.4 2:32.95
> > qemu-system-x86
> > 6265 dvyukov 20 0 3043200 388860 21416 S 399.3 2.4 3:02.88
> > qemu-system-x86
> > 9005 dvyukov 20 0 3043200 383564 21220 S 397.1 2.3 2:35.33
> > qemu-system-x86
> >
> > However, I see some very significant slowdowns with inline
> > instrumentation. I did 3 tests:
> > 1. Boot speed, I measured time for a particular message to appear on
> > console. Before:
> > [ 2.504652] random: crng init done
> > [ 2.435861] random: crng init done
> > [ 2.537135] random: crng init done
> > After:
> > [ 7.263402] random: crng init done
> > [ 7.263402] random: crng init done
> > [ 7.174395] random: crng init done
> >
> > That's ~3x slowdown.
> >
> > 2. I've run bench_readv benchmark:
> > https://raw.githubusercontent.com/google/sanitizers/master/address-sanitizer/kernel_buildbot/slave/bench_readv.c
> > as:
> > while true; do time ./bench_readv bench_readv 300000 1; done
> >
> > Before:
> > sys 0m7.299s
> > sys 0m7.218s
> > sys 0m6.973s
> > sys 0m6.892s
> > sys 0m7.035s
> > sys 0m6.982s
> > sys 0m6.921s
> > sys 0m6.940s
> > sys 0m6.905s
> > sys 0m7.006s
> >
> > After:
> > sys 0m8.141s
> > sys 0m8.077s
> > sys 0m8.067s
> > sys 0m8.116s
> > sys 0m8.128s
> > sys 0m8.115s
> > sys 0m8.108s
> > sys 0m8.326s
> > sys 0m8.529s
> > sys 0m8.164s
> > sys 0m8.380s
> >
> > This is ~19% slowdown.
> >
> > 3. I've run bench_pipes benchmark:
> > https://raw.githubusercontent.com/google/sanitizers/master/address-sanitizer/kernel_buildbot/slave/bench_pipes.c
> > as:
> > while true; do time ./bench_pipes 10 10000 1; done
> >
> > Before:
> > sys 0m5.393s
> > sys 0m6.178s
> > sys 0m5.909s
> > sys 0m6.024s
> > sys 0m5.874s
> > sys 0m5.737s
> > sys 0m5.826s
> > sys 0m5.664s
> > sys 0m5.758s
> > sys 0m5.421s
> > sys 0m5.444s
> > sys 0m5.479s
> > sys 0m5.461s
> > sys 0m5.417s
> >
> > After:
> > sys 0m8.718s
> > sys 0m8.281s
> > sys 0m8.268s
> > sys 0m8.334s
> > sys 0m8.246s
> > sys 0m8.267s
> > sys 0m8.265s
> > sys 0m8.437s
> > sys 0m8.228s
> > sys 0m8.312s
> > sys 0m8.556s
> > sys 0m8.680s
> >
> > This is ~52% slowdown.
> >
> >
> > This does not look acceptable to me. I would ready to pay for this,
> > say, 10% of performance. But it seems that this can have up to 2-4x
> > slowdown for some workloads.
> >
> >
> > Your use-case is embed devices where you care a lot about both code
> > size and memory consumption, right?
> >
> > I see 2 possible ways forward:
> > 1. Enable this new mode only for outline, but keep current scheme for
> > inline. Then outline will be "small but slow" type of configuration.
> > 2. Somehow fix slowness (at least in inline mode).
> >
> >
> >> Mapping zero page to non-kernel memory could cause true-negative
> >> problem since we cannot flush the TLB in all cpus. We will read zero
> >> shadow value value in this case even if actual shadow value is not
> >> zero. This is one of the reason that black page is introduced in this
> >> patchset.
> >
> > What does make your current patch work then?
> > Say we map a new shadow page, update the page shadow to say that there
> > is mapped shadow. Then another CPU loads the page shadow and then
> > loads from the newly mapped shadow. If we don't flush TLB, what makes
> > the second CPU see the newly mapped shadow?
>
> /\/\/\/\/\/\
>
> Joonsoo, please answer this question above.
Hello, I've answered it in another e-mail however it would not be
sufficient. I try again.
If the page isn't used for kernel stack, slab, and global variable
(aka. kernel memory), black shadow is mapped for the page. We map a
new shadow page if the page will be used for kernel memory. We need to
flush TLB in all cpus when mapping a new shadow however it's not
possible in some cases. So, this patch does just flushing local cpu's
TLB. Another cpu could have stale TLB that points black shadow for
this page. If that cpu with stale TLB try to check vailidity of the
object on this page, result would be invalid since stale TLB points
the black shadow and it's shadow value is non-zero. We need a magic
here. At this moment, we cannot make sure if invalid is correct result
or not since we didn't do full TLB flush. So fixup processing is
started. It is implemented in check_memory_region_slow(). Flushing
local TLB and re-checking the shadow value. With flushing local TLB,
we will use fresh TLB at this time. Therefore, we can pass the
validity check as usual.
> I am trying to understand if there is any chance to make mapping a
> single page for all non-interesting shadow ranges work. That would be
This is what this patchset does. Mapping a single (zero/black) shadow
page for all non-interesting (non-kernel memory) shadow ranges.
There is only single instance of zero/black shadow page. On v1,
I used black shadow page only so fail to get enough performance. On
v2 mentioned in another thread, I use zero shadow for some region. I
guess that performance problem would be gone.
> much simpler change that does not require changing instrumentation,
Yes! I think that it is really good benefit of this patchset.
> and will not force inline instrumentation onto slow path for some
> ranges (vmalloc?).
Thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH 0/4 v2] mm: give __GFP_REPEAT a better semantic
From: Michal Hocko @ 2017-05-24 7:34 UTC (permalink / raw)
To: NeilBrown
Cc: Vlastimil Babka, linux-mm, Johannes Weiner, Mel Gorman,
Andrew Morton, LKML, Darrick J. Wong, Heiko Carstens, NeilBrown,
Jonathan Corbet, Paolo Bonzini, Eric W. Biederman
In-Reply-To: <87shjvhxmr.fsf@notabene.neil.brown.name>
On Wed 24-05-17 11:06:04, NeilBrown wrote:
> On Tue, May 23 2017, Vlastimil Babka wrote:
>
> > On 05/16/2017 11:10 AM, Michal Hocko wrote:
> >> So, is there some interest in this? I am not going to push this if there
> >> is a general consensus that we do not need to do anything about the
> >> current situation or need a different approach.
> >
> > After the recent LWN article [1] I think that we should really support
> > marking allocations as failable, without making them too easily failable
> > via __GFP_NORETRY. The __GFP_RETRY_MAY_FAIL flag sounds like a good way
> > to do that without introducing a new __GFP_MAYFAIL. We could also
> > introduce a wrapper such as GFP_KERNEL_MAYFAIL.
> >
> > [1] https://lwn.net/Articles/723317/
>
> Yes please!!!
>
> I particularly like:
>
> > - GFP_KERNEL | __GFP_NORETRY - overrides the default allocator behavior and
> > all allocation requests fail early rather than cause disruptive
> > reclaim (one round of reclaim in this implementation). The OOM killer
> > is not invoked.
> > - GFP_KERNEL | __GFP_RETRY_MAYFAIL - overrides the default allocator behavior
> > and all allocation requests try really hard. The request will fail if the
> > reclaim cannot make any progress. The OOM killer won't be triggered.
> > - GFP_KERNEL | __GFP_NOFAIL - overrides the default allocator behavior
> > and all allocation requests will loop endlessly until they
> > succeed. This might be really dangerous especially for larger orders.
>
> There seems to be a good range here, and the two end points are good
> choices.
> I like that only __GFP_NOFAIL triggers the OOM.
> I would like the middle option to be the default. I think that is what
> many people thought the default was. I appreciate that making the
> transition might be awkward.
Yeah, turning GFP_KERNEL int GFP_KERNEL | __GFP_RETRY_MAYFAIL would be
hard if possible at all. One of the problems with the current code is
that error paths are checked but there is rarely a sane error handling
strategy implemented on top. So we mostly check for the failure and
return -ENOMEM up the call chain without having a great clue what will
happen up there. And the result might be really unexpected. Say that
some allocation fails on the sys_close() path and returns to the
userspace. a) this syscall is not supposed to return -ENOMEM b) there is
no _transaction_ rollback to have the fd in a sane state to retry later.
Therefore I assume that __GFP_RETRY_MAYFAIL will be slowly added to
those places where the error path strategy is clear.
> Maybe create GFP_DEFAULT which matches the middle option and encourage
> that in new code??
>
> We would probably want guidelines on when __GFP_NOFAIL is acceptable.
> I assume:
> - no locks held
This is of course preferable but hard to demand in general. I think that
requiring "no locks which can block oom victim exit" would be more
appropriate, albeit much more fuzzy. But in general locks should be much
smaller problem these days with the async OOM reclaim (oom_reaper) and
with __GFP_NOFAIL gaining access to a part of memory reserves when
hitting the OOM path.
> - small allocations OK, large allocation need clear justification.
yes
> - error would be exposed to systemcall
Not only. There are some FS transaction code paths where failure
basically means RO remount and such. This would be acceptable as well.
> ???
>
> I think it is important to give kernel developers clear options and make
> it easy for them to choose the best option. This helps to do that.
Yes, I completely agree here. Does the updated documentation in the
patch helps or would you suggest som improvements?
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v1 00/11] mm/kasan: support per-page shadow memory to reduce memory consumption
From: Dmitry Vyukov @ 2017-05-24 6:57 UTC (permalink / raw)
To: Joonsoo Kim
Cc: Andrew Morton, Andrey Ryabinin, Alexander Potapenko, kasan-dev,
linux-mm@kvack.org, LKML, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, kernel-team
In-Reply-To: <CACT4Y+anOw8=7u-pZ2ceMw0xVnuaO9YKBJAr-2=KOYt_72b2pw@mail.gmail.com>
On Tue, May 16, 2017 at 10:49 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Mon, May 15, 2017 at 11:23 PM, Joonsoo Kim <js1304@gmail.com> wrote:
>>> >
>>> > Hello, all.
>>> >
>>> > This is an attempt to recude memory consumption of KASAN. Please see
>>> > following description to get the more information.
>>> >
>>> > 1. What is per-page shadow memory
>>>
>>> Hi Joonsoo,
>>
>> Hello, Dmitry.
>>
>>>
>>> First I need to say that this is great work. I wanted KASAN to consume
>>
>> Thanks!
>>
>>> 1/8-th of _kernel_ memory rather than total physical memory for a long
>>> time.
>>>
>>> However, this implementation does not work inline instrumentation. And
>>> the inline instrumentation is the main mode for KASAN. Outline
>>> instrumentation is merely a rudiment to support gcc 4.9, and it needs
>>> to be removed as soon as we stop caring about gcc 4.9 (do we at all?
>>> is it the current compiler in any distro? Ubuntu 12 has 4.8, Ubuntu 14
>>> already has 5.4. And if you build gcc yourself or get a fresher
>>> compiler from somewhere else, you hopefully get something better than
>>> 4.9).
>>
>> Hmm... I don't think that outline instrumentation is something to be
>> removed. In embedded world, there is a fixed partition table and
>> enlarging the kernel binary would cause the problem. Changing that
>> table is possible but is really uncomfortable thing for debugging
>> something. So, I think that outline instrumentation has it's own merit.
>
> Fair. Let's consider both as important.
>
>> Anyway, I have missed inline instrumentation completely.
>>
>> I will attach the fix in the bottom. It doesn't look beautiful
>> since it breaks layer design (some check will be done at report
>> function). However, I think that it's a good trade-off.
>
>
> I can confirm that inline works with that patch.
>
> I can also confirm that it reduces memory usage. I've booted qemu with
> 2G ram and run some fixed workload. Before:
> 31853 dvyukov 20 0 3043200 765464 21312 S 366.0 4.7 2:39.53
> qemu-system-x86
> 7528 dvyukov 20 0 3043200 732444 21676 S 333.3 4.5 2:23.19
> qemu-system-x86
> After:
> 6192 dvyukov 20 0 3043200 394244 20636 S 17.9 2.4 2:32.95
> qemu-system-x86
> 6265 dvyukov 20 0 3043200 388860 21416 S 399.3 2.4 3:02.88
> qemu-system-x86
> 9005 dvyukov 20 0 3043200 383564 21220 S 397.1 2.3 2:35.33
> qemu-system-x86
>
> However, I see some very significant slowdowns with inline
> instrumentation. I did 3 tests:
> 1. Boot speed, I measured time for a particular message to appear on
> console. Before:
> [ 2.504652] random: crng init done
> [ 2.435861] random: crng init done
> [ 2.537135] random: crng init done
> After:
> [ 7.263402] random: crng init done
> [ 7.263402] random: crng init done
> [ 7.174395] random: crng init done
>
> That's ~3x slowdown.
>
> 2. I've run bench_readv benchmark:
> https://raw.githubusercontent.com/google/sanitizers/master/address-sanitizer/kernel_buildbot/slave/bench_readv.c
> as:
> while true; do time ./bench_readv bench_readv 300000 1; done
>
> Before:
> sys 0m7.299s
> sys 0m7.218s
> sys 0m6.973s
> sys 0m6.892s
> sys 0m7.035s
> sys 0m6.982s
> sys 0m6.921s
> sys 0m6.940s
> sys 0m6.905s
> sys 0m7.006s
>
> After:
> sys 0m8.141s
> sys 0m8.077s
> sys 0m8.067s
> sys 0m8.116s
> sys 0m8.128s
> sys 0m8.115s
> sys 0m8.108s
> sys 0m8.326s
> sys 0m8.529s
> sys 0m8.164s
> sys 0m8.380s
>
> This is ~19% slowdown.
>
> 3. I've run bench_pipes benchmark:
> https://raw.githubusercontent.com/google/sanitizers/master/address-sanitizer/kernel_buildbot/slave/bench_pipes.c
> as:
> while true; do time ./bench_pipes 10 10000 1; done
>
> Before:
> sys 0m5.393s
> sys 0m6.178s
> sys 0m5.909s
> sys 0m6.024s
> sys 0m5.874s
> sys 0m5.737s
> sys 0m5.826s
> sys 0m5.664s
> sys 0m5.758s
> sys 0m5.421s
> sys 0m5.444s
> sys 0m5.479s
> sys 0m5.461s
> sys 0m5.417s
>
> After:
> sys 0m8.718s
> sys 0m8.281s
> sys 0m8.268s
> sys 0m8.334s
> sys 0m8.246s
> sys 0m8.267s
> sys 0m8.265s
> sys 0m8.437s
> sys 0m8.228s
> sys 0m8.312s
> sys 0m8.556s
> sys 0m8.680s
>
> This is ~52% slowdown.
>
>
> This does not look acceptable to me. I would ready to pay for this,
> say, 10% of performance. But it seems that this can have up to 2-4x
> slowdown for some workloads.
>
>
> Your use-case is embed devices where you care a lot about both code
> size and memory consumption, right?
>
> I see 2 possible ways forward:
> 1. Enable this new mode only for outline, but keep current scheme for
> inline. Then outline will be "small but slow" type of configuration.
> 2. Somehow fix slowness (at least in inline mode).
>
>
>> Mapping zero page to non-kernel memory could cause true-negative
>> problem since we cannot flush the TLB in all cpus. We will read zero
>> shadow value value in this case even if actual shadow value is not
>> zero. This is one of the reason that black page is introduced in this
>> patchset.
>
> What does make your current patch work then?
> Say we map a new shadow page, update the page shadow to say that there
> is mapped shadow. Then another CPU loads the page shadow and then
> loads from the newly mapped shadow. If we don't flush TLB, what makes
> the second CPU see the newly mapped shadow?
/\/\/\/\/\/\
Joonsoo, please answer this question above.
I am trying to understand if there is any chance to make mapping a
single page for all non-interesting shadow ranges work. That would be
much simpler change that does not require changing instrumentation,
and will not force inline instrumentation onto slow path for some
ranges (vmalloc?).
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm: Define KB, MB, GB, TB in core VM
From: Anshuman Khandual @ 2017-05-24 6:40 UTC (permalink / raw)
To: Anshuman Khandual, Vlastimil Babka, Christoph Hellwig,
Andrew Morton
Cc: linux-mm, linux-kernel
In-Reply-To: <161638da-3b2b-7912-2ae2-3b2936ca1537@linux.vnet.ibm.com>
On 05/23/2017 04:49 PM, Anshuman Khandual wrote:
> On 05/23/2017 02:08 PM, Vlastimil Babka wrote:
>> On 05/23/2017 09:02 AM, Christoph Hellwig wrote:
>>> On Mon, May 22, 2017 at 02:11:49PM -0700, Andrew Morton wrote:
>>>> On Mon, 22 May 2017 16:47:42 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
>>>>
>>>>> There are many places where we define size either left shifting integers
>>>>> or multiplying 1024s without any generic definition to fall back on. But
>>>>> there are couples of (powerpc and lz4) attempts to define these standard
>>>>> memory sizes. Lets move these definitions to core VM to make sure that
>>>>> all new usage come from these definitions eventually standardizing it
>>>>> across all places.
>>>> Grep further - there are many more definitions and some may now
>>>> generate warnings.
>>>>
>>>> Newly including mm.h for these things seems a bit heavyweight. I can't
>>>> immediately think of a more appropriate place. Maybe printk.h or
>>>> kernel.h.
>>> IFF we do these kernel.h is the right place. And please also add the
>>> MiB & co variants for the binary versions right next to the decimal
>>> ones.
>> Those defined in the patch are binary, not decimal. Do we even need
>> decimal ones?
>>
>
> I can define KiB, MiB, .... with the same values as binary.
> Did not get about the decimal ones, we need different names
> for them holding values which are multiple of 1024 ?
Now it seems little bit complicated than I initially thought.
There are three different kind of definitions scattered across
the tree.
(1) Constant defines like these which can be unified across
with little effort.
+#define KB (1UL << 10)
+#define MB (1UL << 20)
+#define GB (1UL << 30)
+#define TB (1UL << 40)
(2) Function type defines like these which need to be renamed
first because of the static defines already added above.
#define KB(x) ((x) * 1024)
#define MB(x) (KB(x) * 1024)
Does these sound good as a rename ?
+#define KBN(x) ((x) * KB)
+#define MBN(x) ((x) * MB)
+#define GBN(x) ((x) * GB)
+#define TBN(x) ((x) * TB)
And these need to be replaced across the tree.
(3) Then there are many defines for MB, KB, GB which have nothing
to do with memory size and they need to be changed as well to
something else more appropriately to something they actually
do.
#define MB CRB
* Defined inside arch/powerpc/xmon/ppc-opc.c
#define GB(p,n,s) gf2k_get_bits(data, p, n, s)
* Defined inside drivers/input/joystick/gf2k.c
#define GB(pos,num) sw_get_bits(buf, pos, num, sw->bits)
* Defined inside drivers/input/joystick/sidewinder.c
So the question is are we willing to do all these changes across
the tree to achieve common definitions of KB, MB, GB, TB in the
kernel ? Is it worth ?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v1 00/11] mm/kasan: support per-page shadow memory to reduce memory consumption
From: Joonsoo Kim @ 2017-05-24 6:18 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Andrew Morton, Alexander Potapenko, Dmitry Vyukov, kasan-dev,
linux-mm, linux-kernel, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, kernel-team
In-Reply-To: <0c14ea7f-1ae9-5923-8c4c-4f1b2f7dad62@virtuozzo.com>
On Mon, May 22, 2017 at 05:00:29PM +0300, Andrey Ryabinin wrote:
>
>
> On 05/19/2017 04:53 AM, Joonsoo Kim wrote:
> > On Wed, May 17, 2017 at 03:17:13PM +0300, Andrey Ryabinin wrote:
> >> On 05/16/2017 04:16 AM, js1304@gmail.com wrote:
> >>> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >>>
> >>> Hello, all.
> >>>
> >>> This is an attempt to recude memory consumption of KASAN. Please see
> >>> following description to get the more information.
> >>>
> >>> 1. What is per-page shadow memory
> >>>
> >>> This patch introduces infrastructure to support per-page shadow memory.
> >>> Per-page shadow memory is the same with original shadow memory except
> >>> the granualarity. It's one byte shows the shadow value for the page.
> >>> The purpose of introducing this new shadow memory is to save memory
> >>> consumption.
> >>>
> >>> 2. Problem of current approach
> >>>
> >>> Until now, KASAN needs shadow memory for all the range of the memory
> >>> so the amount of statically allocated memory is so large. It causes
> >>> the problem that KASAN cannot run on the system with hard memory
> >>> constraint. Even if KASAN can run, large memory consumption due to
> >>> KASAN changes behaviour of the workload so we cannot validate
> >>> the moment that we want to check.
> >>>
> >>> 3. How does this patch fix the problem
> >>>
> >>> This patch tries to fix the problem by reducing memory consumption for
> >>> the shadow memory. There are two observations.
> >>>
> >>
> >>
> >> I think that the best way to deal with your problem is to increase shadow scale size.
> >>
> >> You'll need to add tunable to gcc to control shadow size. I expect that gcc has some
> >> places where 8-shadow scale size is hardcoded, but it should be fixable.
> >>
> >> The kernel also have some small amount of code written with KASAN_SHADOW_SCALE_SIZE == 8 in mind,
> >> which should be easy to fix.
> >>
> >> Note that bigger shadow scale size requires bigger alignment of allocated memory and variables.
> >> However, according to comments in gcc/asan.c gcc already aligns stack and global variables and at
> >> 32-bytes boundary.
> >> So we could bump shadow scale up to 32 without increasing current stack consumption.
> >>
> >> On a small machine (1Gb) 1/32 of shadow is just 32Mb which is comparable to yours 30Mb, but I expect it to be
> >> much faster. More importantly, this will require only small amount of simple changes in code, which will be
> >> a *lot* more easier to maintain.
> >
> > I agree that it is also a good option to reduce memory consumption.
> > Nevertheless, there are two reasons that justifies this patchset.
> >
> > 1) With this patchset, memory consumption isn't increased in
> > proportional to total memory size. Please consider my 4Gb system
> > example on the below. With increasing shadow scale size to 32, memory
> > would be consumed by 128M. However, this patchset consumed 50MB. This
> > difference can be larger if we run KASAN with bigger machine.
> >
>
> Well, yes, but I assume that bigger machine implies that we can use more memory without
> causing a significant change in system's behavior.
In common case, yes. But, I guess that there is a system that
statically uses most of memory and just a few memory is left for others.
For example, consider 64GB system and some program (DB?) runs with
using 60GB. Only 4GB left. If KASAN uses 2GB, just 2GB is left and it
would cause the problem. So, I'd like to insist that this merit 1)
should be considered as valuable.
>
> > 2) These two optimization can be applied simulatenously. It is just an
> > orthogonal feature. If shadow scale size is increased to 32, memory
> > consumption will be decreased in case of my patchset, too.
> >
> > Therefore, I think that this patchset is useful in any case.
>
> These are valid points, but IMO it's not enough to justify this patchset.
> Too much of hacky and fragile code.
>
> If our goal is to make KASAN to eat less memory, the first step definitely would be a 1/32 shadow.
> Simply because it's the best way to achieve that goal.
> And only if it's not enough we could think about something else, like decreasing/turning off quarantine
> and/or smaller redzones.
Please refer the reply to Dmitry. I think that we need an option that
everything else than something compulsory is the same with non-KASAN
build as much as possible. 1/32 scale would change object layout so it
will not work for this option.
Thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v1 00/11] mm/kasan: support per-page shadow memory to reduce memory consumption
From: Joonsoo Kim @ 2017-05-24 6:04 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Andrew Morton, Alexander Potapenko, kasan-dev,
linux-mm@kvack.org, LKML, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, kernel-team
In-Reply-To: <CACT4Y+bZVJpi++kfMkAc-3pXK165ZQyHaEU_6oN94+qQErJd8A@mail.gmail.com>
On Mon, May 22, 2017 at 08:02:36AM +0200, Dmitry Vyukov wrote:
> On Fri, May 19, 2017 at 3:53 AM, Joonsoo Kim <js1304@gmail.com> wrote:
> > On Wed, May 17, 2017 at 03:17:13PM +0300, Andrey Ryabinin wrote:
> >> On 05/16/2017 04:16 AM, js1304@gmail.com wrote:
> >> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >> >
> >> > Hello, all.
> >> >
> >> > This is an attempt to recude memory consumption of KASAN. Please see
> >> > following description to get the more information.
> >> >
> >> > 1. What is per-page shadow memory
> >> >
> >> > This patch introduces infrastructure to support per-page shadow memory.
> >> > Per-page shadow memory is the same with original shadow memory except
> >> > the granualarity. It's one byte shows the shadow value for the page.
> >> > The purpose of introducing this new shadow memory is to save memory
> >> > consumption.
> >> >
> >> > 2. Problem of current approach
> >> >
> >> > Until now, KASAN needs shadow memory for all the range of the memory
> >> > so the amount of statically allocated memory is so large. It causes
> >> > the problem that KASAN cannot run on the system with hard memory
> >> > constraint. Even if KASAN can run, large memory consumption due to
> >> > KASAN changes behaviour of the workload so we cannot validate
> >> > the moment that we want to check.
> >> >
> >> > 3. How does this patch fix the problem
> >> >
> >> > This patch tries to fix the problem by reducing memory consumption for
> >> > the shadow memory. There are two observations.
> >> >
> >>
> >>
> >> I think that the best way to deal with your problem is to increase shadow scale size.
> >>
> >> You'll need to add tunable to gcc to control shadow size. I expect that gcc has some
> >> places where 8-shadow scale size is hardcoded, but it should be fixable.
> >>
> >> The kernel also have some small amount of code written with KASAN_SHADOW_SCALE_SIZE == 8 in mind,
> >> which should be easy to fix.
> >>
> >> Note that bigger shadow scale size requires bigger alignment of allocated memory and variables.
> >> However, according to comments in gcc/asan.c gcc already aligns stack and global variables and at
> >> 32-bytes boundary.
> >> So we could bump shadow scale up to 32 without increasing current stack consumption.
> >>
> >> On a small machine (1Gb) 1/32 of shadow is just 32Mb which is comparable to yours 30Mb, but I expect it to be
> >> much faster. More importantly, this will require only small amount of simple changes in code, which will be
> >> a *lot* more easier to maintain.
>
>
> Interesting option. We never considered increasing scale in user space
> due to performance implications. But the algorithm always supported up
> to 128x scale. Definitely worth considering as an option.
Could you explain me how does increasing scale reduce performance? I
tried to guess the reason but failed.
>
>
> > I agree that it is also a good option to reduce memory consumption.
> > Nevertheless, there are two reasons that justifies this patchset.
> >
> > 1) With this patchset, memory consumption isn't increased in
> > proportional to total memory size. Please consider my 4Gb system
> > example on the below. With increasing shadow scale size to 32, memory
> > would be consumed by 128M. However, this patchset consumed 50MB. This
> > difference can be larger if we run KASAN with bigger machine.
> >
> > 2) These two optimization can be applied simulatenously. It is just an
> > orthogonal feature. If shadow scale size is increased to 32, memory
> > consumption will be decreased in case of my patchset, too.
> >
> > Therefore, I think that this patchset is useful in any case.
>
> It is definitely useful all else being equal. But it does considerably
> increase code size and complexity, which is an important aspect.
>
> Also note that there is also fixed size quarantine (1/32 of RAM) and
> redzones. Reducing shadow overhead beyond some threshold has
> diminishing returns, because overall overhead will be just dominated
> by quarantine/redzones.
My usecase doesn't use quarantine yet since it uses old version kernel
and quarantine isn't back-ported. But, this 1/32 of RAM for quarantine
also could affect the system and I think that we need a switch to
disable it. In our case, making the feature work is more important
than detecting more bugs.
Redzone is also a good target to make selectable since
error pattern could be changed with different object layout. I
sometimes saw that error disappears if KASAN is enabled. I'm not sure
what causes it, but, in some case, it would be helpful that everything
else than something compulsory is the same with non-KASAN build.
> What's your target devices and constraints? We run KASAN on phones
> today without any issues.
My target devices are a smart TV or embedded system on a car. Usually,
these devices have specific use scenario and memory is managed more
tightly than a phone. I have heard that some system with 1GB memory
cannot run if 128MB is used for KASAN. I'm not sure that 1/32 scale
changes the picture, but, yes, I guess that most of problem will disappear.
>
> > Note that increasing shadow scale has it's own trade-off. It requires
> > that the size of slab object is aligned to shadow scale. It will
> > increase memory consumption due to slab.
>
> I've tried to retest your latest change on top of
> http://git.cmpxchg.org/cgit.cgi/linux-mmots.git
> d9cd9c95cc3b2fed0f04d233ebf2f7056741858c, but now this version
> https://codereview.appspot.com/325780043 always crashes during boot
> for me. Report points to zero shadow.
Oops... Maybe, it's due to lack of stale TLB handling on double-free
check in kasan_slab_free(). I fixed it on my version 2 patchset.
And, I also fixed performance problem due to memory allocated by early
allocator(memblock or (no)bootmem).
https://github.com/JoonsooKim/linux/tree/kasan-opt-memory-consumption-v2.0-next-20170511
This branch is based on next-20170511.
Thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox