Introduce latency histogram statics for block devices.
For each accounted operation type latency region [0, +inf) is
divided into subregions by several points. Then, calculate
hits for each subregion.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/accounting.h | 9 +++++
block/accounting.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 106 insertions(+)
diff --git a/include/block/accounting.h b/include/block/accounting.h
index b833d26d6c..9679020f64 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -45,6 +45,12 @@ struct BlockAcctTimedStats {
QSLIST_ENTRY(BlockAcctTimedStats) entries;
};
+typedef struct BlockLatencyHistogram {
+ int size;
+ uint64_t *points; /* @size-1 points here (all points, except 0 and +inf) */
+ uint64_t *histogram[BLOCK_MAX_IOTYPE]; /* @size elements for each type */
+} BlockLatencyHistogram;
+
struct BlockAcctStats {
QemuMutex lock;
uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
@@ -57,6 +63,7 @@ struct BlockAcctStats {
QSLIST_HEAD(, BlockAcctTimedStats) intervals;
bool account_invalid;
bool account_failed;
+ BlockLatencyHistogram latency_histogram;
};
typedef struct BlockAcctCookie {
@@ -82,5 +89,7 @@ void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
int64_t block_acct_idle_time_ns(BlockAcctStats *stats);
double block_acct_queue_depth(BlockAcctTimedStats *stats,
enum BlockAcctType type);
+int block_latency_histogram_set(BlockAcctStats *stats, uint64List *latency);
+void block_latency_histogram_clear(BlockAcctStats *stats);
#endif
diff --git a/block/accounting.c b/block/accounting.c
index 87ef5bbfaa..0051ff0c24 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -94,6 +94,100 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
cookie->type = type;
}
+/* block_latency_histogram_compare_func
+ * Compare @key with interval [@el, @el+1), where @el+1 is a next array element
+ * after @el.
+ * Return: -1 if @key < @el
+ * 0 if @key in [@el, @el+1)
+ * +1 if @key >= @el+1
+ */
+static int block_latency_histogram_compare_func(const void *key, const void *el)
+{
+ uint64_t k = *(uint64_t *)key;
+ uint64_t a = *(uint64_t *)el;
+ uint64_t b = *((uint64_t *)el + 1);
+
+ return k < a ? -1 : (k < b ? 0 : 1);
+}
+
+static void block_latency_histogram_account(BlockLatencyHistogram *hist,
+ enum BlockAcctType type,
+ int64_t latency_ns)
+{
+ uint64_t *data, *pos;
+
+ if (hist->points == NULL) {
+ /* histogram disabled */
+ return;
+ }
+
+ data = hist->histogram[type];
+
+ if (latency_ns < hist->points[0]) {
+ data[0]++;
+ return;
+ }
+
+ if (latency_ns >= hist->points[hist->size - 2]) {
+ data[hist->size - 1]++;
+ return;
+ }
+
+ pos = bsearch(&latency_ns, hist->points, hist->size - 2,
+ sizeof(hist->points[0]),
+ block_latency_histogram_compare_func);
+ assert(pos != NULL);
+
+ data[pos - hist->points + 1]++;
+}
+
+int block_latency_histogram_set(BlockAcctStats *stats, uint64List *latency)
+{
+ BlockLatencyHistogram *hist = &stats->latency_histogram;
+ uint64List *entry;
+ uint64_t *ptr;
+ int i;
+ uint64_t prev = 0;
+
+ hist->size = 1;
+
+ for (entry = latency; entry; entry = entry->next) {
+ if (entry->value <= prev) {
+ return -EINVAL;
+ }
+ hist->size++;
+ prev = entry->value;
+ }
+
+ hist->points = g_renew(uint64_t, hist->points, hist->size - 1);
+ for (entry = latency, ptr = hist->points; entry;
+ entry = entry->next, ptr++)
+ {
+ *ptr = entry->value;
+ }
+
+ for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
+ hist->histogram[i] = g_renew(uint64_t, hist->histogram[i], hist->size);
+ memset(hist->histogram[i], 0, hist->size * sizeof(uint64_t));
+ }
+
+ return 0;
+}
+
+void block_latency_histogram_clear(BlockAcctStats *stats)
+{
+ BlockLatencyHistogram *hist = &stats->latency_histogram;
+ int i;
+
+ g_free(hist->points);
+ hist->points = NULL;
+
+ for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
+ g_free(hist->histogram[i]);
+ hist->histogram[i] = NULL;
+ }
+}
+
static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
bool failed)
{
@@ -116,6 +210,9 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
stats->nr_ops[cookie->type]++;
}
+ block_latency_histogram_account(&stats->latency_histogram, cookie->type,
+ latency_ns);
+
if (!failed || stats->account_failed) {
stats->total_time_ns[cookie->type] += latency_ns;
stats->last_access_time_ns = time_ns;
--
2.11.1
07.02.2018 15:50, Vladimir Sementsov-Ogievskiy wrote:
> Introduce latency histogram statics for block devices.
> For each accounted operation type latency region [0, +inf) is
> divided into subregions by several points. Then, calculate
> hits for each subregion.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> include/block/accounting.h | 9 +++++
> block/accounting.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 106 insertions(+)
>
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index b833d26d6c..9679020f64 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -45,6 +45,12 @@ struct BlockAcctTimedStats {
> QSLIST_ENTRY(BlockAcctTimedStats) entries;
> };
>
> +typedef struct BlockLatencyHistogram {
> + int size;
> + uint64_t *points; /* @size-1 points here (all points, except 0 and +inf) */
> + uint64_t *histogram[BLOCK_MAX_IOTYPE]; /* @size elements for each type */
> +} BlockLatencyHistogram;
> +
> struct BlockAcctStats {
> QemuMutex lock;
> uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
> @@ -57,6 +63,7 @@ struct BlockAcctStats {
> QSLIST_HEAD(, BlockAcctTimedStats) intervals;
> bool account_invalid;
> bool account_failed;
> + BlockLatencyHistogram latency_histogram;
> };
>
> typedef struct BlockAcctCookie {
> @@ -82,5 +89,7 @@ void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
> int64_t block_acct_idle_time_ns(BlockAcctStats *stats);
> double block_acct_queue_depth(BlockAcctTimedStats *stats,
> enum BlockAcctType type);
> +int block_latency_histogram_set(BlockAcctStats *stats, uint64List *latency);
> +void block_latency_histogram_clear(BlockAcctStats *stats);
>
> #endif
> diff --git a/block/accounting.c b/block/accounting.c
> index 87ef5bbfaa..0051ff0c24 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -94,6 +94,100 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
> cookie->type = type;
> }
>
> +/* block_latency_histogram_compare_func
> + * Compare @key with interval [@el, @el+1), where @el+1 is a next array element
> + * after @el.
> + * Return: -1 if @key < @el
> + * 0 if @key in [@el, @el+1)
> + * +1 if @key >= @el+1
> + */
> +static int block_latency_histogram_compare_func(const void *key, const void *el)
> +{
> + uint64_t k = *(uint64_t *)key;
> + uint64_t a = *(uint64_t *)el;
> + uint64_t b = *((uint64_t *)el + 1);
> +
> + return k < a ? -1 : (k < b ? 0 : 1);
> +}
> +
> +static void block_latency_histogram_account(BlockLatencyHistogram *hist,
> + enum BlockAcctType type,
> + int64_t latency_ns)
> +{
> + uint64_t *data, *pos;
> +
> + if (hist->points == NULL) {
> + /* histogram disabled */
> + return;
> + }
> +
> + data = hist->histogram[type];
> +
> + if (latency_ns < hist->points[0]) {
> + data[0]++;
> + return;
> + }
> +
> + if (latency_ns >= hist->points[hist->size - 2]) {
> + data[hist->size - 1]++;
> + return;
> + }
> +
> + pos = bsearch(&latency_ns, hist->points, hist->size - 2,
> + sizeof(hist->points[0]),
> + block_latency_histogram_compare_func);
> + assert(pos != NULL);
> +
> + data[pos - hist->points + 1]++;
> +}
> +
> +int block_latency_histogram_set(BlockAcctStats *stats, uint64List *latency)
> +{
> + BlockLatencyHistogram *hist = &stats->latency_histogram;
> + uint64List *entry;
> + uint64_t *ptr;
> + int i;
> + uint64_t prev = 0;
> +
> + hist->size = 1;
bug here. we can fail, so separate variable new_size is needed to
calculate new size.
> +
> + for (entry = latency; entry; entry = entry->next) {
> + if (entry->value <= prev) {
> + return -EINVAL;
> + }
> + hist->size++;
> + prev = entry->value;
> + }
> +
> + hist->points = g_renew(uint64_t, hist->points, hist->size - 1);
> + for (entry = latency, ptr = hist->points; entry;
> + entry = entry->next, ptr++)
> + {
> + *ptr = entry->value;
> + }
> +
> + for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
> + hist->histogram[i] = g_renew(uint64_t, hist->histogram[i], hist->size);
> + memset(hist->histogram[i], 0, hist->size * sizeof(uint64_t));
> + }
> +
> + return 0;
> +}
> +
> +void block_latency_histogram_clear(BlockAcctStats *stats)
> +{
> + BlockLatencyHistogram *hist = &stats->latency_histogram;
> + int i;
> +
> + g_free(hist->points);
> + hist->points = NULL;
> +
> + for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
> + g_free(hist->histogram[i]);
> + hist->histogram[i] = NULL;
> + }
> +}
> +
> static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
> bool failed)
> {
> @@ -116,6 +210,9 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
> stats->nr_ops[cookie->type]++;
> }
>
> + block_latency_histogram_account(&stats->latency_histogram, cookie->type,
> + latency_ns);
> +
> if (!failed || stats->account_failed) {
> stats->total_time_ns[cookie->type] += latency_ns;
> stats->last_access_time_ns = time_ns;
--
Best regards,
Vladimir
On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Introduce latency histogram statics for block devices.
> For each accounted operation type latency region [0, +inf) is
> divided into subregions by several points. Then, calculate
> hits for each subregion.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> include/block/accounting.h | 9 +++++
> block/accounting.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 106 insertions(+)
>
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index b833d26d6c..9679020f64 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -45,6 +45,12 @@ struct BlockAcctTimedStats {
> QSLIST_ENTRY(BlockAcctTimedStats) entries;
> };
>
> +typedef struct BlockLatencyHistogram {
> + int size;
> + uint64_t *points; /* @size-1 points here (all points, except 0 and +inf) */
> + uint64_t *histogram[BLOCK_MAX_IOTYPE]; /* @size elements for each type */
The semantics of these fields is not obvious. Please include a comment
with an example or ASCII-art diagram so anyone reading the code
understands the purpose of the fields without reading all the code.
According to Wikipedia and Mathworld, "intervals" and "bins" are
commonly used terms:
https://en.wikipedia.org/wiki/Histogram
http://mathworld.wolfram.com/Histogram.html
I suggest:
typedef struct {
/* The following histogram is represented like this:
*
* 5| *
* 4| *
* 3| * *
* 2| * * *
* 1| * * * *
* +------------------
* 10 50 100
*
* BlockLatencyHistogram histogram = {
* .nbins = 4,
* .intervals = {10, 50, 100},
* .bins = {3, 1, 5, 2},
* };
*/
size_t nbins;
/* Interval boundaries (there are @nbins - 1 elements) */
uint64_t *intervals;
/* Frequency data (there are @nbins elements) */
uint64_t *bins[BLOCK_MAX_IOTYPE];
} BlockLatencyHistogram;
> +/* block_latency_histogram_compare_func
> + * Compare @key with interval [@el, @el+1), where @el+1 is a next array element
> + * after @el.
> + * Return: -1 if @key < @el
> + * 0 if @key in [@el, @el+1)
> + * +1 if @key >= @el+1
"@el+1" is confusing, usually x+1 means "the value of x plus 1", not
"y" (a different variable!).
I suggest:
/* block_latency_histogram_compare_func:
* Compare @key with interval [@it[0], @it[1]).
* Return: -1 if @key < @it[0]
* 0 if @key in [@it[0], @it[1])
* +1 if @key >= @it[1]
*/
static int block_latency_histogram_compare_func(const void *key, const void *it)
> +int block_latency_histogram_set(BlockAcctStats *stats, uint64List *latency)
> +{
> + BlockLatencyHistogram *hist = &stats->latency_histogram;
> + uint64List *entry;
> + uint64_t *ptr;
> + int i;
> + uint64_t prev = 0;
> +
> + hist->size = 1;
> +
> + for (entry = latency; entry; entry = entry->next) {
> + if (entry->value <= prev) {
> + return -EINVAL;
> + }
> + hist->size++;
> + prev = entry->value;
> + }
> +
> + hist->points = g_renew(uint64_t, hist->points, hist->size - 1);
> + for (entry = latency, ptr = hist->points; entry;
> + entry = entry->next, ptr++)
> + {
> + *ptr = entry->value;
> + }
> +
> + for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
> + hist->histogram[i] = g_renew(uint64_t, hist->histogram[i], hist->size);
> + memset(hist->histogram[i], 0, hist->size * sizeof(uint64_t));
Is there a reason for using g_renew() and then clearing everything?
This is more concise:
g_free(hist->histogram[i]);
hist->histogram[i] = g_new0(uint64_t, hist->size);
On 03/06/2018 09:32 AM, Stefan Hajnoczi wrote:
> On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Introduce latency histogram statics for block devices.
>> For each accounted operation type latency region [0, +inf) is
>> divided into subregions by several points. Then, calculate
>> hits for each subregion.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
> According to Wikipedia and Mathworld, "intervals" and "bins" are
> commonly used terms:
> https://en.wikipedia.org/wiki/Histogram
> http://mathworld.wolfram.com/Histogram.html
>
> I suggest:
>
> typedef struct {
> /* The following histogram is represented like this:
> *
> * 5| *
> * 4| *
> * 3| * *
> * 2| * * *
> * 1| * * * *
> * +------------------
> * 10 50 100
> *
> * BlockLatencyHistogram histogram = {
> * .nbins = 4,
> * .intervals = {10, 50, 100},
> * .bins = {3, 1, 5, 2},
> * };
The name 'intervals' is still slightly ambiguous: does it hold the
boundary point (0-10 for 10 slots, 10-50 for 40 slots, 50-100, for 50
slots, then 100-INF) or is it the interval size of each slot (first bin
is 10 slots for 0-10, next bin is 50 slots wide so 10-60, next bin is
100 slots wide so 60-160, everything else is 160-INF). But the
ascii-art diagram plus the text is sufficient to resolve the intent if
you keep that name (I don't have a suggestion for a better name).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
08.03.2018 20:31, Eric Blake wrote:
> On 03/06/2018 09:32 AM, Stefan Hajnoczi wrote:
>> On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir
>> Sementsov-Ogievskiy wrote:
>>> Introduce latency histogram statics for block devices.
>>> For each accounted operation type latency region [0, +inf) is
>>> divided into subregions by several points. Then, calculate
>>> hits for each subregion.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>
>> According to Wikipedia and Mathworld, "intervals" and "bins" are
>> commonly used terms:
>> https://en.wikipedia.org/wiki/Histogram
>> http://mathworld.wolfram.com/Histogram.html
>>
>> I suggest:
>>
>> typedef struct {
>> /* The following histogram is represented like this:
>> *
>> * 5| *
>> * 4| *
>> * 3| * *
>> * 2| * * *
>> * 1| * * * *
>> * +------------------
>> * 10 50 100
>> *
>> * BlockLatencyHistogram histogram = {
>> * .nbins = 4,
>> * .intervals = {10, 50, 100},
>> * .bins = {3, 1, 5, 2},
>> * };
>
> The name 'intervals' is still slightly ambiguous: does it hold the
> boundary point (0-10 for 10 slots, 10-50 for 40 slots, 50-100, for 50
> slots, then 100-INF) or is it the interval size of each slot (first
> bin is 10 slots for 0-10, next bin is 50 slots wide so 10-60, next bin
> is 100 slots wide so 60-160, everything else is 160-INF). But the
> ascii-art diagram plus the text is sufficient to resolve the intent if
> you keep that name (I don't have a suggestion for a better name).
>
Hm. these numbers are actually boundary points of histogram intervals,
not intervals itself. And, wiki says "The bins are usually specified as
consecutive, non-overlapping intervals of a variable.", so, intervals
are bins.
So, what about:
1. interval_boundaries
2. bin_boundaries
3. boundaries
(and same names with s/_/-/ for qapi)
--
Best regards,
Vladimir
08.03.2018 21:14, Vladimir Sementsov-Ogievskiy wrote:
> 08.03.2018 20:31, Eric Blake wrote:
>> On 03/06/2018 09:32 AM, Stefan Hajnoczi wrote:
>>> On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir
>>> Sementsov-Ogievskiy wrote:
>>>> Introduce latency histogram statics for block devices.
>>>> For each accounted operation type latency region [0, +inf) is
>>>> divided into subregions by several points. Then, calculate
>>>> hits for each subregion.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>
>>> According to Wikipedia and Mathworld, "intervals" and "bins" are
>>> commonly used terms:
>>> https://en.wikipedia.org/wiki/Histogram
>>> http://mathworld.wolfram.com/Histogram.html
>>>
>>> I suggest:
>>>
>>> typedef struct {
>>> /* The following histogram is represented like this:
>>> *
>>> * 5| *
>>> * 4| *
>>> * 3| * *
>>> * 2| * * *
>>> * 1| * * * *
>>> * +------------------
>>> * 10 50 100
>>> *
>>> * BlockLatencyHistogram histogram = {
>>> * .nbins = 4,
>>> * .intervals = {10, 50, 100},
>>> * .bins = {3, 1, 5, 2},
>>> * };
>>
>> The name 'intervals' is still slightly ambiguous: does it hold the
>> boundary point (0-10 for 10 slots, 10-50 for 40 slots, 50-100, for 50
>> slots, then 100-INF) or is it the interval size of each slot (first
>> bin is 10 slots for 0-10, next bin is 50 slots wide so 10-60, next
>> bin is 100 slots wide so 60-160, everything else is 160-INF). But
>> the ascii-art diagram plus the text is sufficient to resolve the
>> intent if you keep that name (I don't have a suggestion for a better
>> name).
>>
>
> Hm. these numbers are actually boundary points of histogram intervals,
> not intervals itself. And, wiki says "The bins are usually specified
> as consecutive, non-overlapping intervals of a variable.", so,
> intervals are bins.
>
> So, what about:
>
> 1. interval_boundaries
> 2. bin_boundaries
> 3. boundaries
>
> (and same names with s/_/-/ for qapi)
>
>
Also, now I doubt, is it a good idea to share same bin boundaries for
each io type.
--
Best regards,
Vladimir
08.03.2018 21:21, Vladimir Sementsov-Ogievskiy wrote:
> 08.03.2018 21:14, Vladimir Sementsov-Ogievskiy wrote:
>> 08.03.2018 20:31, Eric Blake wrote:
>>> On 03/06/2018 09:32 AM, Stefan Hajnoczi wrote:
>>>> On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir
>>>> Sementsov-Ogievskiy wrote:
>>>>> Introduce latency histogram statics for block devices.
>>>>> For each accounted operation type latency region [0, +inf) is
>>>>> divided into subregions by several points. Then, calculate
>>>>> hits for each subregion.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>>> <vsementsov@virtuozzo.com>
>>>>> ---
>>>
>>>> According to Wikipedia and Mathworld, "intervals" and "bins" are
>>>> commonly used terms:
>>>> https://en.wikipedia.org/wiki/Histogram
>>>> http://mathworld.wolfram.com/Histogram.html
>>>>
>>>> I suggest:
>>>>
>>>> typedef struct {
>>>> /* The following histogram is represented like this:
>>>> *
>>>> * 5| *
>>>> * 4| *
>>>> * 3| * *
>>>> * 2| * * *
>>>> * 1| * * * *
>>>> * +------------------
>>>> * 10 50 100
>>>> *
>>>> * BlockLatencyHistogram histogram = {
>>>> * .nbins = 4,
>>>> * .intervals = {10, 50, 100},
>>>> * .bins = {3, 1, 5, 2},
>>>> * };
>>>
>>> The name 'intervals' is still slightly ambiguous: does it hold the
>>> boundary point (0-10 for 10 slots, 10-50 for 40 slots, 50-100, for
>>> 50 slots, then 100-INF) or is it the interval size of each slot
>>> (first bin is 10 slots for 0-10, next bin is 50 slots wide so 10-60,
>>> next bin is 100 slots wide so 60-160, everything else is 160-INF).
>>> But the ascii-art diagram plus the text is sufficient to resolve the
>>> intent if you keep that name (I don't have a suggestion for a better
>>> name).
>>>
>>
>> Hm. these numbers are actually boundary points of histogram
>> intervals, not intervals itself. And, wiki says "The bins are usually
>> specified as consecutive, non-overlapping intervals of a variable.",
>> so, intervals are bins.
>>
>> So, what about:
>>
>> 1. interval_boundaries
>> 2. bin_boundaries
>> 3. boundaries
>>
>> (and same names with s/_/-/ for qapi)
>>
>>
>
>
> Also, now I doubt, is it a good idea to share same bin boundaries for
> each io type.
>
so, for qmp command, what about:
boundaries - optional, default boundaries for all io operations
boundaries-read - boundaries for read
boundaries-write - boundaries for write
...
so, call without any boundaries: drop _all_ histograms
call with only specific boundaries-*: set or reset _only_ corresponding
specific histograms
call with only boundaries parameter: set or reset _all_ histograms
call with boundaries parameter and some of (or all, but it is not
useful) specific boundaries-*: set or reset _all_ histograms, some to
default and some to specific.
--
Best regards,
Vladimir
On 03/08/2018 12:58 PM, Vladimir Sementsov-Ogievskiy wrote: >>> Hm. these numbers are actually boundary points of histogram >>> intervals, not intervals itself. And, wiki says "The bins are usually >>> specified as consecutive, non-overlapping intervals of a variable.", >>> so, intervals are bins. >>> >>> So, what about: >>> >>> 1. interval_boundaries >>> 2. bin_boundaries >>> 3. boundaries >>> >>> (and same names with s/_/-/ for qapi) Any of those works for me; 1 is probably most precise, while 3 is the shortest to type. >>> >>> >> >> >> Also, now I doubt, is it a good idea to share same bin boundaries for >> each io type. >> > > so, for qmp command, what about: > > boundaries - optional, default boundaries for all io operations > boundaries-read - boundaries for read > boundaries-write - boundaries for write > ... > > so, call without any boundaries: drop _all_ histograms > call with only specific boundaries-*: set or reset _only_ corresponding > specific histograms > call with only boundaries parameter: set or reset _all_ histograms > call with boundaries parameter and some of (or all, but it is not > useful) specific boundaries-*: set or reset _all_ histograms, some to > default and some to specific. Seems reasonable, and if that makes the command more useful for your timing, I'm fine with it (your argument that different timing bins for read and write also makes sense, as there are some inherent differences in the amount of work done in the two directions). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
© 2016 - 2026 Red Hat, Inc.