[Intel-gfx] [PATCH 05/13] drm: locking&new iterators for connector_list

[Sean Paul]

On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> The requirements for connector_list locking are a bit tricky:
> - We need to be able to jump over zombie conectors (i.e. with refcount
>   == 0, but not yet removed from the list). If instead we require that
>   there's no zombies on the list then the final kref_put must happen
>   under the list protection lock, which means that locking context
>   leaks all over the place. Not pretty - better to deal with zombies
>   and wrap the locking just around the list_del in the destructor.
>
> - When we walk the list we must _not_ hold the connector list lock. We
>   walk the connector list at an absolutely massive amounts of places,
>   if all those places can't ever call drm_connector_unreference the
>   code would get unecessarily complicated.
>
> - connector_list needs it own lock, again too many places that walk it
>   that we could reuse e.g. mode_config.mutex without resulting in
>   inversions.
>
> - Lots of code uses these loops to look-up a connector, i.e. they want
>   to be able to call drm_connector_reference. But on the other hand we
>   want connectors to stay on that list until they're dead (i.e.
>   connector_list can't hold a full reference), which means despite the
>   "can't hold lock for the loop body" rule we need to make sure a
>   connector doesn't suddenly become a zombie.
>
> At first Dave&I discussed various horror-show approaches using srcu,
> but turns out it's fairly easy:
>
> - For the loop body we always hold an additional reference to the
>   current connector. That means it can't zombify, and it also means
>   it'll stay on the list, which means we can use it as our iterator to
>   find the next connector.
>
> - When we try to find the next connector we only have to jump over
>   zombies. To make sure we don't chase bad pointers that entire loop
>   is protected with the new connect_list_lock spinlock. And because we
>   know that we're starting out with a non-zombie (need to drop our
>   reference for the old connector only after we have our new one),
>   we're guranteed to still be on the connector_list and either find
>   the next non-zombie or complete the iteration.
>
> - Only downside is that we need to make sure that the temporary
>   reference for the loop body doesn't leak. iter_get/put() functions +
>   lockdep make sure that's the case.
>
> - To avoid a flag day the new iterator macro has an _iter postfix. We
>   can rename it back once all the users of the unsafe version are gone
>   (there's about 100 list walkers for the connector_list).
>
> For now this patch only converts all the list walking in the core,
> leaving helpers and drivers for later patches. The nice thing is that
> we can now finally remove 2 FIXME comments from the
> register/unregister functions.
>
> v2:
> - use irqsafe spinlocks, so that we can use this in drm_state_dump
>   too.
> - nuke drm_modeset_lock_all from drm_connector_init, now entirely
>   cargo-culted nonsense.
>
> v3:
> - do {} while (!kref_get_unless_zero), makes for a tidier loop (Dave).
> - pretty kerneldoc
> - add EXPORT_SYMBOL, helpers&drivers are supposed to use this.
>
> v4: Change lockdep annotations to only check whether we release the
> iter fake lock again (i.e. make sure that iter_put is called), but
> not check any locking dependecies itself. That seams to require a
> recursive read lock in trylock mode.
>
> Cc: Dave Airlie <airlied at gmail.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>



Reviewed-by: Sean Paul <seanpaul at chromium.org>

> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

<snip>