Git SHA: [email protected] Author: Gaetan Rivet <[email protected]> Subject: histogram: Refactor code to a standalone module
This commit extracts histogram functionality from dpif-netdev-perf into a standalone module, making it available for broader use within the codebase. > diff --git a/lib/automake.mk b/lib/automake.mk > index 78d6e65164..ecc8a64e20 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -148,6 +148,8 @@ lib_libopenvswitch_la_SOURCES = \ > lib/dpif.h \ > lib/heap.c \ > lib/heap.h \ > + lib/histogram.c \ > + lib/histogram.h \ > lib/dynamic-string.c \ > lib/entropy.c \ > lib/entropy.h \ [ ... ] > diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c > index 0d029ff00e..17e3195b17 100644 > --- a/lib/dpif-netdev-perf.c > +++ b/lib/dpif-netdev-perf.c > @@ -19,6 +19,7 @@ > > #include "dpdk.h" > #include "dpif-netdev-perf.h" > +#include "histogram.h" > #include "openvswitch/dynamic-string.h" > #include "openvswitch/vlog.h" > #include "ovs-numa.h" [ ... ] > diff --git a/lib/histogram.c b/lib/histogram.c > new file mode 100644 > index 0000000000..85bb3661c3 > --- /dev/null > +++ b/lib/histogram.c > @@ -0,0 +1,94 @@ > +/* > + * Copyright (c) 2017 Ericsson AB. > + * 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 <stdint.h> > +#include <math.h> > + > +#include "histogram.h" > +#include "openvswitch/util.h" > +#include "util.h" Why are both "openvswitch/util.h" and "util.h" included? The coding style guide recommends avoiding redundant includes. Does this code actually need both headers? > +void > +histogram_clear(struct histogram *hist) > +{ > + int i; > + > + for (i = 0; i < HISTOGRAM_N_BINS; i++) { > + hist->bin[i] = 0; > + } > +} The original histogram_clear function in dpif-netdev-perf.c was static, but this new version is exported. Does the new public visibility match the intended usage for this refactored module? > diff --git a/lib/histogram.h b/lib/histogram.h > new file mode 100644 > index 0000000000..8c2a9ea5c7 > --- /dev/null > +++ b/lib/histogram.h > @@ -0,0 +1,64 @@ > +/* > + * Copyright (c) 2017 Ericsson AB. > + * 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. > + */ > + > +#ifndef HISTOGRAM_H > +#define HISTOGRAM_H 1 > + > +#include <stdint.h> > + > +#ifdef __cplusplus > +extern "C" { > +#endif There appears to be two spaces between #ifdef and __cplusplus. The coding style guide specifies single space indentation for preprocessor directives. Should this be corrected to match the style guide? > +static inline void > +histogram_add_sample(struct histogram *hist, uint32_t val) > +{ > + /* TODO: Can do better with binary search? */ > + for (int i = 0; i < HISTOGRAM_N_BINS - 1; i++) { > + if (val <= hist->wall[i]) { > + hist->bin[i]++; > + return; > + } > + } > + hist->bin[HISTOGRAM_N_BINS - 1]++; > +} Does histogram_add_sample perform bounds checking on the hist parameter? What happens if hist is NULL when this inline function is called? > +#ifdef __cplusplus > +} > +#endif Same spacing issue here - are there two spaces between #ifdef and __cplusplus? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
