From: Byungchul Park <max.byungchul.park@gmail.com>
To: Byungchul Park <byungchul.park@lge.com>
Cc: Boqun Feng <boqun.feng@gmail.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@kernel.org>,
tglx@linutronix.de, Michel Lespinasse <walken@google.com>,
kirill@shutemov.name,
"linux-kernel@vger.kernel.org" <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: Fri, 11 Aug 2017 22:06:37 +0900 [thread overview]
Message-ID: <CANrsvRM4ijD0ym0HJySqjOfcCeUbGCc6bBppK43y5MqC5aB1gQ@mail.gmail.com> (raw)
In-Reply-To: <20170811094448.GJ20323@X58A-UD3R>
On Fri, Aug 11, 2017 at 6:44 PM, Byungchul Park <byungchul.park@lge.com> wrote:
> On Fri, Aug 11, 2017 at 05:52:02PM +0900, Byungchul Park wrote:
>> On Fri, Aug 11, 2017 at 04:03:29PM +0800, Boqun Feng wrote:
>> > Thanks for taking a look at it ;-)
>>
>> I rather appriciate it.
>>
>> > > > @@ -5005,7 +5003,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 prev_hist_id = cur + 1;
>> > >
>> > > I should have named it another. Could you suggest a better one?
>> > >
>> >
>> > I think "prev" is fine, because I thought the "previous" means the
>> > xhlock item we visit _previously_.
>> >
>> > > > unsigned int i;
>> > > >
>> > > > if (!graph_lock())
>> > > > @@ -5030,7 +5028,7 @@ static void commit_xhlocks(struct cross_lock *xlock)
>> > > > * hist_id than the following one, which is impossible
>> > > > * otherwise.
>> > >
>> > > Or we need to modify the comment so that the word 'prev' does not make
>> > > readers confused. It was my mistake.
>> > >
>> >
>> > I think the comment needs some help, but before you do it, could you
>> > have another look at what Peter proposed previously? Note you have a
>> > same_context_xhlock() check in the commit_xhlocks(), so the your
>> > previous overwrite case actually could be detected, I think.
>>
>> What is the previous overwrite case?
>>
>> ppppppppppwwwwwwwwwwwwiiiiiiiii
>> iiiiiiiiiiiiiii................
>>
>> Do you mean this one? I missed the check of same_context_xhlock(). Yes,
>> peterz's suggestion also seems to work.
>>
>> > However, one thing may not be detected is this case:
>> >
>> > ppppppppppppppppppppppppppppppppppwwwwwwww
>> > wrapped > wwwwwww
>>
>> To be honest, I think your suggestion is more natual, with which this
>> case would be also covered.
>>
>> >
>> > where p: process and w: worker.
>> >
>> > , because p and w are in the same task_irq_context(). I discussed this
>> > with Peter yesterday, and he has a good idea: unconditionally do a reset
>> > on the ring buffer whenever we do a crossrelease_hist_end(XHLOCK_PROC).
>
> Ah, ok. You meant 'whenever _process_ context exit'.
>
> I need more time to be sure, but anyway for now it seems to work with
> giving up some chances for remaining xhlocks.
>
> But, I am not sure if it's still true even in future and the code can be
> maintained easily. I think your approach is natural and neat enough for
> that purpose. What problem exists with yours?
Let me list up the possible approaches:
0. Byungchul's approach
1. Boqun's approach
2. Peterz's approach
3. Reset on process exit
I like Boqun's approach most but, _whatever_. It's ok if it solves the problem.
The last one is not bad when it is used for syscall exit, but we have to give
up valid dependencies unnecessarily in other cases. And I think Peterz's
approach should be modified a bit to make it work neatly, like:
crossrelease_hist_end(...)
{
...
invalidate_xhlock(&xhlock(cur->xhlock_idx_max));
for (c = 0; c < XHLOCK_CXT_NR; c++)
if ((cur->xhlock_idx_max - cur->xhlock_idx_hist[c]) >=
MAX_XHLOCKS_NR)
invalidate_xhlock(&xhlock(cur->xhlock_idx_hist[c]));
...
}
And then Peterz's approach can also work, I think.
---
Thanks,
Byungchul
next prev parent reply other threads:[~2017-08-11 13:06 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
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 [this message]
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=CANrsvRM4ijD0ym0HJySqjOfcCeUbGCc6bBppK43y5MqC5aB1gQ@mail.gmail.com \
--to=max.byungchul.park@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=boqun.feng@gmail.com \
--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).