[Intel-gfx] [PATCH v2] drm/i915/gt: update request engine before removing virtual GuC engine
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Jul 13 12:11:53 UTC 2023
On 13/07/2023 12:09, Andrzej Hajda wrote:
> Hi,
>
> On 13.07.2023 09:39, Tvrtko Ursulin wrote:
>>
>> On 12/07/2023 19:54, John Harrison wrote:
>>> On 7/12/2023 09:27, Andrzej Hajda wrote:
>>>> On 12.07.2023 14:35, Tvrtko Ursulin wrote:
>>>>> On 12/07/2023 13:18, Andrzej Hajda wrote:
>>>>>> On 11.07.2023 17:27, Tvrtko Ursulin wrote:
>>>>>>> On 11/07/2023 14:58, Andrzej Hajda wrote:
>>>>>>>> On 11.07.2023 13:34, Andi Shyti wrote:
>>>>>>>>> Hi Andrzej,
>>>>>>>>>
>>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11
>>>>>>>>>> +++++++++++
>>>>>>>>>> 1 file changed, 11 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git
>>>>>>>>>> a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>>>>>>>> index a0e3ef1c65d246..2c877ea5eda6f0 100644
>>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>>>>>>>> @@ -3461,6 +3461,8 @@ static void
>>>>>>>>>> guc_prio_fini(struct i915_request *rq, struct intel_context *ce)
>>>>>>>>>> static void remove_from_context(struct i915_request
>>>>>>>>>> *rq)
>>>>>>>>>> {
>>>>>>>>>> struct intel_context *ce =
>>>>>>>>>> request_to_scheduling_context(rq);
>>>>>>>>>> + struct intel_engine_cs *engine;
>>>>>>>>>> + intel_engine_mask_t tmp;
>>>>>>>>>>
>>>>>>>>>> GEM_BUG_ON(intel_context_is_child(ce));
>>>>>>>>>>
>>>>>>>>>> @@ -3478,6 +3480,15 @@ static void
>>>>>>>>>> remove_from_context(struct i915_request *rq)
>>>>>>>>>>
>>>>>>>>>> atomic_dec(&ce->guc_id.ref);
>>>>>>>>>> i915_request_notify_execute_cb_imm(rq);
>>>>>>>>>> +
>>>>>>>>>> + /*
>>>>>>>>>> + * GuC virtual engine can disappear after
>>>>>>>>>> this call, so let's assign
>>>>>>>>>> + * something valid, as driver expects this to
>>>>>>>>>> be always valid pointer.
>>>>>>>>>> + */
>>>>>>>>>> + for_each_engine_masked(engine,
>>>>>>>>>> rq->engine->gt, rq->execution_mask, tmp) {
>>>>>>>>>> + rq->engine = engine;
>>>>>>>>>>
>>>>>>>>>> yes... here the context might lose the virtual engine...
>>>>>>>>>> I wonder
>>>>>>>>>> whether this is the rigth solution, though. Maybe we
>>>>>>>>>> should set
>>>>>>>>>> rq->engine = NULL; and check for NULL? Don't know.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Setting NULL causes occasional null page de-reference in
>>>>>>>>>>
>>>>>>>>>> i915_request_wait_timeout:
>>>>>>>>>>
>>>>>>>>>> mutex_release(&rq->engine->gt->reset.mutex.dep_map, _THIS_IP_)
>>>>>>>>>>
>>>>>>>>>> rq->engine after removing rq from context is (IMHO) used as a
>>>>>>>>>> set of aliases
>>>>>>>>>> for gt and i915 (despite rq itself contains the alias to i915).
>>>>>>>>> without investigating further, but maybe that code is not even
>>>>>>>>> supposed to be executed, at this point, if the request's assigned
>>>>>>>>> virtual engine is removed.
>>>>>>>>
>>>>>>>> Real tests show it is executed and the function
>>>>>>>> i915_request_wait_timeout is quite generic
>>>>>>>> I guess it is quite typical use-case, the only question is about
>>>>>>>> timings - what happens earlier -
>>>>>>>> finalization of i915_request_wait_timeout or context removal.
>>>>>>>>
>>>>>>>> The other point rq->engine is accessed after context removal is
>>>>>>>> i915_fence_release -
>>>>>>>> there is long comment there regarding virtual context and reuse
>>>>>>>> retired rq.
>>>>>>>> Anyway calling there "intel_engine_is_virtual(rq->engine)" is
>>>>>>>> risky without this patch and KASAN complains clearly about it:
>>>>>>>> http://gfx-ci.igk.intel.com/tree/drm-tip/kasan.html?testfilter=gem_exec_balancer
>>>>>>>
>>>>>>> Looks like a bug introduced in bcb9aa45d5a0 ("Revert "drm/i915:
>>>>>>> Hold reference to intel_context over life of i915_request""),
>>>>>>> which was a partial revert of 1e98d8c52ed5 ("drm/i915: Hold
>>>>>>> reference to intel_context over life of i915_request").
>>>>>>>
>>>>>>> Ie. if 1e98d8c52ed5 recognised the problem with disappearing
>>>>>>> rq->engine, then I am confused how bcb9aa45d5a0 left the
>>>>>>> rq->engine dereference in there after removing the extra reference.
>>>>>>>
>>>>>>> Could it be that the intel_engine_is_virtual check simply needs
>>>>>>> to be removed from i915_fence_release, restoring things to how
>>>>>>> they were before 1e98d8c52ed5? Could you try it out?
>>>>>>
>>>>>>
>>>>>> I have already tried something similar [1] and KASAN bugs
>>>>>> disappeared, or more precisely gem_exec_balance tests passed. But
>>>>>> I have been warned by Nirmoy guc virtual engines can be created
>>>>>> for only one real engine (ie. is_power_of_2(rq->execution_mask) is
>>>>>> true but rq->engine points to virtual engine).
>>>>>>
>>>>>> [1]: https://patchwork.freedesktop.org/series/118879/
>>>>>
>>>>> Ugh.. Try involving media umd folks to see if they need a single
>>>>> engine virtual engine? Or we could always just not create it in the
>>>>> driver, I mean just use the physical one.
>>>>
>>>>
>>>> In case there is single physical engine intel_engine_create_virtual
>>>> falls back to intel_context_create (no virtual engine), but in case
>>>> of parallel contexts there is special KMD flag FORCE_VIRTUAL which
>>>> enforces virtual engine even for single physical engine. So it seems
>>>> to be KMD concept.
>>>>
>>>> Anyway is it worth investigating how to make
>>>> "is_power_of_2(rq->execution_mask)" indication of dangling engine
>>>> pointer? It will not help in 1st case:
>>>> mutex_release(&rq->engine->gt->reset.mutex.dep_map, _THIS_IP_)
>>>>
>>>>
>>> There seems to be a fundamental problem here. Object 1 (rq) is
>>> holding a pointer to a reference counted and transient object 2
>>> (engine) but without taking a reference count for itself. That is a
>>> Bad Thing(tm).
>
> Engine is not ref counted (at least directly), hardware engines seems to
> be even persistent across whole life of i915.
> I guess before introduction of virtual engines the assumption about
> validity of rq->engine was correct and developers used it to access
> rq->engine->gt, rq->engine->i915, etc
> So the problems described here are probably leftovers of this change.
> After virtual engines were introduced
> "is_power_of_2(rq->execution_mask)" was used to detect rq->engine is
> virtual.
> And after adding parallel engines it does not seem to be valid check
> anymore.
>
>>> I'm not following the description in the revert patch as to why rq
>>> can't ref count the context/engine. Is there actually a recursive
>>> counting problem? Or is it just a lifetime issue caused by requests
>>> hanging around indefinitely because they are locked by a user process?
>>>
>>> Either way, jumping through convoluted hoops to ensure the code does
>>> not attempt to dereference a dangling pointer seems like the wrong
>>> fix. Removing the engine pointer when the request is completed and no
>>> longer dependent upon an engine (but before the engine can possibly
>>> be destroyed) seems like a much better solution. And then making the
>>> request handling code check for and cope with a null engine pointer.
>>> It sounds like the only problem there is the above mutex, but there
>>> is an alternate route to that? Although why a completed request would
>>> need access to a GT reset mutex seems confusing. If the request is
>>> done, then what connection does it still have to the GT?
>>
>> Agreed in principle but the question is how invasive would it be to
>> change the rules.
>>
>> With the latest info that the issue is really just the GuC _parallel_
>> engine setup, and looking at the code, I wonder if we couldn't just
>> flag the rq->flags with "kernel context request". The code in
>> i915_fence_release claims the rq pool is only relevant for those so it
>> sounds it would be safe to skip everything else based on that new flag.
>>
>> For the mutex_release path, presumable the bad deref is only _after_
>> the wait, right? (Only once the request has been retired.)
>>
>> In which case caching the gt pointer at the start of
>> i915_request_wait_timeout would be sufficient.
>>
>> That should be a few lines fixup overall and then the idea of allowing
>> rq->engine to be reset to NULL can be explored more leisurely.
>
> I guess:
> - setting engine to NULL in remove_from_context,
> - caching gt pointer,
> - checking for null pointer in i915_fence_release
>
> should be enough to solve current issue. However I am not sure if there
> are no more dragons hidden in other execution paths. I will try inspect
> the code.
What you have in this patch, cheat by replacing the engine, *might* work for the short term *if* you can make sure all parallel readers will see the updated rq->engine pointer in time, before the old one gets freed.
For the longer term solution - maybe making the engine reference counted?
Or if setting rq->engine to NULL then evaluating the paths which derefence it. AFAIR these request retirement races have been generally tricky/annoying.
For instance under the i915_request_wait_timeout chain.
1)
__i915_spin_request - could be racy if request gets retired between i915_request_wait_timeout/dma_fence_is_signaled check and __i915_spin_request dereferencing rq->engine.props?
2)
intel_rps_boost - claims to be safe by serialising via i915_request_retire/I915_FENCE_FLAG_BOOST but is it? What prevents retire tearing down the engine between:
if (!test_and_set_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags)) {
---> HERE - if whole retirement happens here <----
struct intel_rps *rps = &READ_ONCE(rq->engine)->gt->rps;
3)
__intel_engine_flush_submission - could be racy to? What if the whole process of consuming the request by the backend and retirement happens between these two lines:
if (i915_request_is_ready(rq))
---> HERE <---
__intel_engine_flush_submission(rq->engine, false);
Then "is ready" can be true, but by the 2nd line request retired and rq->engine freed/NULL already.
Lets hope I am just making unwarranted panic by being to away from this area of the driver for a while. :) But if I am not, then it could be that rq->engine is just overall too problematic and perhaps we should have a look into making it reference counted by the request.
> Btw there is rq->i915 but code only uses "rq->engine->i915" which way
> shall we go? remove former or latter?
Simpler/shorter option should be better.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list