Gitweb: http://git.fedorahosted.org/git/?p=lvm2.git;a=commitdiff;h=b7ab402cc23bfa78d... Commit: b7ab402cc23bfa78d3a138e611ded77d3f67bb0c Parent: 4cc88a734841b6a57421202b1eb5c9abcbcc5916 Author: Bryn M. Reeves bmr@redhat.com AuthorDate: Fri Aug 21 12:17:34 2015 +0100 Committer: Bryn M. Reeves bmr@redhat.com CommitterDate: Fri Aug 21 12:17:34 2015 +0100
libdm: fix histogram memory allocation
The histogram table is currently grown onto the end of an area's counters struct. This cannot work with the current area addressing scheme: since we are using array member access to find an area's counters in a region's area table we cannot change the size of the memory area used to store the counters: doing so causes histogram data to appear in the subsequent region's counter struct.
Keeping the current memory allocation scheme would involve changing every site that accesses an areas table to have it use the correct multiplier when calculating an area's offset in the table. This works since the size of the histogram table is the same for every area within a region.
Avoid this by using a separate pool for the histogram allocations: this allows us to grow the histogram and counter tables at the same time without their interfering with one another. --- libdm/libdm-stats.c | 72 +++++++++++++++++++++++++++++++------------------- 1 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/libdm/libdm-stats.c b/libdm/libdm-stats.c index c5ed4c5..858b52d 100644 --- a/libdm/libdm-stats.c +++ b/libdm/libdm-stats.c @@ -50,7 +50,7 @@ struct dm_stats_counters { uint64_t total_read_nsecs; /* Total time spent reading in milliseconds */ uint64_t total_write_nsecs; /* Total time spent writing in milliseconds */ uint64_t histogram_sum; /* Sum of histogram bin counts. */ - struct dm_stats_hist_bin histogram[0]; /* Histogram entries. */ + struct dm_stats_hist_bin *histogram; /* Histogram entries. */ };
struct dm_stats_region { @@ -74,6 +74,7 @@ struct dm_stats { char *uuid; /* device-mapper UUID */ char *program_id; /* default program_id for this handle */ struct dm_pool *mem; /* memory pool for region and counter tables */ + struct dm_pool *hist_mem; /* separate pool for histogram tables */ uint64_t nr_regions; /* total number of present regions */ uint64_t max_region; /* size of the regions table */ uint64_t interval_ns; /* sampling interval in nanoseconds */ @@ -110,13 +111,19 @@ static char *_program_id_from_proc(void)
struct dm_stats *dm_stats_create(const char *program_id) { + size_t hist_hint = sizeof(struct dm_stats_hist_bin); struct dm_stats *dms = NULL;
if (!(dms = dm_zalloc(sizeof(*dms)))) return_NULL; + + /* FIXME: better hint. */ if (!(dms->mem = dm_pool_create("stats_pool", 4096))) goto_out;
+ if (!(dms->hist_mem = dm_pool_create("histogram_pool", hist_hint))) + return_0; + if (!program_id || !strlen(program_id)) dms->program_id = _program_id_from_proc(); else @@ -254,6 +261,7 @@ static int _stats_check_precise_timestamps(const struct dm_stats *dms) /* Already checked? */ if (dms->precise) return 1; + return dm_message_supports_precise_timestamps(); }
@@ -767,31 +775,24 @@ out: return 0; }
-static void _set_histogram_sums(struct dm_stats_region *region) -{ - uint64_t i, j, sum; - - for (i = 0; i < _nr_areas_region(region); i++) { - for (j = 0, sum = 0; j < region->hist_bins; j++) - sum += region->counters[i].histogram[j].count; - region->counters[i].histogram_sum = sum; - } -} - /* * Parse histogram data returned from a @stats_print operation. */ -static int _stats_parse_histogram(struct dm_pool *mem, char *hist, +static int _stats_parse_histogram(struct dm_pool *mem, char *hist_str, + struct dm_stats_hist_bin **histogram, + uint64_t *histogram_sum, struct dm_stats_region *region) { static const char *_valid_chars = "0123456789:"; - uint64_t *bounds = region->hist_bounds; + uint64_t sum = 0, *bounds = region->hist_bounds; unsigned bins = region->hist_bins; struct dm_stats_hist_bin cur; const char *c, *v; int i = 0;
- c = hist; + c = hist_str; + + dm_pool_begin_object(mem, sizeof(cur));
do { memset(&cur, 0, sizeof(cur)); @@ -802,7 +803,7 @@ static int _stats_parse_histogram(struct dm_pool *mem, char *hist, goto badchar;
if (*c == ',') { - log_error("Invalid histogram: %s", hist); + log_error("Invalid histogram: %s", hist_str); goto out; } else { const char *val_start = c; @@ -833,6 +834,7 @@ static int _stats_parse_histogram(struct dm_pool *mem, char *hist, UINT64_MAX : bounds[i + 1];
cur.count = this_val; + sum += this_val;
dm_pool_grow_object(mem, &cur, sizeof(cur));
@@ -842,6 +844,10 @@ static int _stats_parse_histogram(struct dm_pool *mem, char *hist,
log_debug("Added region histogram data with %d entries.", region->hist_bins); + + *histogram = dm_pool_end_object(mem); + *histogram_sum = sum; + return 1;
badchar: @@ -850,11 +856,13 @@ out: return 0; }
-static int _stats_parse_region(struct dm_pool *mem, const char *resp, +static int _stats_parse_region(struct dm_stats *dms, const char *resp, struct dm_stats_region *region, uint64_t timescale) { + struct dm_stats_hist_bin *hist = NULL; struct dm_stats_counters cur; + struct dm_pool *mem = dms->mem; FILE *stats_rows = NULL; uint64_t start, len; char row[256]; @@ -937,22 +945,32 @@ static int _stats_parse_region(struct dm_pool *mem, const char *resp, cur.total_write_nsecs *= timescale; }
- if(!dm_pool_grow_object(mem, &cur, sizeof(cur))) - goto_out; - if (region->hist_bins) { /* Find first histogram separator. */ - char *hist_start = strchr(row, ':'); - if (!hist_start) { + char *hist_str = strchr(row, ':'); + if (!hist_str) { log_error("Could not parse histogram value."); goto out; } /* Find space preceding histogram. */ - while (hist_start && *(hist_start - 1) != ' ') - hist_start--; - _stats_parse_histogram(mem, hist_start, region); + while (hist_str && *(hist_str - 1) != ' ') + hist_str--; + + /* Use a separate pool for histogram objects since we + * are growing the area table and each area's histogram + * table simultaneously. + */ + if (!_stats_parse_histogram(dms->hist_mem, hist_str, + &hist, &cur.histogram_sum, + region)) + goto out; }
+ cur.histogram = hist; + + if(!dm_pool_grow_object(mem, &cur, sizeof(cur))) + goto_out; + if (region->start == UINT64_MAX) { region->start = start; region->step = len; /* area size is always uniform. */ @@ -962,7 +980,6 @@ static int _stats_parse_region(struct dm_pool *mem, const char *resp, region->len = (start + len) - region->start; region->timescale = timescale; region->counters = dm_pool_end_object(mem); - _set_histogram_sums(region);
if (fclose(stats_rows)) stack; @@ -1331,7 +1348,7 @@ static int _dm_stats_populate_region(struct dm_stats *dms, uint64_t region_id, if (!_stats_bound(dms)) return_0;
- if (!_stats_parse_region(dms->mem, resp, region, region->timescale)) { + if (!_stats_parse_region(dms, resp, region, region->timescale)) { log_error("Could not parse @stats_print message response."); return 0; } @@ -1399,6 +1416,7 @@ void dm_stats_destroy(struct dm_stats *dms) _stats_regions_destroy(dms); _stats_clear_binding(dms); dm_pool_destroy(dms->mem); + dm_pool_destroy(dms->hist_mem); if (dms->histogram) dm_free(dms->histogram); dm_free(dms->program_id);
lvm2-commits@lists.fedorahosted.org
-
Bryn Reeves