Re: [PATCH] drm/atomic: protect crtc|connector->state with rcu

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 20-03-17 om 09:18 schreef Daniel Vetter:
> On Fri, Mar 17, 2017 at 05:52:32PM +0100, Maarten Lankhorst wrote:
>> Op 16-03-17 om 21:15 schreef Daniel Vetter:
>>> On Thu, Mar 16, 2017 at 5:09 PM, Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>> Op 16-03-17 om 16:52 schreef Daniel Vetter:
>>>>> The vblank code really wants to look at crtc->state without having to
>>>>> take a ww_mutex. One option might be to take one of the vblank locks
>>>>> right when assigning crtc->state, which would ensure that the vblank
>>>>> code doesn't race and access freed memory.
>>>> I'm not sure this is the right approach for vblank.
>>> It's not, it's just that I've had to resurrect this patch quickly
>>> before leaving and accidentally left the vblank stuff in.
>>>
>>>> crtc->state might not be the same as the current state in case of a nonblocking
>>>> modeset that hasn't even disabled the old crtc yet.
>>>>> But userspace tends to poke the vblank_ioctl to query the current
>>>>> vblank timestamp rather often, and going all in with rcu would help a
>>>>> bit. We're not there yet, but also doesn't really hurt.
>>>>>
>>>>> v2: Maarten needs this to make connector properties atomic, so he can
>>>>> peek at state from the various ->mode_valid hooks.
>>>>>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_atomic.c        | 26 +++++++++++++++++---------
>>>>>  drivers/gpu/drm/drm_atomic_helper.c |  2 +-
>>>>>  include/drm/drm_atomic.h            |  5 +++++
>>>>>  include/drm/drm_connector.h         | 13 ++++++++++++-
>>>>>  include/drm/drm_crtc.h              |  9 ++++++++-
>>>>>  5 files changed, 43 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>> index 9b892af7811a..ba11e31ff9ba 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>> @@ -213,16 +213,10 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
>>>>>  }
>>>>>  EXPORT_SYMBOL(drm_atomic_state_clear);
>>>>>
>>>>> -/**
>>>>> - * __drm_atomic_state_free - free all memory for an atomic state
>>>>> - * @ref: This atomic state to deallocate
>>>>> - *
>>>>> - * This frees all memory associated with an atomic state, including all the
>>>>> - * per-object state for planes, crtcs and connectors.
>>>>> - */
>>>>> -void __drm_atomic_state_free(struct kref *ref)
>>>>> +void ___drm_atomic_state_free(struct rcu_head *rhead)
>>>>>  {
>>>>> -     struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
>>>>> +     struct drm_atomic_state *state =
>>>>> +             container_of(rhead, typeof(*state), rhead);
>>>>>       struct drm_mode_config *config = &state->dev->mode_config;
>>>>>
>>>>>       drm_atomic_state_clear(state);
>>>>> @@ -236,6 +230,20 @@ void __drm_atomic_state_free(struct kref *ref)
>>>>>               kfree(state);
>>>>>       }
>>>>>  }
>>>> whatisRCU.txt:
>>>> "This function invokes func(head) after a grace period has elapsed.
>>>> This invocation might happen from either softirq or process context,
>>>> so the function is not permitted to block."
>>>>
>>>> Looking at
>>>> commit 6f0f02dc56f18760b46dc1bf5b3f7386869d4162
>>>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Date:   Mon Jan 23 21:29:39 2017 +0000
>>>>
>>>>     drm/i915: Move atomic state free from out of fence release
>>>>
>>>> I would say that drm_atomic_state_free would definitely block..
>>>>
>>>> The only thing that makes sense IMO is doing kfree_rcu on the object_states.
>>>> But I don't think RCU is the answer here, it won't protect you against using
>>>> the wrong crtc state.
>>>>
>>>> I think I would try to use the crtc ww_mutex in the vblank code and serialize it to pending commits, if at all possible.
>>> Oops. I guess it should have come with "totally untested". In that
>>> case we need a workter which does a synchronize_rcu() before
>>> releasing. I don't just want to do a simple kfree_rcu() because by
>>> that point we've (partially) destroyed the state alreayd (so it's
>>> already unsafe to access, and special ruels are ugly). And doing it
>>> here before we release anything in the core would avoid the need for
>>> drivers to bother with kfree_rcu().
>>>
>>> I'll try to respin something less obviously buggy tomorrow :-)
>> It will still be buggy tomorrow, since you have no way to know if the current hardware crtc_state is anything like crtc->state.
>>
>> :(
> Maybe I wasnt' clear, so let me retry:
>
> - this approach doesn't work for vblank and crtc state. Agreed. I'll
>   remove all the leftover comments I've forgotten to remove in a hurry.
>
> - the patch itself is broken, so can't be used for connector->state
>   peeking in mode_valid either. That one I'll fix.
>
> Does that make sense?
> -Daniel

Yes, but I'm still not completely convinced it's required for connector state to use RCU. During detect()
we would take the connection_mutex anyway, so I can probably  do that for mode_valid as well.

~Maarten

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