Re: linux-next: manual merge of the drm tree with the drm-intel-fixes tree

From: Paul McKenney <paulmckrcu@gmail.com>
To: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics <intel-gfx@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>,
	DRI <dri-devel@lists.freedesktop.org>,
	Jani Nikula <jani.nikula@intel.com>,
	linux-next@vger.kernel.org
Subject: Re: linux-next: manual merge of the drm tree with the drm-intel-fixes tree
Date: Mon, 27 Mar 2017 10:14:10 -0700	[thread overview]
Message-ID: <CAJzB8QG8bE1onedcBLJPcwQwVH4BrpNcWNzpH00JMfDqgyzObQ@mail.gmail.com> (raw)
In-Reply-To: <20170322110050.1e2ea77d@canb.auug.org.au>

[-- Attachment #1.1: Type: text/plain, Size: 2157 bytes --]

> On Tue, Mar 21, 2017 at 5:00 PM, Stephen Rothwell <sfr@canb.auug.org.au>
wrote:
> Hi Dave,
>
> Today's linux-next merge of the drm tree got a conflict in:
>
>   drivers/gpu/drm/i915/i915_gem_shrinker.c
>
> between commit:
>
>   3d3d18f086cd ("drm/i915: Avoid rcu_barrier() from reclaim paths
(shrinker)")
>
> from the drm-intel-fixes tree and commit:
>
>   519d52498156 ("drm/i915: i915_gem_shrink_all() needs an awake device")
>
> from the drm tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging.  You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
>
> --
> Cheers,
> Stephen Rothwell
>
> diff --cc drivers/gpu/drm/i915/i915_gem_shrinker.c
> index d5d2b4c6ed38,006a8b908f77..000000000000
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@@ -263,7 -264,9 +264,9 @@@ unsigned long i915_gem_shrink_all(struc
>                                 I915_SHRINK_BOUND |
>                                 I915_SHRINK_UNBOUND |
>                                 I915_SHRINK_ACTIVE);
> +       intel_runtime_pm_put(dev_priv);
> +
>  -      rcu_barrier(); /* wait until our RCU delayed slab frees are
completed */
>  +      synchronize_rcu(); /* wait for our earlier RCU delayed slab frees
*/

Unless I am really confused, the RCU delayed slab frees are via call_rcu().
This means that synchronize_rcu() is -not- guaranteed to wait on them.
For example, CPU 0 might be making its way through a pile of callbacks,
including some RCU delayed slab free callbacks.  If there are enough
callbacks, it could take CPU 0 several grace periods to work through the
backlog.  CPU 1 might have no callbacks queued, and might execute the
above code.  Now synchronize_rcu() will wait for a grace period, but only
one, and not necessarily for CPU 0's backlog to drain.

I do not believe that the above fix is safe.

>         return freed;
>   }
>

[-- Attachment #1.2: Type: text/html, Size: 4202 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel