diff options
author | Android Build Coastguard Worker <android-build-coastguard-worker@google.com> | 2023-06-08 17:22:35 +0000 |
---|---|---|
committer | Android Build Coastguard Worker <android-build-coastguard-worker@google.com> | 2023-06-08 17:22:35 +0000 |
commit | 0d01677d14d0336df0a6d32026e37ffda26b80f0 (patch) | |
tree | a4691dc04f3247f0b418c626e9f12fc9de890f3d | |
parent | ad7ce43b1e6e77cb70bf4e961be86ab8cdc1e56b (diff) | |
parent | 53a8558d57cef212315484797092a82245f0586d (diff) | |
download | perfetto-0d01677d14d0336df0a6d32026e37ffda26b80f0.tar.gz |
Snap for 10276566 from 53a8558d57cef212315484797092a82245f0586d to tm-platform-releaseandroid-platform-13.0.0_r9android-platform-13.0.0_r8android-platform-13.0.0_r19android-platform-13.0.0_r18android-platform-13.0.0_r17android-platform-13.0.0_r16android-platform-13.0.0_r15android-platform-13.0.0_r14android-platform-13.0.0_r13android-platform-13.0.0_r12android-platform-13.0.0_r11android-platform-13.0.0_r10android13-platform-release
Change-Id: I19cfac1ddd78f4936dcc7e02a8b44528af7de118
-rw-r--r-- | include/perfetto/ext/ipc/host.h | 3 | ||||
-rw-r--r-- | protos/perfetto/config/perfetto_config.proto | 4 | ||||
-rw-r--r-- | protos/perfetto/config/trace_config.proto | 4 | ||||
-rw-r--r-- | protos/perfetto/trace/perfetto_trace.proto | 4 | ||||
-rw-r--r-- | src/android_stats/perfetto_atoms.h | 1 | ||||
-rw-r--r-- | src/ipc/host_impl.cc | 9 | ||||
-rw-r--r-- | src/ipc/host_impl.h | 4 | ||||
-rw-r--r-- | src/perfetto_cmd/perfetto_cmd.cc | 4 | ||||
-rw-r--r-- | src/profiling/common/producer_support.cc | 21 | ||||
-rw-r--r-- | src/traced/service/builtin_producer.cc | 23 | ||||
-rw-r--r-- | src/traced/service/builtin_producer.h | 2 | ||||
-rw-r--r-- | src/tracing/core/tracing_service_impl.cc | 20 | ||||
-rw-r--r-- | src/tracing/core/tracing_service_impl_unittest.cc | 25 | ||||
-rw-r--r-- | src/tracing/ipc/service/service_ipc_host_impl.cc | 14 |
14 files changed, 129 insertions, 9 deletions
diff --git a/include/perfetto/ext/ipc/host.h b/include/perfetto/ext/ipc/host.h index a9b5824b9..0f8533ee3 100644 --- a/include/perfetto/ext/ipc/host.h +++ b/include/perfetto/ext/ipc/host.h @@ -59,6 +59,9 @@ class Host { // case of errors (e.g., another service with the same name is already // registered). virtual bool ExposeService(std::unique_ptr<Service>) = 0; + + // Overrides the default send timeout for the per-connection sockets. + virtual void SetSocketSendTimeoutMs(uint32_t timeout_ms) = 0; }; } // namespace ipc diff --git a/protos/perfetto/config/perfetto_config.proto b/protos/perfetto/config/perfetto_config.proto index 00fee9061..3bc791895 100644 --- a/protos/perfetto/config/perfetto_config.proto +++ b/protos/perfetto/config/perfetto_config.proto @@ -1918,6 +1918,10 @@ message TraceConfig { // On R-, this override only affected userdebug builds. Since S, it also // affects user builds. optional uint64 max_upload_per_day_bytes = 1; + + // Overrides the guardrail for maximum trace buffer size. + // Available on U+ + optional uint32 max_tracing_buffer_size_kb = 2; } optional GuardrailOverrides guardrail_overrides = 11; diff --git a/protos/perfetto/config/trace_config.proto b/protos/perfetto/config/trace_config.proto index be1ba8237..4fc271724 100644 --- a/protos/perfetto/config/trace_config.proto +++ b/protos/perfetto/config/trace_config.proto @@ -208,6 +208,10 @@ message TraceConfig { // On R-, this override only affected userdebug builds. Since S, it also // affects user builds. optional uint64 max_upload_per_day_bytes = 1; + + // Overrides the guardrail for maximum trace buffer size. + // Available on U+ + optional uint32 max_tracing_buffer_size_kb = 2; } optional GuardrailOverrides guardrail_overrides = 11; diff --git a/protos/perfetto/trace/perfetto_trace.proto b/protos/perfetto/trace/perfetto_trace.proto index 58a0f68e6..7a80660bb 100644 --- a/protos/perfetto/trace/perfetto_trace.proto +++ b/protos/perfetto/trace/perfetto_trace.proto @@ -1918,6 +1918,10 @@ message TraceConfig { // On R-, this override only affected userdebug builds. Since S, it also // affects user builds. optional uint64 max_upload_per_day_bytes = 1; + + // Overrides the guardrail for maximum trace buffer size. + // Available on U+ + optional uint32 max_tracing_buffer_size_kb = 2; } optional GuardrailOverrides guardrail_overrides = 11; diff --git a/src/android_stats/perfetto_atoms.h b/src/android_stats/perfetto_atoms.h index 75cbbd837..14740ae80 100644 --- a/src/android_stats/perfetto_atoms.h +++ b/src/android_stats/perfetto_atoms.h @@ -70,6 +70,7 @@ enum class PerfettoStatsdAtom { kTracedStartTracingInvalidSessionState = 36, kTracedEnableTracingInvalidFilter = 47, kTracedEnableTracingOobTargetBuffer = 48, + kTracedEnableTracingInvalidTriggerMode = 52, // Checkpoints inside perfetto_cmd after tracing has finished. kOnTracingDisabled = 4, diff --git a/src/ipc/host_impl.cc b/src/ipc/host_impl.cc index c182cd17e..1bf234835 100644 --- a/src/ipc/host_impl.cc +++ b/src/ipc/host_impl.cc @@ -116,6 +116,12 @@ bool HostImpl::ExposeService(std::unique_ptr<Service> service) { return true; } +void HostImpl::SetSocketSendTimeoutMs(uint32_t timeout_ms) { + PERFETTO_DCHECK_THREAD(thread_checker_); + // Should be less than the watchdog period (30s). + socket_tx_timeout_ms_ = timeout_ms; +} + void HostImpl::OnNewIncomingConnection( base::UnixSocket*, std::unique_ptr<base::UnixSocket> new_conn) { @@ -125,8 +131,7 @@ void HostImpl::OnNewIncomingConnection( clients_by_socket_[new_conn.get()] = client.get(); client->id = client_id; client->sock = std::move(new_conn); - // Watchdog is 30 seconds, so set the socket timeout to 10 seconds. - client->sock->SetTxTimeout(10000); + client->sock->SetTxTimeout(socket_tx_timeout_ms_); clients_[client_id] = std::move(client); } diff --git a/src/ipc/host_impl.h b/src/ipc/host_impl.h index ac0a59319..de7d07e29 100644 --- a/src/ipc/host_impl.h +++ b/src/ipc/host_impl.h @@ -32,6 +32,8 @@ namespace perfetto { namespace ipc { +constexpr uint32_t kDefaultIpcTxTimeoutMs = 10000; + class HostImpl : public Host, public base::UnixSocket::EventListener { public: HostImpl(const char* socket_name, base::TaskRunner*); @@ -40,6 +42,7 @@ class HostImpl : public Host, public base::UnixSocket::EventListener { // Host implementation. bool ExposeService(std::unique_ptr<Service>) override; + void SetSocketSendTimeoutMs(uint32_t timeout_ms) override; // base::UnixSocket::EventListener implementation. void OnNewIncomingConnection(base::UnixSocket*, @@ -88,6 +91,7 @@ class HostImpl : public Host, public base::UnixSocket::EventListener { std::map<base::UnixSocket*, ClientConnection*> clients_by_socket_; ServiceID last_service_id_ = 0; ClientID last_client_id_ = 0; + uint32_t socket_tx_timeout_ms_ = kDefaultIpcTxTimeoutMs; PERFETTO_THREAD_CHECKER(thread_checker_) base::WeakPtrFactory<HostImpl> weak_ptr_factory_; // Keep last. }; diff --git a/src/perfetto_cmd/perfetto_cmd.cc b/src/perfetto_cmd/perfetto_cmd.cc index 3ed31406f..aafc8445d 100644 --- a/src/perfetto_cmd/perfetto_cmd.cc +++ b/src/perfetto_cmd/perfetto_cmd.cc @@ -1002,7 +1002,9 @@ void PerfettoCmd::OnConnect() { } PERFETTO_DCHECK(trace_config_); - trace_config_->set_enable_extra_guardrails(save_to_incidentd_); + trace_config_->set_enable_extra_guardrails( + (save_to_incidentd_ || report_to_android_framework_) && + !ignore_guardrails_); // Set the statsd logging flag if we're uploading diff --git a/src/profiling/common/producer_support.cc b/src/profiling/common/producer_support.cc index 8ab0b42a6..a16014997 100644 --- a/src/profiling/common/producer_support.cc +++ b/src/profiling/common/producer_support.cc @@ -110,10 +110,25 @@ bool CanProfileAndroid(const DataSourceConfig& ds_config, return true; } + bool trusted_initiator = ds_config.session_initiator() == + DataSourceConfig::SESSION_INITIATOR_TRUSTED_SYSTEM; + uint64_t uid_without_profile = uid % kAidUserOffset; uint64_t uid_for_lookup = 0; - if (uid_without_profile >= kAidAppStart && - uid_without_profile <= kAidAppEnd) { + if (uid_without_profile < kAidAppStart) { + // Platform processes are considered profileable by the platform itself. + // This includes platform UIDs from other profiles, e.g. "u10_system". + // It's possible that this is an app (e.g. com.android.settings runs as + // AID_SYSTEM), but we will skip checking packages.list for the profileable + // manifest flags, as running under a platform UID is considered sufficient. + // Minor consequence: shell cannot profile platform apps, even if their + // manifest flags opt into profiling from shell. Resolving this would + // require definitively disambiguating native processes from apps if both + // can run as the same platform UID. + return trusted_initiator; + + } else if (uid_without_profile >= kAidAppStart && + uid_without_profile <= kAidAppEnd) { // normal app uid_for_lookup = uid_without_profile; @@ -133,8 +148,6 @@ bool CanProfileAndroid(const DataSourceConfig& ds_config, // even be the package in which the service was defined). // TODO(rsavitski): find a way for the platform to tell native services // about isolated<->app relations. - bool trusted_initiator = ds_config.session_initiator() == - DataSourceConfig::SESSION_INITIATOR_TRUSTED_SYSTEM; return trusted_initiator && AllPackagesProfileableByTrustedInitiator(packages_list_path); diff --git a/src/traced/service/builtin_producer.cc b/src/traced/service/builtin_producer.cc index a2007beb5..0664b5d0d 100644 --- a/src/traced/service/builtin_producer.cc +++ b/src/traced/service/builtin_producer.cc @@ -46,9 +46,11 @@ namespace { constexpr char kHeapprofdDataSourceName[] = "android.heapprofd"; constexpr char kJavaHprofDataSourceName[] = "android.java_hprof"; +constexpr char kJavaHprofOomDataSourceName[] = "android.java_hprof.oom"; constexpr char kTracedPerfDataSourceName[] = "linux.perf"; constexpr char kLazyHeapprofdPropertyName[] = "traced.lazy.heapprofd"; constexpr char kLazyTracedPerfPropertyName[] = "traced.lazy.traced_perf"; +constexpr char kJavaHprofOomActivePropertyName[] = "traced.oome_heap_session.count"; } // namespace @@ -64,6 +66,8 @@ BuiltinProducer::~BuiltinProducer() { SetAndroidProperty(kLazyHeapprofdPropertyName, ""); if (!lazy_traced_perf_.instance_ids.empty()) SetAndroidProperty(kLazyTracedPerfPropertyName, ""); + if (!java_hprof_oome_instances_.empty()) + SetAndroidProperty(kJavaHprofOomActivePropertyName, ""); } void BuiltinProducer::ConnectInProcess(TracingService* svc) { @@ -102,6 +106,11 @@ void BuiltinProducer::OnConnect() { lazy_traced_perf_dsd.set_name(kTracedPerfDataSourceName); endpoint_->RegisterDataSource(lazy_traced_perf_dsd); } + { + DataSourceDescriptor java_hprof_oome_dsd; + java_hprof_oome_dsd.set_name(kJavaHprofOomDataSourceName); + endpoint_->RegisterDataSource(java_hprof_oome_dsd); + } } void BuiltinProducer::SetupDataSource(DataSourceInstanceID ds_id, @@ -120,6 +129,13 @@ void BuiltinProducer::SetupDataSource(DataSourceInstanceID ds_id, lazy_traced_perf_.instance_ids.emplace(ds_id); return; } + + if (ds_config.name() == kJavaHprofOomDataSourceName) { + java_hprof_oome_instances_.emplace(ds_id); + SetAndroidProperty(kJavaHprofOomActivePropertyName, + std::to_string(java_hprof_oome_instances_.size())); + return; + } } void BuiltinProducer::StartDataSource(DataSourceInstanceID ds_id, @@ -152,6 +168,13 @@ void BuiltinProducer::StopDataSource(DataSourceInstanceID ds_id) { MaybeInitiateLazyStop(ds_id, &lazy_heapprofd_, kLazyHeapprofdPropertyName); MaybeInitiateLazyStop(ds_id, &lazy_traced_perf_, kLazyTracedPerfPropertyName); + + auto oome_it = java_hprof_oome_instances_.find(ds_id); + if (oome_it != java_hprof_oome_instances_.end()) { + java_hprof_oome_instances_.erase(oome_it); + SetAndroidProperty(kJavaHprofOomActivePropertyName, + std::to_string(java_hprof_oome_instances_.size())); + } } void BuiltinProducer::MaybeInitiateLazyStop(DataSourceInstanceID ds_id, diff --git a/src/traced/service/builtin_producer.h b/src/traced/service/builtin_producer.h index 9873458ab..7014d4419 100644 --- a/src/traced/service/builtin_producer.h +++ b/src/traced/service/builtin_producer.h @@ -34,6 +34,7 @@ namespace perfetto { // * perfetto metatrace // * lazy heapprofd daemon starter (android only) // * lazy traced_perf daemon starter (android only) +// * java_hprof oom data source counter (android only) class BuiltinProducer : public Producer { public: BuiltinProducer(base::TaskRunner* task_runner, uint32_t lazy_stop_delay_ms); @@ -83,6 +84,7 @@ class BuiltinProducer : public Producer { MetatraceState metatrace_; LazyAndroidDaemonState lazy_heapprofd_; LazyAndroidDaemonState lazy_traced_perf_; + std::set<DataSourceInstanceID> java_hprof_oome_instances_; base::WeakPtrFactory<BuiltinProducer> weak_factory_; // Keep last. }; diff --git a/src/tracing/core/tracing_service_impl.cc b/src/tracing/core/tracing_service_impl.cc index 807b955d8..93419734e 100644 --- a/src/tracing/core/tracing_service_impl.cc +++ b/src/tracing/core/tracing_service_impl.cc @@ -613,6 +613,15 @@ base::Status TracingServiceImpl::EnableTracing(ConsumerEndpointImpl* consumer, cfg.trigger_config().trigger_timeout_ms()); } + // This check has been introduced in May 2023 after finding b/274931668. + if (static_cast<int>(cfg.trigger_config().trigger_mode()) > + TraceConfig::TriggerConfig::TriggerMode_MAX) { + MaybeLogUploadEvent( + cfg, PerfettoStatsdAtom::kTracedEnableTracingInvalidTriggerMode); + return PERFETTO_SVC_ERR( + "The trace config specified an invalid trigger_mode"); + } + if (has_trigger_config && cfg.duration_ms() != 0) { MaybeLogUploadEvent( cfg, PerfettoStatsdAtom::kTracedEnableTracingDurationWithTrigger); @@ -664,12 +673,16 @@ base::Status TracingServiceImpl::EnableTracing(ConsumerEndpointImpl* consumer, } buf_size_sum += buf.size_kb(); } - if (buf_size_sum > kGuardrailsMaxTracingBufferSizeKb) { + + uint32_t max_tracing_buffer_size_kb = + std::max(kGuardrailsMaxTracingBufferSizeKb, + cfg.guardrail_overrides().max_tracing_buffer_size_kb()); + if (buf_size_sum > max_tracing_buffer_size_kb) { MaybeLogUploadEvent( cfg, PerfettoStatsdAtom::kTracedEnableTracingBufferSizeTooLarge); return PERFETTO_SVC_ERR("Requested too large trace buffer (%" PRIu64 "kB > %" PRIu32 " kB)", - buf_size_sum, kGuardrailsMaxTracingBufferSizeKb); + buf_size_sum, max_tracing_buffer_size_kb); } } @@ -940,6 +953,9 @@ base::Status TracingServiceImpl::EnableTracing(ConsumerEndpointImpl* consumer, tracing_session->config.set_duration_ms( cfg.trigger_config().trigger_timeout_ms()); break; + + // The case of unknown modes (coming from future versions of the service) + // is handled few lines above (search for TriggerMode_MAX). } tracing_session->state = TracingSession::CONFIGURED; diff --git a/src/tracing/core/tracing_service_impl_unittest.cc b/src/tracing/core/tracing_service_impl_unittest.cc index 00b7a3a16..bce99e7ff 100644 --- a/src/tracing/core/tracing_service_impl_unittest.cc +++ b/src/tracing/core/tracing_service_impl_unittest.cc @@ -425,6 +425,31 @@ TEST_F(TracingServiceImplTest, StartTracingTriggerTimeOut) { EXPECT_THAT(consumer->ReadBuffers(), IsEmpty()); } +// Regression test for b/274931668. An unkonwn trigger should not cause a trace +// that runs indefinitely. +TEST_F(TracingServiceImplTest, FailOnUnknownTrigger) { + std::unique_ptr<MockConsumer> consumer = CreateMockConsumer(); + consumer->Connect(svc.get()); + + std::unique_ptr<MockProducer> producer = CreateMockProducer(); + producer->Connect(svc.get(), "mock_producer"); + producer->RegisterDataSource("ds_1"); + + TraceConfig trace_config; + trace_config.add_buffers()->set_size_kb(128); + trace_config.add_data_sources()->mutable_config()->set_name("ds_1"); + auto* trigger_config = trace_config.mutable_trigger_config(); + trigger_config->set_trigger_mode( + static_cast<TraceConfig::TriggerConfig::TriggerMode>( + TraceConfig::TriggerConfig::TriggerMode_MAX + 1)); + auto* trigger = trigger_config->add_triggers(); + trigger->set_name("trigger_from_the_future"); + trigger_config->set_trigger_timeout_ms(1); + + consumer->EnableTracing(trace_config); + consumer->WaitForTracingDisabled(); +} + // Creates a tracing session with a START_TRACING trigger and checks that // the session is not started when the configured trigger producer is different // than the producer that sent the trigger. diff --git a/src/tracing/ipc/service/service_ipc_host_impl.cc b/src/tracing/ipc/service/service_ipc_host_impl.cc index 5618b3cb3..3598bf59e 100644 --- a/src/tracing/ipc/service/service_ipc_host_impl.cc +++ b/src/tracing/ipc/service/service_ipc_host_impl.cc @@ -31,6 +31,10 @@ namespace perfetto { +namespace { +constexpr uint32_t kProducerSocketTxTimeoutMs = 10; +} + // TODO(fmayer): implement per-uid connection limit (b/69093705). // Implements the publicly exposed factory method declared in @@ -85,6 +89,16 @@ bool ServiceIPCHostImpl::DoStart() { return false; } + // Lower the timeout for blocking socket sends to producers as we shouldn't + // normally exhaust the kernel send buffer unless the producer is + // unresponsive. We'll drop the connection if the timeout is hit (see + // UnixSocket::Send). Context in b/236813972, b/193234818. + // Consumer port continues using the default timeout (10s) as there are + // generally fewer consumer processes, and they're better behaved. Also the + // consumer port ipcs might exhaust the send buffer under normal operation + // due to large messages such as ReadBuffersResponse. + producer_ipc_port_->SetSocketSendTimeoutMs(kProducerSocketTxTimeoutMs); + // TODO(fmayer): add a test that destroyes the ServiceIPCHostImpl soon after // Start() and checks that no spurious callbacks are issued. bool producer_service_exposed = producer_ipc_port_->ExposeService( |