Git SHA: (Unable to extract - this appears to be from a patch email, not a
commit)
Author: Gaetan Rivet <[email protected]>
Subject: dpif-netdev-perf: Fix histogram initialization
This patch fixes integer overflow and incorrect arithmetic precedence in
histogram initialization functions that could result in decreasing bucket
values and incorrect wall calculations.
> diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c
> index 1cd4ee0842..0d029ff00e 100644
> --- a/lib/dpif-netdev-perf.c
> +++ b/lib/dpif-netdev-perf.c
> @@ -102,22 +102,26 @@ pmd_perf_estimate_tsc_frequency(void)
>
> /* Histogram functions. */
>
> -static void
> +void
> histogram_walls_set_lin(struct histogram *hist, uint32_t min, uint32_t max)
> {
> - int i;
> + uint32_t i, inc;
>
> ovs_assert(min < max);
> + inc = (max - min) / (NUM_BINS - 2);
> for (i = 0; i < NUM_BINS-1; i++) {
> - hist->wall[i] = min + (i * (max - min)) / (NUM_BINS - 2);
> + hist->wall[i] = min + (i * inc);
Does this calculation overflow when i approaches NUM_BINS-1 and inc is
large?
> + }
> + if (max != UINT32_MAX) {
> + hist->wall[NUM_BINS - 2] = max;
> }
> hist->wall[NUM_BINS-1] = UINT32_MAX;
> }
[ ... ]
> @@ -139,7 +143,7 @@ histogram_walls_set_log(struct histogram *hist, uint32_t
> min, uint32_t max)
> wall = MAX(wall, exp(log_min + (i * (log_max - log_min)) /
> (bins-1)));
> hist->wall[start + i] = wall++;
> }
> - if (hist->wall[NUM_BINS-2] < max) {
> + if (hist->wall[NUM_BINS - 2] < max && max != UINT32_MAX) {
> hist->wall[NUM_BINS-2] = max;
> }
> hist->wall[NUM_BINS-1] = UINT32_MAX;
[ ... ]
> diff --git a/tests/test-histogram.c b/tests/test-histogram.c
> new file mode 100644
> index 0000000000..d43f8d7012
> --- /dev/null
> +++ b/tests/test-histogram.c
> @@ -0,0 +1,155 @@
> +/*
> + * 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 "dpif-netdev-perf.h"
> +#include "openvswitch/util.h"
> +#include "ovstest.h"
> +#include "random.h"
> +#include "util.h"
Is the random.h include used anywhere in this file?
> +
> +#define FUZZ(v) { v == 0 ? 0 : v - 1, v, v == UINT32_MAX ? v : v + 1 }
[ ... ]
> +static void
> +test_histogram_check(struct histogram *hist,
> + uint32_t min, uint32_t max,
> + bool fuzzy)
> +{
> + enum { EQ, LT };
> + bool (*ops[])(uint32_t, uint32_t) = {
> + [EQ] = fuzzy ? fuzzy_eq : eq,
> + [LT] = fuzzy ? fuzzy_lt : lt,
> + };
> + bool min_found = false, max_found = false;
> +
> + for (size_t i = 0; i < NUM_BINS; i++) {
> + if (ops[EQ](hist->wall[i], min)) {
> + min_found = true;
> + }
> + if (hist->wall[i] == max) {
> + max_found = true;
> + }
> + }
> + ovs_assert(min_found);
> + ovs_assert(max_found);
Will max_found always be true when max equals UINT32_MAX, given that the
last wall entry is always set to UINT32_MAX in both initialization
functions?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
