On Fri, Oct 30, 2020 at 11:51:18AM +0800, Boqun Feng wrote:
> On Wed, Oct 28, 2020 at 08:59:10PM +0100, Peter Zijlstra wrote:
> Sorry for the late response.
No worries, glad you could have a look.
> > So that's commit f611e8cf98ec ("lockdep: Take read/write status in
> > consideration when generate chainkey") that did that.
> >
>
> Yeah, I think that's related, howver ...
It's the commit that made the chainkey depend on the read state, and
thus introduced this connondrum.
> > So validate_chain() requires the new chain_key, but can change ->read
> > which then invalidates the chain_key we just calculated.
> >
> > This happens when check_deadlock() returns 2, which only happens when:
> >
> > - next->read == 2 && ... ; however @hext is our @hlock, so that's
> > pointless
> >
>
> I don't think we should return 2 (earlier) in this case anymore. Because
> now we have recursive read deadlock detection, it's safe to add dep:
> "prev -> next" in the dependency graph. I think we can just continue in
> this case. Actually I think this is something I'm missing in my
> recursive read detection patchset :-/
Yes, I agree, this case should go. We now fully support recursive read
depndencies per your recent work.
> > - when there's a nest_lock involved ; ww_mutex uses that !!!
> >
>
> That leaves check_deadlock() return 2 only if hlock is a nest_lock, and
> ...
> > @@ -3597,8 +3598,12 @@ static int validate_chain(struct task_struct *curr,
> > * building dependencies (just like we jump over
> > * trylock entries):
> > */
> > - if (ret == 2)
> > + if (ret == 2) {
> > hlock->read = 2;
> > + *chain_key = iterate_chain_key(hlock->prev_chain_key, hlock_id(hlock));
>
> If "ret == 2" means hlock is a a nest_lock, than we don't need the
> "->read = 2" trick here and we don't need to update chain_key either.
> We used to have this "->read = 2" only because we want to skip the
> dependency adding step afterwards. So how about the following:
>
> It survived a lockdep selftest at boot time.
Right, but our self-tests didn't trigger this problem to begin with, let
me go try and create one that does.
> ----------------------------->8
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 3e99dfef8408..b23ca6196561 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -2765,7 +2765,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
> * (Note that this has to be done separately, because the graph cannot
> * detect such classes of deadlocks.)
> *
> - * Returns: 0 on deadlock detected, 1 on OK, 2 on recursive read
> + * Returns: 0 on deadlock detected, 1 on OK, 2 on nest_lock
> */
> static int
> check_deadlock(struct task_struct *curr, struct held_lock *next)
> @@ -2788,7 +2788,7 @@ check_deadlock(struct task_struct *curr, struct held_lock *next)
> * lock class (i.e. read_lock(lock)+read_lock(lock)):
> */
> if ((next->read == 2) && prev->read)
> - return 2;
> + continue;
>
> /*
> * We're holding the nest_lock, which serializes this lock's
> @@ -3592,16 +3592,9 @@ static int validate_chain(struct task_struct *curr,
>
> if (!ret)
> return 0;
> - /*
> - * Mark recursive read, as we jump over it when
> - * building dependencies (just like we jump over
> - * trylock entries):
> - */
> - if (ret == 2)
> - hlock->read = 2;
> /*
> * Add dependency only if this lock is not the head
> - * of the chain, and if it's not a secondary read-lock:
> + * of the chain, and if it's not a nest_lock:
> */
> if (!chain_head && ret != 2) {
> if (!check_prevs_add(curr, hlock))
I'm not entirely sure that doesn't still trigger the problem. Consider
@chain_head := true.
Anyway, let me go try and write this self-tests, maybe that'll get my
snot-addled brains sufficiently aligned to make sense of all this :/
|