From: Boqun Feng <boqun.feng@gmail.com>
To: Byungchul Park <byungchul.park@lge.com>
Cc: peterz@infradead.org, mingo@kernel.org, tglx@linutronix.de,
walken@google.com, kirill@shutemov.name,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org, willy@infradead.org,
npiggin@gmail.com, kernel-team@lge.com
Subject: Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring buffer overwrite
Date: Thu, 10 Aug 2017 20:51:33 +0800 [thread overview]
Message-ID: <20170810125133.2poixhni4d5aqkpy@tardis> (raw)
In-Reply-To: <016b01d311d1$d02acfa0$70806ee0$@lge.com>
[-- Attachment #1: Type: text/plain, Size: 7624 bytes --]
On Thu, Aug 10, 2017 at 09:11:32PM +0900, Byungchul Park wrote:
> > -----Original Message-----
> > From: Boqun Feng [mailto:boqun.feng@gmail.com]
> > Sent: Thursday, August 10, 2017 8:59 PM
> > To: Byungchul Park
> > Cc: peterz@infradead.org; mingo@kernel.org; tglx@linutronix.de;
> > walken@google.com; kirill@shutemov.name; linux-kernel@vger.kernel.org;
> > linux-mm@kvack.org; akpm@linux-foundation.org; willy@infradead.org;
> > npiggin@gmail.com; kernel-team@lge.com
> > 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@lge.com>
> > > ---
> > > 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.
>
Hey, I'm not buying it. task_struct::hist_id and task_struct::xhlock_idx
are increased at the same place(in add_xhlock()), right?
And, yes, xhlock_idx will get decreased when we do ring-buffer
unwinding, but that's OK, because we need to throw away those recently
added items.
And xhlock_idx always points to the most recently added valid item,
right? Any other item's idx must "before()" the most recently added
one's, right? So ::xhlock_idx acts just like a timestamp, doesn't it?
Maybe I'm missing something subtle, but could you show me an example,
that could end up being a problem if we use xhlock_idx as the hist_id?
> 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++;
Besides, is this code correct? Does this just make xhlock->hist_id
one-less-than the curr->hist_id, which cause the invalidation every time
you do ring buffer unwinding?
Regards,
Boqun
> >
> > 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.
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2017-08-10 12:51 UTC|newest]
Thread overview: 89+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-07 7:12 [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park
2017-08-07 7:12 ` [PATCH v8 01/14] lockdep: Refactor lookup_chain_cache() Byungchul Park
2017-08-10 12:18 ` [tip:locking/core] locking/lockdep: " tip-bot for Byungchul Park
2017-08-07 7:12 ` [PATCH v8 02/14] lockdep: Add a function building a chain between two classes Byungchul Park
2017-08-10 12:18 ` [tip:locking/core] locking/lockdep: " tip-bot for Byungchul Park
2017-08-07 7:12 ` [PATCH v8 03/14] lockdep: Change the meaning of check_prev_add()'s return value Byungchul Park
2017-08-10 12:19 ` [tip:locking/core] locking/lockdep: " tip-bot for Byungchul Park
2017-08-07 7:12 ` [PATCH v8 04/14] lockdep: Make check_prev_add() able to handle external stack_trace Byungchul Park
2017-08-10 12:19 ` [tip:locking/core] locking/lockdep: " tip-bot for Byungchul Park
2017-08-07 7:12 ` [PATCH v8 05/14] lockdep: Implement crossrelease feature Byungchul Park
2017-08-09 14:05 ` Peter Zijlstra
2017-08-10 1:30 ` Byungchul Park
2017-08-10 9:21 ` Peter Zijlstra
2017-08-10 12:19 ` [tip:locking/core] locking/lockdep: Implement the 'crossrelease' feature tip-bot for Byungchul Park
2017-08-07 7:12 ` [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring buffer overwrite Byungchul Park
2017-08-09 14:16 ` Peter Zijlstra
2017-08-10 1:32 ` Byungchul Park
2017-08-10 9:22 ` Peter Zijlstra
2017-08-10 10:32 ` Byungchul Park
2017-08-10 11:59 ` Boqun Feng
2017-08-10 12:11 ` Byungchul Park
2017-08-10 12:51 ` Boqun Feng [this message]
2017-08-10 13:17 ` Boqun Feng
2017-08-11 0:44 ` Byungchul Park
2017-08-11 3:43 ` Byungchul Park
2017-08-11 8:03 ` Boqun Feng
2017-08-11 8:52 ` Byungchul Park
2017-08-11 9:44 ` Byungchul Park
2017-08-11 13:06 ` Byungchul Park
2017-08-14 7:05 ` Boqun Feng
2017-08-14 7:22 ` Byungchul Park
2017-08-14 7:29 ` Byungchul Park
2017-08-11 0:40 ` Byungchul Park
2017-08-11 1:03 ` Boqun Feng
2017-08-10 12:20 ` [tip:locking/core] locking/lockdep: " tip-bot for Byungchul Park
2017-08-07 7:12 ` [PATCH v8 07/14] lockdep: Handle non(or multi)-acquisition of a crosslock Byungchul Park
2017-08-10 12:20 ` [tip:locking/core] locking/lockdep: " tip-bot for Byungchul Park
2017-08-07 7:12 ` [PATCH v8 08/14] lockdep: Make print_circular_bug() aware of crossrelease Byungchul Park
2017-08-10 12:21 ` [tip:locking/core] locking/lockdep: " tip-bot for Byungchul Park
2017-08-07 7:12 ` [PATCH v8 09/14] lockdep: Apply crossrelease to completions Byungchul Park
2017-08-07 10:20 ` kbuild test robot
2017-08-07 11:45 ` kbuild test robot
2017-08-09 9:51 ` Peter Zijlstra
2017-08-09 10:24 ` Peter Zijlstra
2017-08-10 1:24 ` Byungchul Park
2017-08-10 12:21 ` [tip:locking/core] locking/lockdep: " tip-bot for Byungchul Park
2017-08-14 8:50 ` [PATCH v8 09/14] lockdep: " Arnd Bergmann
2017-08-18 23:43 ` Boqun Feng
2017-08-19 12:51 ` Arnd Bergmann
2017-08-19 13:34 ` Arnd Bergmann
2017-08-23 14:43 ` Boqun Feng
2017-08-20 3:18 ` Boqun Feng
2017-08-07 7:12 ` [PATCH v8 10/14] pagemap.h: Remove trailing white space Byungchul Park
2017-08-07 7:12 ` [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks Byungchul Park
2017-08-07 10:36 ` kbuild test robot
2017-08-10 1:35 ` Byungchul Park
2017-08-10 9:25 ` Peter Zijlstra
2017-09-05 1:03 ` Byungchul Park
2017-08-07 7:12 ` [PATCH v8 12/14] lockdep: Apply lock_acquire(release) on __Set(__Clear)PageLocked Byungchul Park
2017-08-07 7:13 ` [PATCH v8 13/14] lockdep: Move data of CONFIG_LOCKDEP_PAGELOCK from page to page_ext Byungchul Park
2017-08-07 10:43 ` kbuild test robot
2017-08-07 7:13 ` [PATCH v8 14/14] lockdep: Crossrelease feature documentation Byungchul Park
2017-08-07 15:58 ` kbuild test robot
2017-08-10 12:22 ` [tip:locking/core] locking/lockdep: Add 'crossrelease' " tip-bot for Byungchul Park
2017-08-09 15:50 ` [PATCH v8 00/14] lockdep: Implement crossrelease feature Peter Zijlstra
2017-08-10 0:55 ` Byungchul Park
2017-08-10 3:47 ` Byungchul Park
2017-08-10 10:52 ` Byungchul Park
2017-08-10 9:37 ` Byungchul Park
2017-08-10 10:52 ` Peter Zijlstra
2017-08-10 11:10 ` Ingo Molnar
2017-08-10 11:45 ` Byungchul Park
2017-08-14 10:57 ` Ingo Molnar
2017-08-14 11:10 ` Byungchul Park
2017-08-15 8:20 ` Ingo Molnar
2017-08-16 0:16 ` Byungchul Park
2017-08-16 4:05 ` Boqun Feng
2017-08-16 4:37 ` Byungchul Park
2017-08-16 5:40 ` Boqun Feng
2017-08-16 6:37 ` Byungchul Park
2017-08-16 5:05 ` Byungchul Park
2017-08-16 5:58 ` Boqun Feng
2017-08-16 7:14 ` Byungchul Park
2017-08-16 8:06 ` Byungchul Park
2017-08-16 9:38 ` Byungchul Park
2017-08-17 7:48 ` Ingo Molnar
2017-08-17 8:04 ` Boqun Feng
2017-08-17 8:12 ` Ingo Molnar
2017-08-17 8:33 ` Boqun Feng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170810125133.2poixhni4d5aqkpy@tardis \
--to=boqun.feng@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=byungchul.park@lge.com \
--cc=kernel-team@lge.com \
--cc=kirill@shutemov.name \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mingo@kernel.org \
--cc=npiggin@gmail.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=walken@google.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).