Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/2] block latency histogram

[Emilio G. Cota]

On Thu, Mar 08, 2018 at 22:07:35 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 08.03.2018 21:56, Emilio G. Cota wrote:
> >  * Binning happens only at print time, so that we retain the flexibility to
> >  * choose the binning. This might not be ideal for workloads that do not 
> > care
> >  * much about precision and insert many samples all with different x values;
> >  * in that case, pre-binning (e.g. entering both 0.115 and 0.097 as 0.1)
> >  * should be considered.
(snip)
> In this case, I'll have to do same bin search (and store same interval
> settings) as I already do, on my part, to calculate a parameter for qdist
> interface. And I'll have store almost all same data on my part. So, it
> doesn't really help. And I need nothing of qdist benefits: I don't need (and
> don't want) dynamic allocation of bins on adding an element or any type of
> visualization.

I see. You require a couple of features that qdist doesn't yet support:

- Arbitrarily-sized, pre-defined bins.
- Support for querying the data programmatically instead of just
  printing it out.

We could circumvent the first missing feature with pre-binning,
but in that case we'd do a bsearch twice as you point out (BTW
your concern about memory allocation wouldn't apply though).

The second missing feature should be easy to add to qdist.

That said, given that you want this in for 2.12, I'd go with your
approach for now. In the future we should look into supporting
your use case in qdist, since it is likely that there will be
more users with a similar need.

Thanks,

Emilio

Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/2] block latency histogram

[Emilio G. Cota]

On Thu, Mar 08, 2018 at 14:42:29 +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi Emilio!
> 
> Looked through qdist, if I understand correctly, it saves each added (with
> different value) element. It is not effective for disk io timing - we'll
> have too much elements. In my approach, histogram don't grow, it initially
> have several ranges and counts hits to each range.

I thought about this use case, i.e. having a gazillion elements.
You should just do some pre-binning before inserting samples
into qdist, as pointed out in this comment in qdist.h:

/*
 * Samples with the same 'x value' end up in the same qdist_entry,
 * e.g. inc(0.1) and inc(0.1) end up as {x=0.1, count=2}.
 *
 * Binning happens only at print time, so that we retain the flexibility to
 * choose the binning. This might not be ideal for workloads that do not care
 * much about precision and insert many samples all with different x values;
 * in that case, pre-binning (e.g. entering both 0.115 and 0.097 as 0.1)
 * should be considered.
 */
struct qdist_entry {
double x;
unsigned long count;
};

Let me know if you need help with that.

Thanks,

Emilio

Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/2] block latency histogram

[Emilio G. Cota]

On Tue, Mar 06, 2018 at 16:00:17 +, Stefan Hajnoczi wrote:
> On Wed, Feb 07, 2018 at 03:50:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Vladimir Sementsov-Ogievskiy (2):
> >   block/accounting: introduce latency histogram
> >   qapi: add block latency histogram interface
(snip)
> >  5 files changed, 228 insertions(+), 1 deletion(-)
> 
> The feature looks good.  I posted documentation and code readability
> suggestions.

Hi Vladimir,

Did you consider using qdist? For reference, see commit bf3afd5f419
and/or grep 'qdist'.

Thanks,

Emilio

