linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Will Deacon <will.deacon@arm.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	mingo@redhat.com, tj@kernel.org, longman@redhat.com,
	johannes.berg@intel.com, linux-kernel@vger.kernel.org,
	Paul McKenney <paulmck@linux.vnet.ibm.com>
Subject: Re: [PATCH v6 00/16] locking/lockdep: Add support for dynamic keys
Date: Fri, 8 Feb 2019 11:43:15 +0000	[thread overview]
Message-ID: <20190208114315.GD6972@fuggles.cambridge.arm.com> (raw)
In-Reply-To: <3c533b0e-b079-79ad-4935-cd61af000ce6@acm.org>

Hi Bart, Peter,

On Sun, Feb 03, 2019 at 09:36:38AM -0800, Bart Van Assche wrote:
> On 2/1/19 4:15 AM, Peter Zijlstra wrote:
> > On Fri, Jan 18, 2019 at 06:34:20PM -0800, Bart Van Assche wrote:
> > > I agree with what you wrote. The only code I know of that accesses list
> > > entries using RCU is the __bfs() function. In that function I found the
> > > following loop:
> > > 
> > > 	list_for_each_entry_rcu(entry, head, entry) { [ ... ] }
> > 
> > Thing is; I can't seem to find any __bfs() usage outside of graph_lock.
> > 
> >    count_{fwd,bwd}_deps() - takes graph lock
> > 
> >    check_{noncircular,redudant}() - called from check_prev_add() <-
> >    check_prevs_add() <- validate_chain() which takes graph lock
> > 
> >    find_usage{,_fwd,_bwd}
> >      <- check_usage() <- check_irq_usage() <- check_prev_add_irq() <-
> >      check_prev_add <- check_prevs_add() <- validate_chain() which takes
> >      graph lock
> > 
> >      <- check_usage_{fwd,bdw}() <- mark_lock_irq() <- mark_lock() which
> >      takes graph lock
> > 
> > Or did I miss something? If there are no __bfs() users outside of graph
> > lock, then we can simply remove that _rcu from the iteration, and
> > simplify all that.
> 
> Every time I make a single change to the lockdep code I have to rerun my
> test case for a week to make sure that no regressions have been introduced.
> In other words, I can make further changes but that could take some time. Do
> you want me to look into this simplification now or after this patch series
> went upstream?
> 
> > > Since zap_class() calls list_del_rcu(&entry->entry), since a grace period
> > > occurs between the call_rcu() invocation and the RCU callback function,
> > > since at least an RCU reader lock must be held around RCU loops and since
> > > sleeping is not allowed while holding an RCU read lock I think there is
> > > no risk that __bfs() will examine a list entry after it has been freed.
> > 
> > So you agree that list_entry_being_freed() should only check the current
> > pf?
> 
> Sorry if I wasn't clear enough. In a previous e-mail I tried to explain that
> both pf's have to be checked. Another way to explain that is as follows:
> - Each list entry has one of the following states: free, in use or being
>   freed.
> - "Free" means that the corresponding bit in the list_entries_in_use
>   bitmap has not been set.
> - "In use" means that the corresponding bit in the list_entries_in_use
>   bitmap has been set and that none of the corresponding bits in the
>   list_entries_being_freed bitmaps have been set.
> - "Being freed" means that the corresponding bit in one of the
>   list_entries_being_freed bitmaps has been set.
> 
> Since it can happen that multiple elements of the pending_free[] array are
> in the state where call_rcu() has been called but the RCU callback function
> has not yet been called, I think that zap_class() must check the
> list_entries_being_freed bitmaps in all pending_free[] array elements.

I've also been trying to understand why it's necessary to check both of the
pending_free entries, and I'm still struggling somewhat. It's true that the
wakeup in get_pending_free_lock() could lead to both entries being used
without the RCU call back running in between, however in this scenario then
any list entries marked for freeing in the first pf will have been unhashed
and therefore made unreachable to look_up_lock_class().

So I think the concern remains that entries are somehow remaining visible
after being zapped.

You mentioned earlier in the thread that people actually complained about
list corruption if you only checked the current pf:

  | The list_del_rcu() call must only happen once. I ran into complaints
  | reporting that the list_del_rcu() call triggered list corruption. This
  | change made these complaints disappear.

Do you have any more details about these complaints (e.g. kernel logs etc)?
Failing that, any idea how to reproduce them?

Thanks,

Will

  reply	other threads:[~2019-02-08 11:43 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-09 21:01 [PATCH v6 00/16] locking/lockdep: Add support for dynamic keys Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 01/16] locking/lockdep: Fix reported required memory size Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 02/16] locking/lockdep: Avoid that add_chain_cache() adds an invalid chain to the cache Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 03/16] locking/lockdep: Make zap_class() remove all matching lock order entries Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 04/16] locking/lockdep: Reorder struct lock_class members Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 05/16] locking/lockdep: Initialize the locks_before and locks_after lists earlier Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 06/16] locking/lockdep: Split lockdep_free_key_range() and lockdep_reset_lock() Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 07/16] locking/lockdep: Make it easy to detect whether or not inside a selftest Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 08/16] locking/lockdep: Free lock classes that are no longer in use Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 09/16] locking/lockdep: Reuse list entries " Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 10/16] locking/lockdep: Introduce lockdep_next_lockchain() and lock_chain_count() Bart Van Assche
2019-01-09 21:01 ` [PATCH v6 11/16] locking/lockdep: Reuse lock chains that have been freed Bart Van Assche
2019-01-09 21:02 ` [PATCH v6 12/16] locking/lockdep: Check data structure consistency Bart Van Assche
2019-01-09 21:02 ` [PATCH v6 13/16] locking/lockdep: Verify whether lock objects are small enough to be used as class keys Bart Van Assche
2019-01-09 21:02 ` [PATCH v6 14/16] locking/lockdep: Add support for dynamic keys Bart Van Assche
2019-01-09 21:02 ` [PATCH v6 15/16] kernel/workqueue: Use dynamic lockdep keys for workqueues Bart Van Assche
2019-01-09 21:02 ` [PATCH v6 16/16] lockdep tests: Test dynamic key registration Bart Van Assche
2019-01-11 12:48 ` [PATCH v6 00/16] locking/lockdep: Add support for dynamic keys Peter Zijlstra
2019-01-11 15:55   ` Bart Van Assche
2019-01-11 16:55     ` Peter Zijlstra
2019-01-11 17:01       ` Bart Van Assche
2019-01-14 12:52         ` Peter Zijlstra
2019-01-14 16:52           ` Bart Van Assche
2019-01-18  9:48             ` Peter Zijlstra
2019-01-19  2:34               ` Bart Van Assche
2019-02-01 12:15                 ` Peter Zijlstra
2019-02-03 17:36                   ` Bart Van Assche
2019-02-08 11:43                     ` Will Deacon [this message]
2019-02-08 16:31                       ` Bart Van Assche
2019-02-13 22:32                       ` Bart Van Assche

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=20190208114315.GD6972@fuggles.cambridge.arm.com \
    --to=will.deacon@arm.com \
    --cc=bvanassche@acm.org \
    --cc=johannes.berg@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=longman@redhat.com \
    --cc=mingo@redhat.com \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=tj@kernel.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).