[counters] remove "max" functionality It cannot be properly implemented lock-free per core. This can be provided in a lossy way via the usermode kcounter app. ZX-3337 #comment progress Test: see bug above for manual test. Change-Id: I28f035e3aad92791d91b8acf95624ad77602b820
diff --git a/kernel/lib/counters/include/lib/counters.h b/kernel/lib/counters/include/lib/counters.h index c732ea2..bdff755 100644 --- a/kernel/lib/counters/include/lib/counters.h +++ b/kernel/lib/counters/include/lib/counters.h
@@ -25,17 +25,12 @@ // Kernel counters public API: // 1- define a new counter. // KCOUNTER(counter_name, "<counter name>"); -// KCOUNTER_MAX(counter_name, "<counter name>"); // // 2- counters start at zero, increment the counter: // kcounter_add(counter_name, 1); -// or -// kcounter_max(counter_name, value); // // By default with KCOUNTER, the `kcounter` presentation will calculate a -// sum() across cores rather than summing. KCOUNTER_MAX() calculates the max() -// of the counters across cores. -// +// sum() across cores rather than summing. // // Naming the counters // The naming convention is "kernel.subsystem.thing_or_action" @@ -96,15 +91,30 @@ static int64_t arena_page_end_[] __asm__("kcounters_arena_page_end"); }; -class CounterBase { +class Counter { public: - explicit constexpr CounterBase(const counters::Descriptor* desc) : + explicit constexpr Counter(const counters::Descriptor* desc) : desc_(desc) { } int64_t Value() const { return *Slot(); } + void Add(int64_t delta) const { +#if defined(__aarch64__) + // Use a relaxed atomic load/store for arm64 to avoid a potentially + // nasty race between the regular load/store operations for a +1. + // Relaxed atomic load/stores are about as efficient as a regular + // load/store. + atomic_add_64_relaxed(Slot(), delta); +#else + // x86 can do the add in a single non atomic instruction, so the data + // loss of a preemption in the middle of this sequence is fairly + // minimal. + *Slot() += delta; +#endif + } + protected: int64_t* Slot() const { return &get_local_percpu()->counters[Index()]; @@ -120,38 +130,6 @@ const counters::Descriptor* desc_; }; -struct CounterSum : public CounterBase { - explicit constexpr CounterSum(const counters::Descriptor* desc) : - CounterBase(desc) { } - void Add(int64_t delta) const { -#if defined(__aarch64__) - // Use a relaxed atomic load/store for arm64 to avoid a potentially - // nasty race between the regular load/store operations for a +1. - // Relaxed atomic load/stores are about as efficient as a regular - // load/store. - atomic_add_64_relaxed(Slot(), delta); -#else - // x86 can do the add in a single non atomic instruction, so the data - // loss of a preemption in the middle of this sequence is fairly - // minimal. - *Slot() += delta; -#endif - } -}; - -struct CounterMax : public CounterBase { - explicit constexpr CounterMax(const counters::Descriptor* desc) : - CounterBase(desc) { } - void Update(int64_t value) const { - // TODO(travisg|scottmg): Revisit, consider more efficient arm-specific - // instruction sequence here. - int64_t prev_value = atomic_load_64_relaxed(Slot()); - while (prev_value < value && - !atomic_cmpxchg_64_relaxed(Slot(), &prev_value, value)) - ; - } -}; - // Define the descriptor and reserve the arena space for the counters. // Because of -fdata-sections, each kcounter_arena_* array will be // placed in a .bss.kcounter.* section; kernel.ld recognizes those names @@ -164,21 +142,11 @@ namespace { \ __USED int64_t kcounter_arena_##var[SMP_MAX_CPUS] __asm__("kcounter." name); \ alignas(counters::Descriptor) __USED __SECTION("kcountdesc." name) const counters::Descriptor kcounter_desc_##var{name, counters::Type::k##type}; \ - constexpr Counter##type var(&kcounter_desc_##var); \ + constexpr Counter var(&kcounter_desc_##var); \ } // anonymous namespace #define KCOUNTER(var, name) KCOUNTER_DECLARE(var, name, Sum) -#define KCOUNTER_MAX(var, name) KCOUNTER_DECLARE(var, name, Max) -static inline void kcounter_add(const CounterSum& counter, int64_t delta) { +static inline void kcounter_add(const Counter& counter, int64_t delta) { counter.Add(delta); } - -static inline void kcounter_max(const CounterMax& counter, int64_t value) { - counter.Update(value); -} - -static inline void kcounter_max_counter(const CounterMax& counter, - const CounterBase& other) { - counter.Update(other.Value()); -}
diff --git a/kernel/lib/heap/cmpctmalloc/cmpctmalloc.cpp b/kernel/lib/heap/cmpctmalloc/cmpctmalloc.cpp index c798eb7..eb056bb 100644 --- a/kernel/lib/heap/cmpctmalloc/cmpctmalloc.cpp +++ b/kernel/lib/heap/cmpctmalloc/cmpctmalloc.cpp
@@ -116,8 +116,6 @@ #define LOCAL_TRACE 0 -KCOUNTER_MAX(max_allocation, "kernel.heap.max_allocation"); - // Use HEAP_ENABLE_TESTS to enable internal testing. The tests are not useful // when the target system is up. By that time we have done hundreds of allocations // already. @@ -904,9 +902,6 @@ if (size == 0u) { return NULL; } - - kcounter_max(max_allocation, size); - // Large allocations are no longer allowed. See ZX-1318 for details. if (size > (HEAP_LARGE_ALLOC_BYTES - sizeof(header_t))) { return NULL;
diff --git a/kernel/object/handle.cpp b/kernel/object/handle.cpp index ed5f165..a6c1cff 100644 --- a/kernel/object/handle.cpp +++ b/kernel/object/handle.cpp
@@ -24,7 +24,6 @@ KCOUNTER(handle_count_made, "kernel.handles.made"); KCOUNTER(handle_count_duped, "kernel.handles.duped"); KCOUNTER(handle_count_live, "kernel.handles.live"); -KCOUNTER_MAX(handle_count_max_live, "kernel.handles.max_live"); // Masks for building a Handle's base_value, which ProcessDispatcher // uses to create zx_handle_t values. @@ -120,7 +119,6 @@ return nullptr; kcounter_add(handle_count_made, 1); kcounter_add(handle_count_live, 1); - kcounter_max_counter(handle_count_max_live, handle_count_live); return HandleOwner(new (addr) Handle(ktl::move(dispatcher), rights, base_value)); } @@ -141,7 +139,6 @@ return nullptr; kcounter_add(handle_count_duped, 1); kcounter_add(handle_count_live, 1); - kcounter_max_counter(handle_count_max_live, handle_count_live); return HandleOwner(new (addr) Handle(source, rights, base_value)); }
diff --git a/system/utest/kcounter/kcounter-test.cpp b/system/utest/kcounter/kcounter-test.cpp index e8a4c0b..4e8e952 100644 --- a/system/utest/kcounter/kcounter-test.cpp +++ b/system/utest/kcounter/kcounter-test.cpp
@@ -84,16 +84,15 @@ const counters::Descriptor& b) { return strcmp(a.name, b.name) < 0; }); - return (result.first == desc->end() ? nullptr : - &desc->descriptor_table[result.first - desc->begin()]); + return (result.first == result.second) ? + nullptr : &desc->descriptor_table[result.first - desc->begin()]; }; constexpr counters::Descriptor kExpected[] = { + {"kernel.counters.magic", counters::Type::kSum}, {"kernel.handles.duped", counters::Type::kSum}, {"kernel.handles.live", counters::Type::kSum}, {"kernel.handles.made", counters::Type::kSum}, - {"kernel.handles.max_live", counters::Type::kMax}, - {"kernel.counters.magic", counters::Type::kSum}, }; for (const auto& ref : kExpected) { auto found = find(ref); @@ -111,7 +110,7 @@ case counters::Type::kSum: value += cpu_value; break; - case counters::Type::kMax: + case counters::Type::kMax: // Not used, see ZX-3337. value = std::max(value, cpu_value); break; }