__mutex_lock_common() unlikely very likely

__mutex_lock_common() unlikely very likely

[Steven Rostedt]

Chris,

My branch tracer flagged the unlikely in __mutex_lock_common() as
always hit. That's the:

	if (use_ww_ctx) {
		[...]
		if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
			return -EALREADY;
	}

This is hit 100% of the time, and its coming from the drm logic:

for example:

<stack trace>
 => drm_atomic_get_crtc_state
 => drm_atomic_helper_duplicate_state
 => intel_modeset_init
 => i915_driver_load
 => i915_pci_probe
 => local_pci_probe
 => pci_device_probe
 => driver_probe_device
 => __driver_attach
 => bus_for_each_dev
 => driver_attach
 => bus_add_driver
 => driver_register
 => __pci_register_driver
 => ext4_has_free_clusters
 => do_one_initcall
 => do_init_module
 => load_module
 => SYSC_init_module
 => SyS_init_module
 => entry_SYSCALL_64_fastpath

This is happening on 3 boxes of mine running normal loads (servers,
email, facebook, etc).

Commit 0422e83d84ae2 says:

    Recursive locking for ww_mutexes was originally conceived as an
    exception. However, it is heavily used by the DRM atomic modesetting
    code. Currently, the recursive deadlock is checked after we have queued
    up for a busy-spin and as we never release the lock, we spin until
    kicked, whereupon the deadlock is discovered and reported.

Should this be converted to a likely?

-- Steve

Re: __mutex_lock_common() unlikely very likely

[Chris Wilson]

On Wed, Jan 18, 2017 at 03:58:24PM -0500, Steven Rostedt wrote:
> Chris,
> 
> My branch tracer flagged the unlikely in __mutex_lock_common() as
> always hit. That's the:
> 
> 	if (use_ww_ctx) {
> 		[...]
> 		if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
> 			return -EALREADY;
> 	}
> 
> This is hit 100% of the time, and its coming from the drm logic:

By design this is an exceptional case. In practice, drm modesetting is a
little slapsidasical when it comes to locking. However, it is the
minority use case, just that on intel, the more prevalent users do not
hit this path - though they will with the ww_mutex refactoring work. ttm
drivers (amdgpu, nouveau etc) will be demonstrating that this is the
unlikely branch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: __mutex_lock_common() unlikely very likely

[Steven Rostedt]

On Thu, 19 Jan 2017 08:55:07 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Jan 18, 2017 at 03:58:24PM -0500, Steven Rostedt wrote:
> > Chris,
> > 
> > My branch tracer flagged the unlikely in __mutex_lock_common() as
> > always hit. That's the:
> > 
> > 	if (use_ww_ctx) {
> > 		[...]
> > 		if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
> > 			return -EALREADY;
> > 	}
> > 
> > This is hit 100% of the time, and its coming from the drm logic:  
> 
> By design this is an exceptional case. In practice, drm modesetting is a
> little slapsidasical when it comes to locking. However, it is the
> minority use case, just that on intel, the more prevalent users do not
> hit this path - though they will with the ww_mutex refactoring work. ttm
> drivers (amdgpu, nouveau etc) will be demonstrating that this is the
> unlikely branch.
>

Then I suggest that we remove the unlikely, as it's only "unlikely" if
you have the right hardware. If you don't (and I appear to have three
boxes that don't) then it becomes very likely.

"unlikely" is not about how likely you have the right hardware. It's
about high likely the logic is. If it's a hardware issue, it shouldn't
have a likely or unlikely attached to it.

I'll send a patch to nuke it. When drm is no longer a special case,
because it's a popular platform, we can add it back.

-- Steve

[PATCH] mutex: Remove ww_ctx unlikely() from __mutex_lock_common()

[Steven Rostedt (VMware)]

The unikely() used in __mutex_lock_common() when use_ww_ctx is set is
currently dependent on the hardware if it is likely or unlikely. The
intel drm code calls into this function and triggers this branch 100%
of the time. As this hardware is very commonly used, this is not a rare
case at all (the three boxes I tested this on, all triggered it).

The likely/unlikely annotation should be used for logical cases that
cause it to mostly be hit or not, to let gcc optimize for a certain
case. If hardware causes it to be the opposite, then the hint is
punishing some hardware over other hardware, and no hint should be
placed at all.

I added a comment stating that the branch should be unlikely, but due
to the intel drm logic, it currently isn't. Then if drm changes in the
future, we could then try it again.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index a70b90d..577bb74 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -513,7 +513,11 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 
 	if (use_ww_ctx) {
 		struct ww_mutex *ww = container_of(lock, struct ww_mutex, base);
-		if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
+		/*
+		 * This really should be an unlikely() but currently
+		 * the intel drm makes this a very likely case.
+		 */
+		if (ww_ctx == READ_ONCE(ww->ctx))
 			return -EALREADY;
 	}
 

Re: [PATCH] mutex: Remove ww_ctx unlikely() from __mutex_lock_common()

[Peter Zijlstra]

On Thu, Jan 19, 2017 at 08:54:41AM -0500, Steven Rostedt (VMware) wrote:
> I added a comment stating that the branch should be unlikely, but due
> to the intel drm logic, it currently isn't. Then if drm changes in the
> future, we could then try it again.

I really don't see the point here. The unlikely() also conveys this is
not a fast path branch and that is still true, regardsless of what
runtime does.

Also, the patch wouldn't apply even if I were so inclined.

> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index a70b90d..577bb74 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -513,7 +513,11 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>  
>  	if (use_ww_ctx) {
>  		struct ww_mutex *ww = container_of(lock, struct ww_mutex, base);
> -		if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
> +		/*
> +		 * This really should be an unlikely() but currently
> +		 * the intel drm makes this a very likely case.
> +		 */
> +		if (ww_ctx == READ_ONCE(ww->ctx))
>  			return -EALREADY;
>  	}
>  

Re: [PATCH] mutex: Remove ww_ctx unlikely() from __mutex_lock_common()

[Steven Rostedt]

On Thu, 19 Jan 2017 17:42:52 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jan 19, 2017 at 08:54:41AM -0500, Steven Rostedt (VMware) wrote:
> > I added a comment stating that the branch should be unlikely, but due
> > to the intel drm logic, it currently isn't. Then if drm changes in the
> > future, we could then try it again.  
> 
> I really don't see the point here. The unlikely() also conveys this is
> not a fast path branch and that is still true, regardsless of what
> runtime does.

Fair enough.

> 
> Also, the patch wouldn't apply even if I were so inclined.

Probably because I created it against the ancient 4.9-rc7 kernel ;-)

-- Steve