git sha: [Not provided in the patch content] Author: Gaetan Rivet <[email protected]> One line subject: coverage: Add metrics entries for all counters.
This commit adds dedicated metrics entries for coverage counters that can be accessed through a collection. The counters are registered with the metrics framework and made available only in the debug metrics page due to their volume. > diff --git a/lib/automake.mk b/lib/automake.mk > index 0b69354bc4..ff657f3cc0 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -99,6 +99,8 @@ lib_libopenvswitch_la_SOURCES = \ > lib/cooperative-multitasking-private.h \ > lib/coverage.c \ > lib/coverage.h \ > + lib/coverage-metrics.c \ > + lib/coverage-private.h \ > lib/cpu.c \ > lib/cpu.h \ > lib/crc32c.c \ [ ... ] > diff --git a/lib/coverage-metrics.c b/lib/coverage-metrics.c > new file mode 100644 > index 0000000000..38638df54a > --- /dev/null > +++ b/lib/coverage-metrics.c > @@ -0,0 +1,65 @@ > +/* > + * Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > + > +#include "coverage.h" > +#include "coverage-private.h" > +#include "metrics.h" > + > +static void > +do_foreach_coverage_counter(metrics_visitor_cb callback, > + struct metrics_visitor *visitor, > + struct metrics_node *node, > + struct metrics_label *labels, > + size_t n_labels OVS_UNUSED) > +{ > + struct coverage_counter *c; > + size_t i; > + > + ovs_mutex_lock(&coverage_mutex); > + for (i = 0; i < n_coverage_counters; i++) { > + c = coverage_counters[i]; Would it make sense to check whether coverage_counters[i] is NULL before dereferencing? The coverage_counters array might contain null pointers if counters were unregistered or if the array was not properly initialized. > + labels[0].value = c->name; > + visitor->it = c; > + callback(visitor, node); > + } > + ovs_mutex_unlock(&coverage_mutex); > +} [ ... ] > +static void > +coverage_metrics_read(double *value, void *c_) > +{ > + struct coverage_counter *c = c_; > + > + c->total += c->count(); > + *value = c->total; > +} Does this function handle the case where c or c->count might be NULL? The c_ parameter is cast directly without null checking. Is the c->total field being modified here thread-safe? The function modifies c->total but doesn't appear to hold the coverage_mutex during this operation, while do_foreach_coverage_counter does hold the mutex. [ ... ] > diff --git a/lib/coverage.c b/lib/coverage.c > index a95b6aa255..7f2a1296ae 100644 > --- a/lib/coverage.c > +++ b/lib/coverage.c > @@ -16,6 +16,7 @@ > > #include <config.h> > #include "coverage.h" > +#include "coverage-private.h" > #include <inttypes.h> > #include <stdlib.h> > #include "openvswitch/dynamic-string.h" > @@ -29,11 +30,11 @@ > VLOG_DEFINE_THIS_MODULE(coverage); > > /* The coverage counters. */ > -static struct coverage_counter **coverage_counters = NULL; > -static size_t n_coverage_counters = 0; > -static size_t allocated_coverage_counters = 0; > +struct coverage_counter **coverage_counters = NULL; > +size_t n_coverage_counters = 0; > +size_t allocated_coverage_counters = 0; Would it be better to provide accessor functions rather than exposing these global variables directly? This would help maintain better encapsulation and allow for future changes to the internal structure. > -static struct ovs_mutex coverage_mutex = OVS_MUTEX_INITIALIZER; > +struct ovs_mutex coverage_mutex = OVS_MUTEX_INITIALIZER; > > DEFINE_STATIC_PER_THREAD_DATA(long long int, coverage_clear_time, LLONG_MIN); > static long long int coverage_run_time = LLONG_MIN; [ ... ] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
