Re: [PATCH 5/8] drm: trylock modest locking for fbdev panics

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Jul 29, 2014 at 11:32:20PM +0200, Daniel Vetter wrote:
> In the fbdev code we want to do trylocks only to avoid deadlocks and
> other ugly issues. Thus far we've only grabbed the overall modeset
> lock, but that already failed to exclude a pile of potential
> concurrent operations. With proper atomic support this will be worse.
> 
> So add a trylock mode to the modeset locking code which attempts all
> locks only with trylocks, if possible. We need to track this in the
> locking functions themselves and can't restrict this to drivers since
> driver-private w/w mutexes must be treated the same way.
> 
> There's still the issue that other driver private locks aren't handled
> here at all, but well can't have everything. With this we will at
> least not regress, even once atomic allows lots of concurrent kms
> activity.
> 
> Aside: We should move the acquire context to stack-based allocation in
> the callers to get rid of that awful WARN_ON(kmalloc_failed) control
> flow which just blows up when memory is short. But that's material for
> separate patches.
> 
> v2:
> - Fix logic inversion fumble in the fb helper.
> - Add proper kerneldoc.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c    | 10 +++----
>  drivers/gpu/drm/drm_modeset_lock.c | 56 ++++++++++++++++++++++++++++++--------
>  include/drm/drm_modeset_lock.h     |  6 ++++
>  3 files changed, 55 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 3144db9dc0f1..841e039ba028 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -419,11 +419,11 @@ static bool drm_fb_helper_force_kernel_mode(void)
>  		if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  			continue;
>  
> -		/* NOTE: we use lockless flag below to avoid grabbing other
> -		 * modeset locks.  So just trylock the underlying mutex
> -		 * directly:
> +		/*
> +		 * NOTE: Use trylock mode to avoid deadlocks and sleeping in
> +		 * panic context.
>  		 */
> -		if (!mutex_trylock(&dev->mode_config.mutex)) {
> +		if (__drm_modeset_lock_all(dev, true) != 0) {
>  			error = true;
>  			continue;
>  		}

Can we succeed locking config->mutex and connection_mutex inside
__drm_modeset_lock_all(), but then fail to lock one of the CRTC mutexes
in drm_modeset_lock_all_crtcs()?  If so, __drm_modeset_lock_all() will
return -EBUSY, but not drop the locks it acquired, and then it seems
like we can return from the function here without ever dropping locks.


Matt


> @@ -432,7 +432,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
>  		if (ret)
>  			error = true;
>  
> -		mutex_unlock(&dev->mode_config.mutex);
> +		drm_modeset_unlock_all(dev);
>  	}
>  	return error;
>  }
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
> index 4d2aa549c614..acfe187609b0 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -57,26 +57,37 @@
>  
>  
>  /**
> - * drm_modeset_lock_all - take all modeset locks
> - * @dev: drm device
> + * __drm_modeset_lock_all - internal helper to grab all modeset locks
> + * @dev: DRM device
> + * @trylock: trylock mode for atomic contexts
>   *
> - * This function takes all modeset locks, suitable where a more fine-grained
> - * scheme isn't (yet) implemented. Locks must be dropped with
> - * drm_modeset_unlock_all.
> + * This is a special version of drm_modeset_lock_all() which can also be used in
> + * atomic contexts. Then @trylock must be set to true.
> + *
> + * Returns:
> + * 0 on success or negative error code on failure.
>   */
> -void drm_modeset_lock_all(struct drm_device *dev)
> +int __drm_modeset_lock_all(struct drm_device *dev,
> +			   bool trylock)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_modeset_acquire_ctx *ctx;
>  	int ret;
>  
> -	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> -	if (WARN_ON(!ctx))
> -		return;
> +	ctx = kzalloc(sizeof(*ctx),
> +		      trylock ? GFP_ATOMIC : GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
>  
> -	mutex_lock(&config->mutex);
> +	if (trylock) {
> +		if (!mutex_trylock(&config->mutex))
> +			return -EBUSY;
> +	} else {
> +		mutex_lock(&config->mutex);
> +	}
>  
>  	drm_modeset_acquire_init(ctx, 0);
> +	ctx->trylock_only = trylock;
>  
>  retry:
>  	ret = drm_modeset_lock(&config->connection_mutex, ctx);
> @@ -95,13 +106,29 @@ retry:
>  
>  	drm_warn_on_modeset_not_all_locked(dev);
>  
> -	return;
> +	return 0;
>  
>  fail:
>  	if (ret == -EDEADLK) {
>  		drm_modeset_backoff(ctx);
>  		goto retry;
>  	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(__drm_modeset_lock_all);
> +
> +/**
> + * drm_modeset_lock_all - take all modeset locks
> + * @dev: drm device
> + *
> + * This function takes all modeset locks, suitable where a more fine-grained
> + * scheme isn't (yet) implemented. Locks must be dropped with
> + * drm_modeset_unlock_all.
> + */
> +void drm_modeset_lock_all(struct drm_device *dev)
> +{
> +	WARN_ON(__drm_modeset_lock_all(dev, false) != 0);
>  }
>  EXPORT_SYMBOL(drm_modeset_lock_all);
>  
> @@ -287,7 +314,12 @@ static inline int modeset_lock(struct drm_modeset_lock *lock,
>  
>  	WARN_ON(ctx->contended);
>  
> -	if (interruptible && slow) {
> +	if (ctx->trylock_only) {
> +		if (!ww_mutex_trylock(&lock->mutex))
> +			return -EBUSY;
> +		else
> +			return 0;
> +	} else if (interruptible && slow) {
>  		ret = ww_mutex_lock_slow_interruptible(&lock->mutex, &ctx->ww_ctx);
>  	} else if (interruptible) {
>  		ret = ww_mutex_lock_interruptible(&lock->mutex, &ctx->ww_ctx);
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index d38e1508f11a..a3f736d24382 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -53,6 +53,11 @@ struct drm_modeset_acquire_ctx {
>  	 * list of held locks (drm_modeset_lock)
>  	 */
>  	struct list_head locked;
> +
> +	/**
> +	 * Trylock mode, use only for panic handlers!
> +	 */
> +	bool trylock_only;
>  };
>  
>  /**
> @@ -123,6 +128,7 @@ struct drm_device;
>  struct drm_crtc;
>  
>  void drm_modeset_lock_all(struct drm_device *dev);
> +int __drm_modeset_lock_all(struct drm_device *dev, bool trylock);
>  void drm_modeset_unlock_all(struct drm_device *dev);
>  void drm_modeset_lock_crtc(struct drm_crtc *crtc);
>  void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795