Re: __mutex_lock_common() unlikely very likely

[Steven Rostedt]

On Thu, 19 Jan 2017 08:55:07 +
Chris Wilson  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

Re: __mutex_lock_common() unlikely very likely

[Steven Rostedt]

On Thu, 19 Jan 2017 08:55:07 +
Chris Wilson  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

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

[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

__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:


 => 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

__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:


 => 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