Re: [Qemu-block] [Qemu-devel] [PATCH v1 2/8] tests: Replace fprintf(stderr, "*\n" with error_report()

[Emilio G. Cota]

On Mon, Sep 25, 2017 at 17:08:48 -0700, Alistair Francis wrote:
> diff --git a/tests/atomic_add-bench.c b/tests/atomic_add-bench.c
> index caa1e8e689..41ba1600c0 100644
> --- a/tests/atomic_add-bench.c
> +++ b/tests/atomic_add-bench.c
> @@ -29,7 +29,7 @@ static const char commands_string[] =
>  static void usage_complete(char *argv[])
>  {
>  fprintf(stderr, "Usage: %s [options]\n", argv[0]);
> -fprintf(stderr, "options:\n%s\n", commands_string);
> +fprintf(stderr, "options:\n%s", commands_string);
>  }

We do want that trailing \n, unless we move it to commands_string.

Also, I think using error_report here would be confusing -- this is a standalone
test program with as little QEMU-specific knowledge as possible (QemuThreads
are used for portability); using error_report here is confusing (this is not
an error).

> diff --git a/tests/check-qlit b/tests/check-qlit
> new file mode 100755
> index 
> ..950429524e3eb07e6daed1fe01caad0f5d554809
> GIT binary patch
> literal 272776
> zcmeEvdwf*Ywf~vPB$){zGeCghJ;($So{10*LNEgfoIs*MKvBRDLV(l`3b5QKOS6

? I don't know what this is, I don't seem to have this binary in my
checked out tree.

(snips thousands of lines)

> diff --git a/tests/qht-bench.c b/tests/qht-bench.c
> index 11c1cec766..2637d601a9 100644
> --- a/tests/qht-bench.c
> +++ b/tests/qht-bench.c
> @@ -5,6 +5,7 @@
>   *   See the COPYING file in the top-level directory.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qemu/processor.h"
>  #include "qemu/atomic.h"
>  #include "qemu/qht.h"
> @@ -89,7 +90,7 @@ static const char commands_string[] =
>  static void usage_complete(int argc, char *argv[])
>  {
>  fprintf(stderr, "Usage: %s [options]\n", argv[0]);
> -fprintf(stderr, "options:\n%s\n", commands_string);
> +fprintf(stderr, "options:\n%s", commands_string);

Same as above: this removes the necessary trailing \n.

>  exit(-1);
>  }
>  
> @@ -328,7 +329,7 @@ static void htable_init(void)
>  retries++;
>  }
>  }
> -fprintf(stderr, " populated after %zu retries\n", retries);
> +error_report(" populated after %zu retries", retries);
>  }

ditto -- I'd rather keep fprintf(stderr) here, it's less confusing.

Thanks,

Emilio

Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] qemu-thread: introduce QemuLockCnt

[Emilio G. Cota]

On Tue, Nov 29, 2016 at 12:46:59 +0100, Paolo Bonzini wrote:
> A QemuLockCnt comprises a counter and a mutex, with primitives
> to increment and decrement the counter, and to take and release the
> mutex.  It can be used to do lock-free visits to a data structure
> whenever mutexes would be too heavy-weight and the critical section
> is too long for RCU.
> 
> This could be implemented simply by protecting the counter with the
> mutex, but QemuLockCnt is harder to misuse and more efficient.
> 
> Signed-off-by: Paolo Bonzini 
(snip)
> +++ b/docs/lockcnt.txt
> @@ -0,0 +1,343 @@
> +DOCUMENTATION FOR LOCKED COUNTERS (aka QemuLockCnt)
> +===
(snip)
> +bool qemu_lockcnt_dec_if_lock(QemuLockCnt *lockcnt);
> +
> +If the count is 1, decrement the count to zero, lock
> +the mutex and return true.  Otherwise, return false.
> +
(snip)
> +++ b/util/lockcnt.c
(snip)
> +void qemu_lockcnt_init(QemuLockCnt *lockcnt)
> +{
> +qemu_mutex_init(>mutex);
> +lockcnt->count = 0;

Minor nit: a release barrier here wouldn't harm.

> +}
> +
> +void qemu_lockcnt_destroy(QemuLockCnt *lockcnt)
> +{
> +qemu_mutex_destroy(>mutex);
> +}
> +
> +void qemu_lockcnt_inc(QemuLockCnt *lockcnt)
> +{
> +int old;
> +for (;;) {
> +old = atomic_read(>count);
> +if (old == 0) {
> +qemu_lockcnt_lock(lockcnt);
> +qemu_lockcnt_inc_and_unlock(lockcnt);
> +return;
> +} else {
> +if (atomic_cmpxchg(>count, old, old + 1) == old) {
> +return;
> +}
> +}
> +}
> +}
> +
> +void qemu_lockcnt_dec(QemuLockCnt *lockcnt)
> +{
> +atomic_dec(>count);
> +}
> +
> +/* Decrement a counter, and return locked if it is decremented to zero.
> + * It is impossible for the counter to become nonzero while the mutex
> + * is taken.
> + */
> +bool qemu_lockcnt_dec_and_lock(QemuLockCnt *lockcnt)
> +{
> +int val = atomic_read(>count);
> +while (val > 1) {
> +int old = atomic_cmpxchg(>count, val, val - 1);
> +if (old != val) {
> +val = old;
> +continue;
> +}
> +
> +return false;
> +}

Minor nit:
while (val > 1) {
int old = cmpxchg();
if (old == val) {
return false;
}
val = old;
}
seems to me a little easier to read.

> +qemu_lockcnt_lock(lockcnt);
> +if (atomic_fetch_dec(>count) == 1) {
> +return true;
> +}
> +
> +qemu_lockcnt_unlock(lockcnt);
> +return false;
> +}
> +
> +/* Decrement a counter and return locked if it is decremented to zero.
> + * Otherwise do nothing.

This comment doesn't match the code below nor the description in the
.txt file (quoted above) [we might not decrement the counter at all!]

> + *
> + * It is impossible for the counter to become nonzero while the mutex
> + * is taken.
> + */
> +bool qemu_lockcnt_dec_if_lock(QemuLockCnt *lockcnt)
> +{
> +int val = atomic_mb_read(>count);
> +if (val > 1) {
> +return false;
> +}
> +
> +qemu_lockcnt_lock(lockcnt);
> +if (atomic_fetch_dec(>count) == 1) {
> +return true;
> +}
> +
> +qemu_lockcnt_inc_and_unlock(lockcnt);

The choice of dec+(maybe)inc over cmpxchg seems a little odd to me.

Emilio

Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] qemu-thread: introduce QemuLockCnt

[Emilio G. Cota]

Just noticed the message above mistakenly sat in my outbox for
nearly 2 months. Just flushed it, so do not be surprised by
its original date.

Sorry for the noise,

Emilio