On 7/13/26 02:36, Andrey Zhadchenko wrote:
> A region currently tracks a single latency histogram, whose boundaries
> and per-bucket counters are stored directly in struct dm_stat. In order
> to later add histograms that only account I/O of a particular direction,
> generalize the storage so that a region can hold several independent
> histograms.
>
> Each histogram is now described by struct dm_stat_histogram, carrying its
> own boundaries, entry count and an offset into the per-entry bucket array.
> The per-entry bucket array is the concatenation of every histogram's
> buckets, and histogram_buckets holds the total. All allocation, clearing,
> summing, printing and listing loops are updated to iterate over
> the histograms and their individual sizes.
>
> This is a preparatory change with no functional difference.
>
> https://virtuozzo.atlassian.net/browse/VSTOR-103846
> Signed-off-by: Andrey Zhadchenko <[email protected]>
> ---
> drivers/md/dm-stats.c | 160 +++++++++++++++++++++++++++++-------------
> 1 file changed, 110 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/md/dm-stats.c b/drivers/md/dm-stats.c
> index 7a465b5e36cd3..21109ccfa10a9 100644
> --- a/drivers/md/dm-stats.c
> +++ b/drivers/md/dm-stats.c
> @@ -39,6 +39,14 @@ struct dm_stat_shared {
> struct dm_stat_percpu tmp;
> };
>
> +#define DM_STAT_MAX_HISTOGRAMS 1
> +
> +struct dm_stat_histogram {
> + unsigned int n_entries;
> + unsigned int offset;
> + unsigned long long *boundaries;
> +};
> +
> struct dm_stat {
> struct list_head list_entry;
> int id;
> @@ -47,8 +55,9 @@ struct dm_stat {
> sector_t start;
> sector_t end;
> sector_t step;
> - unsigned int n_histogram_entries;
> - unsigned long long *histogram_boundaries;
> + unsigned int n_histograms;
> + unsigned int histogram_buckets;
> + struct dm_stat_histogram histograms[DM_STAT_MAX_HISTOGRAMS];
> const char *program_id;
> const char *aux_data;
> struct rcu_head rcu_head;
> @@ -173,9 +182,11 @@ static void dm_kvfree(void *ptr, size_t alloc_size)
> static void dm_stat_free(struct rcu_head *head)
> {
> int cpu;
> + unsigned int i;
> struct dm_stat *s = container_of(head, struct dm_stat, rcu_head);
>
> - kfree(s->histogram_boundaries);
> + for (i = 0; i < s->n_histograms; i++)
> + kfree(s->histograms[i].boundaries);
> kfree(s->program_id);
> kfree(s->aux_data);
> for_each_possible_cpu(cpu) {
> @@ -262,8 +273,8 @@ static void dm_stats_recalc_precise_timestamps(struct
> dm_stats *stats)
>
> static int dm_stats_create(struct dm_stats *stats, sector_t start, sector_t
> end,
> sector_t step, unsigned int stat_flags,
> - unsigned int n_histogram_entries,
> - unsigned long long *histogram_boundaries,
> + unsigned int n_histograms,
> + struct dm_stat_histogram *histograms,
> const char *program_id, const char *aux_data,
> void (*suspend_callback)(struct mapped_device *),
> void (*resume_callback)(struct mapped_device *),
> @@ -276,6 +287,8 @@ static int dm_stats_create(struct dm_stats *stats,
> sector_t start, sector_t end,
> size_t shared_alloc_size;
> size_t percpu_alloc_size;
> size_t histogram_alloc_size;
> + unsigned int histogram_buckets = 0;
> + unsigned int hn;
> struct dm_stat_percpu *p;
> int cpu;
> int ret_id;
> @@ -284,6 +297,9 @@ static int dm_stats_create(struct dm_stats *stats,
> sector_t start, sector_t end,
> if (end < start || !step)
> return -EINVAL;
>
> + for (hn = 0; hn < n_histograms; hn++)
> + histogram_buckets += histograms[hn].n_entries + 1;
> +
> n_entries = end - start;
> if (dm_sector_div64(n_entries, step))
> n_entries++;
> @@ -302,15 +318,17 @@ static int dm_stats_create(struct dm_stats *stats,
> sector_t start, sector_t end,
> if (percpu_alloc_size / sizeof(struct dm_stat_percpu) != n_entries)
> return -EOVERFLOW;
>
> - histogram_alloc_size = (n_histogram_entries + 1) * (size_t)n_entries *
> sizeof(unsigned long long);
> - if (histogram_alloc_size / (n_histogram_entries + 1) !=
> (size_t)n_entries * sizeof(unsigned long long))
> + histogram_alloc_size = histogram_buckets * (size_t)n_entries *
> sizeof(unsigned long long);
> + if (histogram_buckets &&
> + histogram_alloc_size / histogram_buckets
> + != (size_t)n_entries * sizeof(unsigned long long))
> return -EOVERFLOW;
>
> - if ((n_histogram_entries + 1) * (size_t)n_entries >
> DM_STAT_MAX_HISTOGRAM_ENTRIES)
> + if (histogram_buckets * (size_t)n_entries >
> DM_STAT_MAX_HISTOGRAM_ENTRIES)
> return -EOVERFLOW;
>
> if ((stat_flags & STAT_HIST_TOTAL_LATENCY) &&
> - (!n_histogram_entries || !(stat_flags & STAT_PRECISE_TIMESTAMPS)))
> + (!histogram_buckets || !(stat_flags & STAT_PRECISE_TIMESTAMPS)))
> return -EINVAL;
>
> if (!check_shared_memory(shared_alloc_size + histogram_alloc_size +
> @@ -330,12 +348,20 @@ static int dm_stats_create(struct dm_stats *stats,
> sector_t start, sector_t end,
> s->percpu_alloc_size = percpu_alloc_size;
> s->histogram_alloc_size = histogram_alloc_size;
>
> - s->n_histogram_entries = n_histogram_entries;
> - s->histogram_boundaries = kmemdup(histogram_boundaries,
> - s->n_histogram_entries *
> sizeof(unsigned long long), GFP_KERNEL);
> - if (!s->histogram_boundaries) {
> - r = -ENOMEM;
> - goto out;
> + s->n_histograms = n_histograms;
> + s->histogram_buckets = histogram_buckets;
> + for (hn = 0; hn < n_histograms; hn++) {
> + s->histograms[hn].n_entries = histograms[hn].n_entries;
> + s->histograms[hn].offset = (hn ? s->histograms[hn - 1].offset +
> + s->histograms[hn - 1].n_entries + 1
> : 0);
> + s->histograms[hn].boundaries =
> kmemdup(histograms[hn].boundaries,
> + histograms[hn].n_entries
> *
> + sizeof(unsigned long
> long),
> + GFP_KERNEL);
> + if (!s->histograms[hn].boundaries) {
> + r = -ENOMEM;
> + goto out;
> + }
> }
>
> s->program_id = kstrdup(program_id, GFP_KERNEL);
> @@ -355,7 +381,7 @@ static int dm_stats_create(struct dm_stats *stats,
> sector_t start, sector_t end,
> cond_resched();
> }
>
> - if (s->n_histogram_entries) {
> + if (s->histogram_buckets) {
> unsigned long long *hi;
>
> hi = dm_kvzalloc(s->histogram_alloc_size, NUMA_NO_NODE);
> @@ -365,7 +391,7 @@ static int dm_stats_create(struct dm_stats *stats,
> sector_t start, sector_t end,
> }
> for (ni = 0; ni < n_entries; ni++) {
> s->stat_shared[ni].tmp.histogram = hi;
> - hi += s->n_histogram_entries + 1;
> + hi += s->histogram_buckets;
> cond_resched();
> }
> }
> @@ -377,7 +403,7 @@ static int dm_stats_create(struct dm_stats *stats,
> sector_t start, sector_t end,
> goto out;
> }
> s->stat_percpu[cpu] = p;
> - if (s->n_histogram_entries) {
> + if (s->histogram_buckets) {
> unsigned long long *hi;
>
> hi = dm_kvzalloc(s->histogram_alloc_size,
> cpu_to_node(cpu));
> @@ -387,7 +413,7 @@ static int dm_stats_create(struct dm_stats *stats,
> sector_t start, sector_t end,
> }
> for (ni = 0; ni < n_entries; ni++) {
> p[ni].histogram = hi;
> - hi += s->n_histogram_entries + 1;
> + hi += s->histogram_buckets;
> cond_resched();
> }
> }
> @@ -498,6 +524,7 @@ static int dm_stats_list(struct dm_stats *stats, const
> char *program,
> struct dm_stat *s;
> sector_t len;
> unsigned int sz = 0;
> + unsigned int hn;
>
> /*
> * Output format:
> @@ -518,14 +545,15 @@ static int dm_stats_list(struct dm_stats *stats, const
> char *program,
> DMEMIT(" precise_timestamps");
> if (s->stat_flags & STAT_HIST_TOTAL_LATENCY)
> DMEMIT(" hist_total_latency");
> - if (s->n_histogram_entries) {
> + for (hn = 0; hn < s->n_histograms; hn++) {
> + struct dm_stat_histogram *h =
> &s->histograms[hn];
> unsigned int i;
>
> DMEMIT(" histogram:");
> - for (i = 0; i < s->n_histogram_entries; i++) {
> + for (i = 0; i < h->n_entries; i++) {
> if (i)
> DMEMIT(",");
> - DMEMIT("%llu",
> s->histogram_boundaries[i]);
> + DMEMIT("%llu", h->boundaries[i]);
> }
> }
> DMEMIT("\n");
> @@ -568,6 +596,30 @@ static void dm_stat_round(struct dm_stat *s, struct
> dm_stat_shared *shared,
> shared->stamp = now;
> }
>
> +
> +static void dm_stats_bin_duration(struct dm_stat *s, struct dm_stat_percpu
> *p,
> + unsigned long long duration)
> +{
> + unsigned int hn;
> +
> +
> + for (hn = 0; hn < s->n_histograms; hn++) {
> + struct dm_stat_histogram *h = &s->histograms[hn];
> + unsigned int lo = 0, hi = h->n_entries + 1;
> +
> + while (lo + 1 < hi) {
> + unsigned int mid = (lo + hi) / 2;
> +
> + if (h->boundaries[mid - 1] > duration)
> + hi = mid;
> + else
> + lo = mid;
> + }
> + p->histogram[h->offset + lo]++;
> + }
> +}
I feel like this binary search is over complicated with all +1/-1 magic.
Maybe (note: I made it on top of the series):
@@ -655,17 +655,17 @@ static void dm_stats_bin_duration(struct dm_stat *s,
struct dm_stat_percpu *p,
{
do {
struct dm_stat_histogram *h = &s->histograms[__ffs(hmask)];
- unsigned int lo = 0, hi = h->n_entries + 1;
+ unsigned int lo = 0, hi = h->n_entries;
hmask &= hmask - 1;
- while (lo + 1 < hi) {
+ while (lo < hi) {
unsigned int mid = (lo + hi) / 2;
- if (h->boundaries[mid - 1] > duration)
+ if (h->boundaries[mid] > duration)
hi = mid;
else
- lo = mid;
+ lo = mid + 1;
}
p->histogram[h->offset + lo]++;
} while (hmask);
Should probably be an equivalent.
But now I see it comes from upstream, so probably we can leave as is.
> +
> +
> static void dm_stat_for_entry(struct dm_stat *s, size_t entry,
> blk_opf_t bi_opf, sector_t len,
> struct dm_stats_aux *stats_aux, bool end,
> @@ -619,22 +671,12 @@ static void dm_stat_for_entry(struct dm_stat *s, size_t
> entry,
> p->ticks[idx] += stats_aux->duration_ns;
> duration = stats_aux->duration_ns;
> }
> - if (s->n_histogram_entries) {
> - unsigned int lo = 0, hi = s->n_histogram_entries + 1;
> -
> + if (s->n_histograms) {
> if ((s->stat_flags & STAT_HIST_TOTAL_LATENCY) &&
> stats_aux->histogram_duration_ns)
> duration = stats_aux->histogram_duration_ns;
>
> - while (lo + 1 < hi) {
> - unsigned int mid = (lo + hi) / 2;
> -
> - if (s->histogram_boundaries[mid - 1] > duration)
> - hi = mid;
> - else
> - lo = mid;
> - }
> - p->histogram[lo]++;
> + dm_stats_bin_duration(s, p, duration);
> }
> }
>
> @@ -756,8 +798,8 @@ static void __dm_stat_init_temporary_percpu_totals(struct
> dm_stat_shared *shared
> shared->tmp.io_ticks_total = 0;
> shared->tmp.time_in_queue = 0;
>
> - if (s->n_histogram_entries)
> - memset(shared->tmp.histogram, 0, (s->n_histogram_entries + 1) *
> sizeof(unsigned long long));
> + if (s->histogram_buckets)
> + memset(shared->tmp.histogram, 0, s->histogram_buckets *
> sizeof(unsigned long long));
>
> for_each_possible_cpu(cpu) {
> p = &s->stat_percpu[cpu][x];
> @@ -773,10 +815,10 @@ static void
> __dm_stat_init_temporary_percpu_totals(struct dm_stat_shared *shared
> shared->tmp.io_ticks[WRITE] += READ_ONCE(p->io_ticks[WRITE]);
> shared->tmp.io_ticks_total += READ_ONCE(p->io_ticks_total);
> shared->tmp.time_in_queue += READ_ONCE(p->time_in_queue);
> - if (s->n_histogram_entries) {
> + if (s->histogram_buckets) {
> unsigned int i;
>
> - for (i = 0; i < s->n_histogram_entries + 1; i++)
> + for (i = 0; i < s->histogram_buckets; i++)
> shared->tmp.histogram[i] +=
> READ_ONCE(p->histogram[i]);
> }
> }
> @@ -808,10 +850,10 @@ static void __dm_stat_clear(struct dm_stat *s, size_t
> idx_start, size_t idx_end,
> p->io_ticks_total -= shared->tmp.io_ticks_total;
> p->time_in_queue -= shared->tmp.time_in_queue;
> local_irq_enable();
> - if (s->n_histogram_entries) {
> + if (s->histogram_buckets) {
> unsigned int i;
>
> - for (i = 0; i < s->n_histogram_entries + 1; i++) {
> + for (i = 0; i < s->histogram_buckets; i++) {
> local_irq_disable();
> p = &s->stat_percpu[smp_processor_id()][x];
> p->histogram[i] -= shared->tmp.histogram[i];
> @@ -874,6 +916,7 @@ static int dm_stats_print(struct dm_stats *stats, int id,
> size_t x;
> sector_t start, end, step;
> size_t idx_end;
> + unsigned int hn;
> struct dm_stat_shared *shared;
>
> /*
> @@ -924,11 +967,13 @@ static int dm_stats_print(struct dm_stats *stats, int
> id,
> dm_jiffies_to_msec64(s, shared->tmp.time_in_queue),
> dm_jiffies_to_msec64(s, shared->tmp.io_ticks[READ]),
> dm_jiffies_to_msec64(s, shared->tmp.io_ticks[WRITE]));
> - if (s->n_histogram_entries) {
> + for (hn = 0; hn < s->n_histograms; hn++) {
> + struct dm_stat_histogram *h = &s->histograms[hn];
> unsigned int i;
>
> - for (i = 0; i < s->n_histogram_entries + 1; i++)
> - DMEMIT("%s%llu", !i ? " " : ":",
> shared->tmp.histogram[i]);
> + for (i = 0; i < h->n_entries + 1; i++)
> + DMEMIT("%s%llu", !i ? " " : ":",
> + shared->tmp.histogram[h->offset + i]);
> }
> DMEMIT("\n");
>
> @@ -1013,6 +1058,21 @@ static int parse_histogram(const char *h, unsigned int
> *n_histogram_entries,
> }
> }
>
> +static int add_histogram(struct dm_stat_histogram *histograms,
> + unsigned int *n_histograms, const char *h)
> +{
> + unsigned int idx;
> +
> + if (*n_histograms >= DM_STAT_MAX_HISTOGRAMS)
> + return -EINVAL;
> +
> + idx = (*n_histograms)++;
> + histograms[idx].boundaries = NULL;
> +
> + return parse_histogram(h, &histograms[idx].n_entries,
> + &histograms[idx].boundaries);
> +}
> +
> static int message_stats_create(struct mapped_device *md,
> unsigned int argc, char **argv,
> char *result, unsigned int maxlen)
> @@ -1024,11 +1084,12 @@ static int message_stats_create(struct mapped_device
> *md,
> unsigned int divisor;
> const char *program_id, *aux_data;
> unsigned int stat_flags = 0;
> - unsigned int n_histogram_entries = 0;
> - unsigned long long *histogram_boundaries = NULL;
> + unsigned int n_histograms = 0;
> + struct dm_stat_histogram histograms[DM_STAT_MAX_HISTOGRAMS];
> struct dm_arg_set as, as_backup;
> const char *a;
> unsigned int feature_args;
> + unsigned int i;
>
> /*
> * Input format:
> @@ -1081,9 +1142,7 @@ static int message_stats_create(struct mapped_device
> *md,
> else if (!strcasecmp(a, "hist_total_latency"))
> stat_flags |= STAT_HIST_TOTAL_LATENCY;
> else if (!strncasecmp(a, "histogram:", 10)) {
> - if (n_histogram_entries)
> - goto ret_einval;
> - r = parse_histogram(a + 10,
> &n_histogram_entries, &histogram_boundaries);
> + r = add_histogram(histograms, &n_histograms, a
> + 10);
> if (r)
> goto ret;
> } else
> @@ -1120,7 +1179,7 @@ static int message_stats_create(struct mapped_device
> *md,
> }
>
> id = dm_stats_create(dm_get_stats(md), start, end, step, stat_flags,
> - n_histogram_entries, histogram_boundaries,
> program_id, aux_data,
> + n_histograms, histograms, program_id, aux_data,
> dm_internal_suspend_fast, dm_internal_resume_fast,
> md);
> if (id < 0) {
> r = id;
> @@ -1135,7 +1194,8 @@ static int message_stats_create(struct mapped_device
> *md,
> ret_einval:
> r = -EINVAL;
> ret:
> - kfree(histogram_boundaries);
> + for (i = 0; i < n_histograms; i++)
> + kfree(histograms[i].boundaries);
> return r;
> }
>
--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
