> -----Original Message----- > From: Boqun Feng [mailto:boqun.feng@xxxxxxxxx] > Sent: Thursday, August 10, 2017 8:59 PM > To: Byungchul Park > Cc: peterz@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; > walken@xxxxxxxxxx; kirill@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > linux-mm@xxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx; willy@xxxxxxxxxxxxx; > npiggin@xxxxxxxxx; kernel-team@xxxxxxx > Subject: Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring > buffer overwrite > > On Mon, Aug 07, 2017 at 04:12:53PM +0900, Byungchul Park wrote: > > 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@xxxxxxx> > > --- > > include/linux/lockdep.h | 20 +++++++++++++++++++ > > include/linux/sched.h | 3 +++ > > kernel/locking/lockdep.c | 52 > +++++++++++++++++++++++++++++++++++++++++++----- > > 3 files changed, 70 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h > > index 0c8a1b8..48c244c 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 5becef5..373466b 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -855,6 +855,9 @@ struct task_struct { > > unsigned int xhlock_idx; > > /* For restoring at history boundaries */ > > unsigned int xhlock_idx_hist[CONTEXT_NR]; > > + unsigned int hist_id; > > + /* For overwrite check at each context exit */ > > + unsigned int hist_id_save[CONTEXT_NR]; > > #endif > > > > #ifdef CONFIG_UBSAN > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > > index afd6e64..5168dac 100644 > > --- a/kernel/locking/lockdep.c > > +++ b/kernel/locking/lockdep.c > > @@ -4742,6 +4742,17 @@ 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; > > +} > > + > > +/* > > * Lock history stacks; we have 3 nested lock history stacks: > > * > > * Hard IRQ > > @@ -4773,14 +4784,28 @@ void lockdep_rcu_suspicious(const char *file, > const int line, const char *s) > > */ > > void crossrelease_hist_start(enum context_t c) > > { > > - if (current->xhlocks) > > - current->xhlock_idx_hist[c] = current->xhlock_idx; > > + struct task_struct *cur = current; > > + > > + if (cur->xhlocks) { > > + cur->xhlock_idx_hist[c] = cur->xhlock_idx; > > + cur->hist_id_save[c] = cur->hist_id; > > + } > > } > > > > void crossrelease_hist_end(enum context_t c) > > { > > - if (current->xhlocks) > > - current->xhlock_idx = current->xhlock_idx_hist[c]; > > + struct task_struct *cur = current; > > + > > + if (cur->xhlocks) { > > + unsigned int idx = cur->xhlock_idx_hist[c]; > > + struct hist_lock *h = &xhlock(idx); > > + > > + cur->xhlock_idx = idx; > > + > > + /* Check if the ring was overwritten. */ > > + if (h->hist_id != cur->hist_id_save[c]) > > Could we use: > > if (h->hist_id != idx) No, we cannot. hist_id is a kind of timestamp and used to detect overwriting data into places of same indexes of the ring buffer. And idx is just an index. :) IOW, they mean different things. > > here, and > > > + invalidate_xhlock(h); > > + } > > } > > > > static int cross_lock(struct lockdep_map *lock) > > @@ -4826,6 +4851,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. > > */ > > @@ -4857,6 +4883,7 @@ static void add_xhlock(struct held_lock *hlock) > > > > /* Initialize hist_lock's members */ > > xhlock->hlock = *hlock; > > + xhlock->hist_id = current->hist_id++; > > use: > > xhlock->hist_id = idx; > > and, Same. > > > > > > xhlock->trace.nr_entries = 0; > > xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES; > > @@ -4995,6 +5022,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; > > use: > unsigned int prev_hist_id = cur; > > here. Same. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
