On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
<snip>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
