[Intel-gfx] [PATCH 05/13] drm: locking&new iterators for connector_list
On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter
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
> Cc: Chris Wilson
Reviewed-by: Sean Paul
> Signed-off-by: Daniel Vetter
[Intel-gfx] [PATCH 05/13] drm: locking&new iterators for connector_list
On Wed, 14 Dec 2016, Daniel Vetter wrote: > On Wed, Dec 14, 2016 at 01:22:15PM +0200, Jani Nikula wrote: >> On Wed, 14 Dec 2016, Daniel Vetter wrote: >> > +/** >> > + * drm_for_each_connector_iter - connector_list iterator macro >> > + * @connector: struct &drm_connector pointer used as cursor >> > + * @iter: struct &drm_connector_list_iter >> > + * >> > + * Note that @connector is only valid within the list body, if you want >> > to use >> > + * @connector after calling drm_connector_list_iter_put() then you need >> > to grab >> > + * your own reference first using drm_connector_reference(). >> > + */ >> > +#define drm_for_each_connector_iter(connector, iter) \ >> > + while ((connector = drm_connector_list_iter_next(iter))) >> > + >> >> Observe that in most, if not all, cases you lock over the for loop, but >> not more. That means you always get/put right around the loop. >> >> You could have a variant of get() that returns the first item, and a >> variant of next() that does put() automatically when it's about to >> return NULL, and implement most of the loops like this: >> >> #define drm_for_each_connector_simple(dev, iter, connector) \ >> for (connector = drm_connector_list_iter_get_first(dev, iter); \ >> connector != NULL; \ >> connector = drm_connector_list_iter_next_put(iter)) >> >> In the long run, that should be called just drm_for_each_connector. >> >> The only case where you'd have to call put() explicitly is when you >> break out of the loop early. Otherwise all looping would be dead simple, >> without all the gets and puts, just like they are now. Perhaps the >> naming of the functions should be such that this is the most common >> case. Perhaps you don't actually need the versions with "manual" locking >> at all. > > I had this in an earlier iteration of this patch series. The implemenation > was somewhat misguided (as in it used srcu and some other wizzardry that I > now managed to remove), but otherwise was exactly what you've asking for > here. > > The amount of leaking was mindboggling. > > And that was only me being sloppyin in converting the piles of existing > loop, not even ongoing maintenance of new loops additions done by people > who're not well versed in the finer details of connector_list walking and > the refcounting dance involved. > > Given that I've concluded that hiding those details is a bad choice, and > to top it off the new code enforces matching get/put using lockdep. We do > pay a price in that simple loops become a bit more verbose, but > unfortunately there's no way to create something which is guarnateed to > get destructed when leaving a code block (unlike in C++). And without that > guarantee I don't think it'll be maintainable long-term. > > I expect that drm_for_each_connector will stay around for a long time > (maybe even forever). As long as your driver doesn't hotplug connectors, > it's perfectly fine. Only core + helpers + any driver supporting mst > really need to switch over. > > Overall not the prettiest thing, but still an acceptable tradeoff imo. Fair enough. Jani. -- Jani Nikula, Intel Open Source Technology Center
[Intel-gfx] [PATCH 05/13] drm: locking&new iterators for connector_list
On Wed, Dec 14, 2016 at 01:22:15PM +0200, Jani Nikula wrote: > On Wed, 14 Dec 2016, Daniel Vetter wrote: > > +/** > > + * drm_for_each_connector_iter - connector_list iterator macro > > + * @connector: struct &drm_connector pointer used as cursor > > + * @iter: struct &drm_connector_list_iter > > + * > > + * Note that @connector is only valid within the list body, if you want to > > use > > + * @connector after calling drm_connector_list_iter_put() then you need to > > grab > > + * your own reference first using drm_connector_reference(). > > + */ > > +#define drm_for_each_connector_iter(connector, iter) \ > > + while ((connector = drm_connector_list_iter_next(iter))) > > + > > Observe that in most, if not all, cases you lock over the for loop, but > not more. That means you always get/put right around the loop. > > You could have a variant of get() that returns the first item, and a > variant of next() that does put() automatically when it's about to > return NULL, and implement most of the loops like this: > > #define drm_for_each_connector_simple(dev, iter, connector) \ > for (connector = drm_connector_list_iter_get_first(dev, iter); \ >connector != NULL; \ >connector = drm_connector_list_iter_next_put(iter)) > > In the long run, that should be called just drm_for_each_connector. > > The only case where you'd have to call put() explicitly is when you > break out of the loop early. Otherwise all looping would be dead simple, > without all the gets and puts, just like they are now. Perhaps the > naming of the functions should be such that this is the most common > case. Perhaps you don't actually need the versions with "manual" locking > at all. I had this in an earlier iteration of this patch series. The implemenation was somewhat misguided (as in it used srcu and some other wizzardry that I now managed to remove), but otherwise was exactly what you've asking for here. The amount of leaking was mindboggling. And that was only me being sloppyin in converting the piles of existing loop, not even ongoing maintenance of new loops additions done by people who're not well versed in the finer details of connector_list walking and the refcounting dance involved. Given that I've concluded that hiding those details is a bad choice, and to top it off the new code enforces matching get/put using lockdep. We do pay a price in that simple loops become a bit more verbose, but unfortunately there's no way to create something which is guarnateed to get destructed when leaving a code block (unlike in C++). And without that guarantee I don't think it'll be maintainable long-term. I expect that drm_for_each_connector will stay around for a long time (maybe even forever). As long as your driver doesn't hotplug connectors, it's perfectly fine. Only core + helpers + any driver supporting mst really need to switch over. Overall not the prettiest thing, but still an acceptable tradeoff imo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[Intel-gfx] [PATCH 05/13] drm: locking&new iterators for connector_list
On Wed, 14 Dec 2016, Daniel Vetter 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
> Cc: Chris Wilson
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_atomic.c | 14 -
> drivers/gpu/drm/drm_connector.c | 116
> --
> drivers/gpu/drm/drm_encoder.c | 6 +-
> drivers/gpu/drm/drm_mode_config.c | 34 +--
> include/drm/drm_connector.h | 38 +
> include/drm/drm_mode_config.h | 12 +++-
> 6 files changed, 177 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 60697482b94c..b23b4abd67be 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1417,6 +1417,7 @@ drm_atomic_add_affected_connectors(struct
> drm_atomic_state *state,
> struct drm_mode_config *config = &state->dev->mode_config;
> struct drm_connector *connector;
> struct drm_connector_state *conn_state;
> + struct drm_connector_list_iter conn_iter;
> int ret;
>
> ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
> @@ -1430,14 +1431,18 @@ drm_atomic_add_affected_connectors(struct
> drm_atomic_state *state,
>* Changed connectors are already in @state, so only need to look at the
>* current configuration.
>*/
> - drm_for_each_connector(connector, state->dev) {
> + drm_connector_list_iter_get(state->dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> if (connector->state->crtc != crtc)
> continue;
>
> conn_state = drm_atomic_get_con
