From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.fireflyinternet.com ([109.228.58.192]:54607 "EHLO fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751515AbdLMLFU (ORCPT ); Wed, 13 Dec 2017 06:05:20 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: Daniel Vetter , "Intel Graphics Development" From: Chris Wilson In-Reply-To: <20171213104553.30821-1-daniel.vetter@ffwll.ch> Cc: "DRI Development" , "Daniel Vetter" , "Ben Widawsky" , "Dave Airlie" , "Sean Paul" , stable@vger.kernel.org, "Daniel Vetter" , "Jani Nikula" , "Gustavo Padovan" , "David Airlie" , "Javier Martinez Canillas" , "Shuah Khan" , "Guillaume Tucker" , "Mark Brown" , "Kevin Hilman" , "Matt Hart" , "Thierry Escande" , "Tomeu Vizoso" , "Enric Balletbo i Serra" References: <20171213104553.30821-1-daniel.vetter@ffwll.ch> Message-ID: <151316309304.4445.13717289674758228537@mail.alporthouse.com> Subject: Re: [PATCH] drm: rework delayed connector cleanup in connector_iter Date: Wed, 13 Dec 2017 11:04:53 +0000 Sender: stable-owner@vger.kernel.org List-ID: Quoting Daniel Vetter (2017-12-13 10:45:53) > PROBE_DEFER also uses system_wq to reprobe drivers, which means when > that again fails, and we try to flush the overall system_wq (to get > all the delayed connectore cleanup work_struct completed), we > deadlock. > > Fix this by using just a single cleanup work, so that we can only > flush that one and don't block on anything else. That means a free > list plus locking, a standard pattern. > > Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter") > Fixes: 613051dac40d ("drm: locking&new iterators for connector_list") > Cc: Ben Widawsky > Cc: Dave Airlie > Cc: Chris Wilson > Cc: Sean Paul > Cc: # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list" > Cc: # v4.11+ > Cc: Daniel Vetter > Cc: Jani Nikula > Cc: Gustavo Padovan > Cc: David Airlie > Cc: Javier Martinez Canillas > Cc: Shuah Khan > Cc: Guillaume Tucker > Cc: Mark Brown > Cc: Kevin Hilman > Cc: Matt Hart > Cc: Thierry Escande > Cc: Tomeu Vizoso > Cc: Enric Balletbo i Serra > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_connector.c | 46 ++++++++++++++++++++++++++----------- > drivers/gpu/drm/drm_crtc_internal.h | 1 + > drivers/gpu/drm/drm_mode_config.c | 5 +++- > include/drm/drm_connector.h | 4 ++-- > include/drm/drm_mode_config.h | 17 +++++++++++++- > 5 files changed, 55 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 0b7e0974e6da..2f43dc217936 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref) > connector->funcs->destroy(connector); > } > > -static void drm_connector_free_work_fn(struct work_struct *work) > +void drm_connector_free_work_fn(struct work_struct *work) > { > - struct drm_connector *connector = > - container_of(work, struct drm_connector, free_work); > - struct drm_device *dev = connector->dev; > + struct drm_connector *connector; > + struct drm_device *dev = > + container_of(work, struct drm_device, mode_config.connector_free_work); > + struct drm_mode_config *config = &dev->mode_config; > + unsigned long flags; > + LIST_HEAD(list_head); > > - drm_mode_object_unregister(dev, &connector->base); > - connector->funcs->destroy(connector); > + spin_lock_irqsave(&config->connector_list_lock, flags); > + list_splice(&config->connector_free_list, &list_head); > + spin_unlock_irqrestore(&config->connector_list_lock, flags); > + > + list_for_each_entry(connector, &list_head, free_head) { > + drm_mode_object_unregister(dev, &connector->base); > + connector->funcs->destroy(connector); > + } > } > > /** > @@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev, > if (ret) > return ret; > > - INIT_WORK(&connector->free_work, drm_connector_free_work_fn); > - > connector->base.properties = &connector->properties; > connector->dev = dev; > connector->funcs = funcs; > @@ -550,10 +557,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin); > * actually release the connector when dropping our final reference. > */ > static void > -drm_connector_put_safe(struct drm_connector *conn) > +__drm_connector_put_safe(struct drm_connector *conn) > { > + struct drm_mode_config *config = &conn->dev->mode_config; > + > + lockdep_assert_held(&config->connector_list_lock); > + list_add(&conn->free_head, &config->connector_free_list); Adding to the free_list before refcount==0? May I suggest, if (!refcount_dec_and_test()) return; if (llist_add(&conn->free_link, &config->connector_free_list)) schedule_work(&config->connector_free_work); llist being much more friendly for freelists: void drm_connector_free_work_fn(struct work_struct *work) { struct drm_device *dev = container_of(work, struct drm_device, mode_config.connector_free_work); struct drm_mode_config *config = &dev->mode_config; struct drm_connector *connector, *n; struct llist_node *freed; freed = llist_del_all(config->connector_free_list); llist_for_each_entry_safe(connector, n, freed, free_link) { drm_mode_object_unregister(dev, &connector->base); connector->funcs->destroy(connector); } } -Chris From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH] drm: rework delayed connector cleanup in connector_iter Date: Wed, 13 Dec 2017 11:04:53 +0000 Message-ID: <151316309304.4445.13717289674758228537@mail.alporthouse.com> References: <20171213104553.30821-1-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <20171213104553.30821-1-daniel.vetter@ffwll.ch> Sender: stable-owner@vger.kernel.org To: Intel Graphics Development Cc: DRI Development , Daniel Vetter , Ben Widawsky , Dave Airlie , Sean Paul , stable@vger.kernel.org, Daniel Vetter , Jani Nikula , Gustavo Padovan , David Airlie , Javier Martinez Canillas , Shuah Khan , Guillaume Tucker , Mark Brown , Kevin Hilman , Matt Hart , Thierry Escande , Tomeu Vizoso , Enric Balletbo List-Id: dri-devel@lists.freedesktop.org Quoting Daniel Vetter (2017-12-13 10:45:53) > PROBE_DEFER also uses system_wq to reprobe drivers, which means when > that again fails, and we try to flush the overall system_wq (to get > all the delayed connectore cleanup work_struct completed), we > deadlock. > > Fix this by using just a single cleanup work, so that we can only > flush that one and don't block on anything else. That means a free > list plus locking, a standard pattern. > > Fixes: a703c55004e1 ("drm: safely free connectors from connector_iter") > Fixes: 613051dac40d ("drm: locking&new iterators for connector_list") > Cc: Ben Widawsky > Cc: Dave Airlie > Cc: Chris Wilson > Cc: Sean Paul > Cc: # v4.11+: 613051dac40d ("drm: locking&new iterators for connector_list" > Cc: # v4.11+ > Cc: Daniel Vetter > Cc: Jani Nikula > Cc: Gustavo Padovan > Cc: David Airlie > Cc: Javier Martinez Canillas > Cc: Shuah Khan > Cc: Guillaume Tucker > Cc: Mark Brown > Cc: Kevin Hilman > Cc: Matt Hart > Cc: Thierry Escande > Cc: Tomeu Vizoso > Cc: Enric Balletbo i Serra > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_connector.c | 46 ++++++++++++++++++++++++++----------- > drivers/gpu/drm/drm_crtc_internal.h | 1 + > drivers/gpu/drm/drm_mode_config.c | 5 +++- > include/drm/drm_connector.h | 4 ++-- > include/drm/drm_mode_config.h | 17 +++++++++++++- > 5 files changed, 55 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 0b7e0974e6da..2f43dc217936 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -153,14 +153,23 @@ static void drm_connector_free(struct kref *kref) > connector->funcs->destroy(connector); > } > > -static void drm_connector_free_work_fn(struct work_struct *work) > +void drm_connector_free_work_fn(struct work_struct *work) > { > - struct drm_connector *connector = > - container_of(work, struct drm_connector, free_work); > - struct drm_device *dev = connector->dev; > + struct drm_connector *connector; > + struct drm_device *dev = > + container_of(work, struct drm_device, mode_config.connector_free_work); > + struct drm_mode_config *config = &dev->mode_config; > + unsigned long flags; > + LIST_HEAD(list_head); > > - drm_mode_object_unregister(dev, &connector->base); > - connector->funcs->destroy(connector); > + spin_lock_irqsave(&config->connector_list_lock, flags); > + list_splice(&config->connector_free_list, &list_head); > + spin_unlock_irqrestore(&config->connector_list_lock, flags); > + > + list_for_each_entry(connector, &list_head, free_head) { > + drm_mode_object_unregister(dev, &connector->base); > + connector->funcs->destroy(connector); > + } > } > > /** > @@ -192,8 +201,6 @@ int drm_connector_init(struct drm_device *dev, > if (ret) > return ret; > > - INIT_WORK(&connector->free_work, drm_connector_free_work_fn); > - > connector->base.properties = &connector->properties; > connector->dev = dev; > connector->funcs = funcs; > @@ -550,10 +557,15 @@ EXPORT_SYMBOL(drm_connector_list_iter_begin); > * actually release the connector when dropping our final reference. > */ > static void > -drm_connector_put_safe(struct drm_connector *conn) > +__drm_connector_put_safe(struct drm_connector *conn) > { > + struct drm_mode_config *config = &conn->dev->mode_config; > + > + lockdep_assert_held(&config->connector_list_lock); > + list_add(&conn->free_head, &config->connector_free_list); Adding to the free_list before refcount==0? May I suggest, if (!refcount_dec_and_test()) return; if (llist_add(&conn->free_link, &config->connector_free_list)) schedule_work(&config->connector_free_work); llist being much more friendly for freelists: void drm_connector_free_work_fn(struct work_struct *work) { struct drm_device *dev = container_of(work, struct drm_device, mode_config.connector_free_work); struct drm_mode_config *config = &dev->mode_config; struct drm_connector *connector, *n; struct llist_node *freed; freed = llist_del_all(config->connector_free_list); llist_for_each_entry_safe(connector, n, freed, free_link) { drm_mode_object_unregister(dev, &connector->base); connector->funcs->destroy(connector); } } -Chris