From 130f3e4d850f4bc7387cfb8d08aa993d288a67a9 Mon Sep 17 00:00:00 2001 From: Ramy El Garawany Date: Thu, 15 Dec 2022 22:03:59 +0000 Subject: [PATCH] Fixing scheduler order number DCHECK when called from ShouldYield. Fixes https://crbug.com/1400202. The unit test's code is the simplest explanation: ShouldYield is called from a task that hasn't released its fence, FindNextTaskFromRoot asserts because the sequence's next task order_num is greater than the wait fence's order number. This is harmless if there is only scheduler thread (because the recursion into FindNextFromRoot immediately terminates because the sequence is already running). Leaving the check until I find the root cause of https://crbug.com/1400203. Bug: 1400202 Change-Id: I459a06c0725552df3855dbad6db67833bfeafcc5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4110028 Auto-Submit: Ramy El Garawany Commit-Queue: Sunny Sachanandani Reviewed-by: Sunny Sachanandani Cr-Commit-Position: refs/heads/main@{#1084008} --- gpu/command_buffer/service/scheduler_dfs.cc | 16 ++--- .../service/scheduler_dfs_unittest.cc | 63 ++++++++++++++++--- 2 files changed, 64 insertions(+), 15 deletions(-) diff --git a/gpu/command_buffer/service/scheduler_dfs.cc b/gpu/command_buffer/service/scheduler_dfs.cc index d21e69eedac043..d18158198c7f65 100644 --- a/gpu/command_buffer/service/scheduler_dfs.cc +++ b/gpu/command_buffer/service/scheduler_dfs.cc @@ -576,11 +576,13 @@ SchedulerDfs::Sequence* SchedulerDfs::FindNextTaskFromRoot( << " (order_num: " << fence_iter->first.order_num << ")."; Sequence* release_sequence = GetSequence(fence_iter->first.release_sequence_id); - // Sanity check to make sure we're not processing a sequence on another - // thread that's past - DCHECK(!(release_sequence && release_sequence->HasTasks() && - release_sequence->tasks_.front().order_num >= - fence_iter->first.order_num)); + // ShouldYield might be calling this function, and a dependency might depend + // on the calling sequence, which might have not released its fences yet. + if (release_sequence && release_sequence->HasTasks() && + release_sequence->tasks_.front().order_num >= + fence_iter->first.order_num) { + continue; + } if (Sequence* result = FindNextTaskFromRoot(release_sequence); result != nullptr) { return result; @@ -617,7 +619,7 @@ SchedulerDfs::Sequence* SchedulerDfs::FindNextTask() { // dependency tied to another thread. for (const SchedulingState& state : sorted_sequences) { Sequence* root_sequence = GetSequence(state.sequence_id); - DVLOG(10) << "RunNextTask: Calling FindNextTask on sequence " + DVLOG(10) << "FindNextTask: Calling FindNextTaskFromRoot on sequence " << root_sequence->sequence_id().value(); if (Sequence* sequence = FindNextTaskFromRoot(root_sequence); sequence != nullptr) { @@ -767,9 +769,7 @@ void SchedulerDfs::ExecuteSequence(const SequenceId sequence_id) { total_blocked_time_ += blocked_time; // Reset pointers after reaquiring the lock. - thread_state = &per_thread_state_map_[task_runner]; sequence = GetSequence(sequence_id); - if (sequence) { sequence->FinishTask(); } diff --git a/gpu/command_buffer/service/scheduler_dfs_unittest.cc b/gpu/command_buffer/service/scheduler_dfs_unittest.cc index ec9eda680202a2..1bb1828e38ac59 100644 --- a/gpu/command_buffer/service/scheduler_dfs_unittest.cc +++ b/gpu/command_buffer/service/scheduler_dfs_unittest.cc @@ -15,7 +15,6 @@ #include "base/functional/bind.h" #include "base/test/scoped_feature_list.h" #include "base/test/task_environment.h" -#include "base/test/test_simple_task_runner.h" #include "base/time/time.h" #include "gpu/command_buffer/service/scheduler.h" #include "gpu/command_buffer/service/sync_point_manager.h" @@ -171,9 +170,9 @@ class SchedulerDfsTaskRunOrderTest : public SchedulerDfsTest { sync_point_manager()->CreateSyncPointClientState( kNamespaceId, command_buffer_id, sequence_id); - sequence_info_.emplace(std::make_pair( + sequence_info_.emplace( sequence_key, - SequenceInfo(sequence_id, command_buffer_id, release_state))); + SequenceInfo(sequence_id, command_buffer_id, release_state)); } void CreateExternalSequence(int sequence_key) { @@ -182,9 +181,9 @@ class SchedulerDfsTaskRunOrderTest : public SchedulerDfsTest { auto release_state = sync_point_manager()->CreateSyncPointClientState( kNamespaceId, command_buffer_id, order_data->sequence_id()); - sequence_info_.emplace(std::make_pair( + sequence_info_.emplace( sequence_key, - SequenceInfo(std::move(order_data), command_buffer_id, release_state))); + SequenceInfo(std::move(order_data), command_buffer_id, release_state)); } void DestroySequence(int sequence_key) { @@ -205,9 +204,9 @@ class SchedulerDfsTaskRunOrderTest : public SchedulerDfsTest { ASSERT_TRUE(info_it != sequence_info_.end()); uint64_t release = release_sync + 1; - sync_tokens_.emplace(std::make_pair( + sync_tokens_.emplace( release_sync, - SyncToken(kNamespaceId, info_it->second.command_buffer_id, release))); + SyncToken(kNamespaceId, info_it->second.command_buffer_id, release)); } static void RunExternalTask(base::OnceClosure task, @@ -605,6 +604,56 @@ TEST_F(SchedulerDfsTest, ReleaseSequenceShouldYield) { scheduler()->DestroySequence(sequence_id2); } +// Tests a situation where a sequence's WaitFence has an order number less than +// the sequence's first order number, because the sequence is currently running, +// and called ShouldYield before release the WaitFence. +TEST_F(SchedulerDfsTest, ShouldYieldIsValidWhenSequenceReleaseIsPending) { + SequenceId sequence_id1 = + scheduler()->CreateSequenceForTesting(SchedulingPriority::kHigh); + CommandBufferNamespace namespace_id = CommandBufferNamespace::GPU_IO; + CommandBufferId command_buffer_id1 = CommandBufferId::FromUnsafeValue(1); + scoped_refptr release_state1 = + sync_point_manager()->CreateSyncPointClientState( + namespace_id, command_buffer_id1, sequence_id1); + + SequenceId sequence_id2 = + scheduler()->CreateSequenceForTesting(SchedulingPriority::kNormal); + CommandBufferId command_buffer_id2 = CommandBufferId::FromUnsafeValue(2); + scoped_refptr release_state2 = + sync_point_manager()->CreateSyncPointClientState( + namespace_id, command_buffer_id2, sequence_id2); + + SyncToken sync_token1(namespace_id, command_buffer_id1, 1); + SyncToken sync_token2(namespace_id, command_buffer_id2, 2); + + // Job 1.1 doesn't depend on anything. + scheduler()->ScheduleTask( + Scheduler::Task(sequence_id1, GetClosure([&] { + EXPECT_FALSE(scheduler()->ShouldYield(sequence_id1)); + release_state1->ReleaseFenceSync(1); + }), + {})); + + // Job 2.1 depends on Job 1.1. + scheduler()->ScheduleTask(Scheduler::Task(sequence_id2, GetClosure([&] { + release_state2->ReleaseFenceSync( + sync_token2.release_count()); + }), + {sync_token1})); + + // Job 1.2 depends on Job 2.1. + scheduler()->ScheduleTask( + Scheduler::Task(sequence_id1, GetClosure([&] {}), {sync_token2})); + + RunAllPendingTasks(); + + release_state1->Destroy(); + release_state2->Destroy(); + + scheduler()->DestroySequence(sequence_id1); + scheduler()->DestroySequence(sequence_id2); +} + TEST_F(SchedulerDfsTest, ReentrantEnableSequenceShouldNotDeadlock) { SequenceId sequence_id1 = scheduler()->CreateSequenceForTesting(SchedulingPriority::kHigh);