From 3327bb4391143a0ae60d45dc1885bb6d89022464 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Wed, 7 Dec 2022 10:10:55 +0000 Subject: [PATCH] histogram_data: check bucket_shift when merging When merging data from one spdk_histogram_data to another, the merging is only valid if the bucket_shift for each structure is the same. Otherwise we are combining data points that cover different ranges of values. So check that the bucket_shifts are the same before merging. Change the return type to int to return -EINVAL if structures with different bucket_shifts are attempted to be merged. Signed-off-by: Jim Harris Change-Id: If98e2d03384d85f478965956da2a42cfcff4713d Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15813 Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot Reviewed-by: Shuhei Matsumoto Reviewed-by: Tomasz Zawadzki --- include/spdk/histogram_data.h | 12 +++++++++++- .../include/spdk/histogram_data.h/histogram_ut.c | 13 ++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/include/spdk/histogram_data.h b/include/spdk/histogram_data.h index e004dc9f0..e43530d4c 100644 --- a/include/spdk/histogram_data.h +++ b/include/spdk/histogram_data.h @@ -179,15 +179,25 @@ spdk_histogram_data_iterate(const struct spdk_histogram_data *histogram, } } -static inline void +static inline int spdk_histogram_data_merge(const struct spdk_histogram_data *dst, const struct spdk_histogram_data *src) { uint64_t i; + /* Histograms with different bucket_shift values cannot be simply + * merged, because the buckets represent different ranges of + * values. + */ + if (dst->bucket_shift != src->bucket_shift) { + return -EINVAL; + } + for (i = 0; i < SPDK_HISTOGRAM_NUM_BUCKETS(dst); i++) { dst->bucket[i] += src->bucket[i]; } + + return 0; } static inline struct spdk_histogram_data * diff --git a/test/unit/include/spdk/histogram_data.h/histogram_ut.c b/test/unit/include/spdk/histogram_data.h/histogram_ut.c index a42e38d7e..9c8d943ce 100644 --- a/test/unit/include/spdk/histogram_data.h/histogram_ut.c +++ b/test/unit/include/spdk/histogram_data.h/histogram_ut.c @@ -82,6 +82,7 @@ histogram_merge(void) struct spdk_histogram_data *h1, *h2; uint64_t *values = g_values; uint32_t i; + int rc; h1 = spdk_histogram_data_alloc(); h2 = spdk_histogram_data_alloc(); @@ -91,7 +92,8 @@ histogram_merge(void) spdk_histogram_data_tally(h2, g_values[i]); } - spdk_histogram_data_merge(h1, h2); + rc = spdk_histogram_data_merge(h1, h2); + CU_ASSERT(rc == 0); g_total = 0; g_number_of_merged_histograms = 2; @@ -99,6 +101,15 @@ histogram_merge(void) spdk_histogram_data_free(h1); spdk_histogram_data_free(h2); + + h1 = spdk_histogram_data_alloc_sized(SPDK_HISTOGRAM_BUCKET_SHIFT_DEFAULT); + h2 = spdk_histogram_data_alloc_sized(SPDK_HISTOGRAM_BUCKET_SHIFT_DEFAULT - 1); + + rc = spdk_histogram_data_merge(h1, h2); + CU_ASSERT(rc == -EINVAL); + + spdk_histogram_data_free(h1); + spdk_histogram_data_free(h2); } int