[PATCH v2 0/8] simplebench improvements

[PATCH v2 0/8] simplebench improvements

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are some improvements to simplebench lib, to support my
"qcow2: compressed write cache" series.

v1 was inside "[PATCH 0/7] qcow2: compressed write cache"
  <20210129165030.640169-1-vsementsov@virtuozzo.com>
  https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07795.html
  https://patchew.org/QEMU/20210129165030.640169-1-vsementsov@virtuozzo.com/

v2: split out to be a separate series, which I can finally pull by
    myself.
01: fix s/50/slow_limit
05-08: new

Vladimir Sementsov-Ogievskiy (8):
  simplebench: bench_one(): add slow_limit argument
  simplebench: bench_one(): support count=1
  simplebench/bench-backup: add --compressed option
  simplebench/bench-backup: add target-cache argument
  simplebench/bench_block_job: handle error in BLOCK_JOB_COMPLETED
  simplebench/bench-backup: support qcow2 source files
  simplebench/bench-backup: add --count and --no-initial-run
  simplebench/bench_block_job: drop caches before test run

 scripts/simplebench/bench-backup.py    | 89 +++++++++++++++++++++-----
 scripts/simplebench/bench_block_job.py | 44 ++++++++++++-
 scripts/simplebench/simplebench.py     | 34 ++++++++--
 3 files changed, 142 insertions(+), 25 deletions(-)

-- 
2.29.2

[PATCH v2 1/8] simplebench: bench_one(): add slow_limit argument

[Vladimir Sementsov-Ogievskiy]

Sometimes one of cells in a testing table runs too slow. And we really
don't want to wait so long. Limit number of runs in this case.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/simplebench.py | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/scripts/simplebench/simplebench.py b/scripts/simplebench/simplebench.py
index f61513af90..b153cae274 100644
--- a/scripts/simplebench/simplebench.py
+++ b/scripts/simplebench/simplebench.py
@@ -19,9 +19,11 @@
 #
 
 import statistics
+import time
 
 
-def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
+def bench_one(test_func, test_env, test_case, count=5, initial_run=True,
+              slow_limit=100):
     """Benchmark one test-case
 
     test_func   -- benchmarking function with prototype
@@ -36,6 +38,8 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
     test_case   -- test case - opaque second argument for test_func
     count       -- how many times to call test_func, to calculate average
     initial_run -- do initial run of test_func, which don't get into result
+    slow_limit  -- reduce test runs to 2, if current run exceedes the limit
+                   (in seconds)
 
     Returns dict with the following fields:
         'runs':     list of test_func results
@@ -47,17 +51,34 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
         'n-failed': number of failed runs (exists only if at least one run
                     failed)
     """
+    runs = []
+    i = 0
     if initial_run:
+        t = time.time()
+
         print('  #initial run:')
-        print('   ', test_func(test_env, test_case))
+        res = test_func(test_env, test_case)
+        print('   ', res)
+
+        if time.time() - t > slow_limit:
+            print('    - initial run is too slow, so it counts')
+            runs.append(res)
+            i = 1
+
+    for i in range(i, count):
+        t = time.time()
 
-    runs = []
-    for i in range(count):
         print('  #run {}'.format(i+1))
         res = test_func(test_env, test_case)
         print('   ', res)
         runs.append(res)
 
+        if time.time() - t > slow_limit and len(runs) >= 2:
+            print('    - run is too slow, and we have enough runs, stop here')
+            break
+
+    count = len(runs)
+
     result = {'runs': runs}
 
     succeeded = [r for r in runs if ('seconds' in r or 'iops' in r)]
-- 
2.29.2

[PATCH v2 2/8] simplebench: bench_one(): support count=1

[Vladimir Sementsov-Ogievskiy]

statistics.stdev raises if sequence length is less than two. Support
that case by hand.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/simplebench.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/simplebench/simplebench.py b/scripts/simplebench/simplebench.py
index b153cae274..712e1f845b 100644
--- a/scripts/simplebench/simplebench.py
+++ b/scripts/simplebench/simplebench.py
@@ -92,7 +92,10 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True,
             dim = 'seconds'
         result['dimension'] = dim
         result['average'] = statistics.mean(r[dim] for r in succeeded)
-        result['stdev'] = statistics.stdev(r[dim] for r in succeeded)
+        if len(succeeded) == 1:
+            result['stdev'] = 0
+        else:
+            result['stdev'] = statistics.stdev(r[dim] for r in succeeded)
 
     if len(succeeded) < count:
         result['n-failed'] = count - len(succeeded)
-- 
2.29.2

[PATCH v2 3/8] simplebench/bench-backup: add --compressed option

[Vladimir Sementsov-Ogievskiy]

Allow bench compressed backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/bench-backup.py    | 55 ++++++++++++++++++--------
 scripts/simplebench/bench_block_job.py | 23 +++++++++++
 2 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/scripts/simplebench/bench-backup.py b/scripts/simplebench/bench-backup.py
index 33a1ecfefa..72eae85bb1 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -23,7 +23,7 @@
 
 import simplebench
 from results_to_text import results_to_text
-from bench_block_job import bench_block_copy, drv_file, drv_nbd
+from bench_block_job import bench_block_copy, drv_file, drv_nbd, drv_qcow2
 
 
 def bench_func(env, case):
@@ -37,29 +37,41 @@ def bench_func(env, case):
 def bench(args):
     test_cases = []
 
-    sources = {}
-    targets = {}
-    for d in args.dir:
-        label, path = d.split(':')  # paths with colon not supported
-        sources[label] = drv_file(path + '/test-source')
-        targets[label] = drv_file(path + '/test-target')
+    # paths with colon not supported, so we just split by ':'
+    dirs = dict(d.split(':') for d in args.dir)
 
+    nbd_drv = None
     if args.nbd:
         nbd = args.nbd.split(':')
         host = nbd[0]
         port = '10809' if len(nbd) == 1 else nbd[1]
-        drv = drv_nbd(host, port)
-        sources['nbd'] = drv
-        targets['nbd'] = drv
+        nbd_drv = drv_nbd(host, port)
 
     for t in args.test:
         src, dst = t.split(':')
 
-        test_cases.append({
-            'id': t,
-            'source': sources[src],
-            'target': targets[dst]
-        })
+        if src == 'nbd' and dst == 'nbd':
+            raise ValueError("Can't use 'nbd' label for both src and dst")
+
+        if (src == 'nbd' or dst == 'nbd') and not nbd_drv:
+            raise ValueError("'nbd' label used but --nbd is not given")
+
+        if src == 'nbd':
+            source = nbd_drv
+        else:
+            source = drv_file(dirs[src] + '/test-source')
+
+        if dst == 'nbd':
+            test_cases.append({'id': t, 'source': source, 'target': nbd_drv})
+            continue
+
+        fname = dirs[dst] + '/test-target'
+        if args.compressed:
+            fname += '.qcow2'
+        target = drv_file(fname)
+        if args.compressed:
+            target = drv_qcow2(target)
+        test_cases.append({'id': t, 'source': source, 'target': target})
 
     binaries = []  # list of (<label>, <path>, [<options>])
     for i, q in enumerate(args.env):
@@ -106,6 +118,13 @@ def bench(args):
             elif opt.startswith('max-workers='):
                 x_perf['max-workers'] = int(opt.split('=')[1])
 
+        backup_options = {}
+        if x_perf:
+            backup_options['x-perf'] = x_perf
+
+        if args.compressed:
+            backup_options['compress'] = True
+
         if is_mirror:
             assert not x_perf
             test_envs.append({
@@ -117,7 +136,7 @@ def bench(args):
             test_envs.append({
                 'id': f'backup({label})\n' + '\n'.join(opts),
                 'cmd': 'blockdev-backup',
-                'cmd-options': {'x-perf': x_perf} if x_perf else {},
+                'cmd-options': backup_options,
                 'qemu-binary': path
             })
 
@@ -163,5 +182,9 @@ def __call__(self, parser, namespace, values, option_string=None):
     p.add_argument('--test', nargs='+', help='''\
 Tests, in form source-dir-label:target-dir-label''',
                    action=ExtendAction)
+    p.add_argument('--compressed', help='''\
+Use compressed backup. It automatically means
+automatically creating qcow2 target with
+lazy_refcounts for each test run''', action='store_true')
 
     bench(p.parse_args())
diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index 7332845c1c..08f86ed9c1 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -21,6 +21,7 @@
 
 import sys
 import os
+import subprocess
 import socket
 import json
 
@@ -77,11 +78,29 @@ def bench_block_job(cmd, cmd_args, qemu_args):
     return {'seconds': (end_ms - start_ms) / 1000000.0}
 
 
+def get_image_size(path):
+    out = subprocess.run(['qemu-img', 'info', '--out=json', path],
+                         stdout=subprocess.PIPE, check=True).stdout
+    return json.loads(out)['virtual-size']
+
+
 # Bench backup or mirror
 def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
     """Helper to run bench_block_job() for mirror or backup"""
     assert cmd in ('blockdev-backup', 'blockdev-mirror')
 
+    if target['driver'] == 'qcow2':
+        try:
+            os.remove(target['file']['filename'])
+        except OSError:
+            pass
+
+        subprocess.run(['qemu-img', 'create', '-f', 'qcow2',
+                        target['file']['filename'],
+                        str(get_image_size(source['filename']))],
+                       stdout=subprocess.DEVNULL,
+                       stderr=subprocess.DEVNULL, check=True)
+
     source['node-name'] = 'source'
     target['node-name'] = 'target'
 
@@ -106,6 +125,10 @@ def drv_nbd(host, port):
             'server': {'type': 'inet', 'host': host, 'port': port}}
 
 
+def drv_qcow2(file):
+    return {'driver': 'qcow2', 'file': file}
+
+
 if __name__ == '__main__':
     import sys
 
-- 
2.29.2

[PATCH v2 4/8] simplebench/bench-backup: add target-cache argument

[Vladimir Sementsov-Ogievskiy]

Allow benchmark with different kinds of target cache.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/bench-backup.py    | 33 ++++++++++++++++++++------
 scripts/simplebench/bench_block_job.py | 10 +++++---
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/scripts/simplebench/bench-backup.py b/scripts/simplebench/bench-backup.py
index 72eae85bb1..fbc85f266f 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -65,13 +65,26 @@ def bench(args):
             test_cases.append({'id': t, 'source': source, 'target': nbd_drv})
             continue
 
-        fname = dirs[dst] + '/test-target'
-        if args.compressed:
-            fname += '.qcow2'
-        target = drv_file(fname)
-        if args.compressed:
-            target = drv_qcow2(target)
-        test_cases.append({'id': t, 'source': source, 'target': target})
+        if args.target_cache == 'both':
+            target_caches = ['direct', 'cached']
+        else:
+            target_caches = [args.target_cache]
+
+        for c in target_caches:
+            o_direct = c == 'direct'
+            fname = dirs[dst] + '/test-target'
+            if args.compressed:
+                fname += '.qcow2'
+            target = drv_file(fname, o_direct=o_direct)
+            if args.compressed:
+                target = drv_qcow2(target)
+
+            test_id = t
+            if args.target_cache == 'both':
+                test_id += f'({c})'
+
+            test_cases.append({'id': test_id, 'source': source,
+                               'target': target})
 
     binaries = []  # list of (<label>, <path>, [<options>])
     for i, q in enumerate(args.env):
@@ -186,5 +199,11 @@ def __call__(self, parser, namespace, values, option_string=None):
 Use compressed backup. It automatically means
 automatically creating qcow2 target with
 lazy_refcounts for each test run''', action='store_true')
+    p.add_argument('--target-cache', help='''\
+Setup cache for target nodes. Options:
+   direct: default, use O_DIRECT and aio=native
+   cached: use system cache (Qemu default) and aio=threads (Qemu default)
+   both: generate two test cases for each src:dst pair''',
+                   default='direct', choices=('direct', 'cached', 'both'))
 
     bench(p.parse_args())
diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index 08f86ed9c1..8f8385ccce 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -115,9 +115,13 @@ def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
                             '-blockdev', json.dumps(target)])
 
 
-def drv_file(filename):
-    return {'driver': 'file', 'filename': filename,
-            'cache': {'direct': True}, 'aio': 'native'}
+def drv_file(filename, o_direct=True):
+    node = {'driver': 'file', 'filename': filename}
+    if o_direct:
+        node['cache'] = {'direct': True}
+        node['aio'] = 'native'
+
+    return node
 
 
 def drv_nbd(host, port):
-- 
2.29.2

[PATCH v2 5/8] simplebench/bench_block_job: handle error in BLOCK_JOB_COMPLETED

[Vladimir Sementsov-Ogievskiy]

We should not report success if there is an error in final event.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/bench_block_job.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index 8f8385ccce..71d2e489c8 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -70,6 +70,10 @@ def bench_block_job(cmd, cmd_args, qemu_args):
             vm.shutdown()
             return {'error': 'block-job failed: ' + str(e),
                     'vm-log': vm.get_log()}
+        if 'error' in e['data']:
+            vm.shutdown()
+            return {'error': 'block-job failed: ' + e['data']['error'],
+                    'vm-log': vm.get_log()}
         end_ms = e['timestamp']['seconds'] * 1000000 + \
             e['timestamp']['microseconds']
     finally:
-- 
2.29.2

[PATCH v2 6/8] simplebench/bench-backup: support qcow2 source files

[Vladimir Sementsov-Ogievskiy]

Add support for qcow2 source. New option says to use test-source.qcow2
instead of test-source. Of course, test-source.qcow2 should be
precreated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/bench-backup.py    | 5 +++++
 scripts/simplebench/bench_block_job.py | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/scripts/simplebench/bench-backup.py b/scripts/simplebench/bench-backup.py
index fbc85f266f..a2120fcbf0 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -58,6 +58,8 @@ def bench(args):
 
         if src == 'nbd':
             source = nbd_drv
+        elif args.qcow2_sources:
+            source = drv_qcow2(drv_file(dirs[src] + '/test-source.qcow2'))
         else:
             source = drv_file(dirs[src] + '/test-source')
 
@@ -199,6 +201,9 @@ def __call__(self, parser, namespace, values, option_string=None):
 Use compressed backup. It automatically means
 automatically creating qcow2 target with
 lazy_refcounts for each test run''', action='store_true')
+    p.add_argument('--qcow2-sources', help='''\
+Use test-source.qcow2 images as sources instead of
+test-source raw images''', action='store_true')
     p.add_argument('--target-cache', help='''\
 Setup cache for target nodes. Options:
    direct: default, use O_DIRECT and aio=native
diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index 71d2e489c8..4f03c12169 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -88,6 +88,11 @@ def get_image_size(path):
     return json.loads(out)['virtual-size']
 
 
+def get_blockdev_size(obj):
+    img = obj['filename'] if 'filename' in obj else obj['file']['filename']
+    return get_image_size(img)
+
+
 # Bench backup or mirror
 def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
     """Helper to run bench_block_job() for mirror or backup"""
@@ -101,7 +106,7 @@ def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
 
         subprocess.run(['qemu-img', 'create', '-f', 'qcow2',
                         target['file']['filename'],
-                        str(get_image_size(source['filename']))],
+                        str(get_blockdev_size(source))],
                        stdout=subprocess.DEVNULL,
                        stderr=subprocess.DEVNULL, check=True)
 
-- 
2.29.2

[PATCH v2 7/8] simplebench/bench-backup: add --count and --no-initial-run

[Vladimir Sementsov-Ogievskiy]

Add arguments to set number of test runs per table cell and to disable
initial run that is not counted in results.

It's convenient to set --count 1 --no-initial-run to fast run test
onece, and to set --count to some large enough number for good
precision of the results.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/bench-backup.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/scripts/simplebench/bench-backup.py b/scripts/simplebench/bench-backup.py
index a2120fcbf0..519a985a7f 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -155,7 +155,9 @@ def bench(args):
                 'qemu-binary': path
             })
 
-    result = simplebench.bench(bench_func, test_envs, test_cases, count=3)
+    result = simplebench.bench(bench_func, test_envs, test_cases,
+                               count=args.count,
+                               initial_run = not args.no_initial_run)
     with open('results.json', 'w') as f:
         json.dump(result, f, indent=4)
     print(results_to_text(result))
@@ -211,4 +213,10 @@ def __call__(self, parser, namespace, values, option_string=None):
    both: generate two test cases for each src:dst pair''',
                    default='direct', choices=('direct', 'cached', 'both'))
 
+    p.add_argument('--count', type=int, default=3, help='''\
+Number of test runs per table cell''')
+
+    p.add_argument('--no-initial-run', action='store_true', help='''\
+Don't do initial run of test for each cell which doesn't count''')
+
     bench(p.parse_args())
-- 
2.29.2

[PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[Vladimir Sementsov-Ogievskiy]

It probably may improve reliability of results when testing in cached
mode.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/bench_block_job.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index 4f03c12169..fa45ad2655 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
         return {'error': 'qemu failed: ' + str(vm.get_log())}
 
     try:
+        subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', shell=True,
+                       check=True)
         res = vm.qmp(cmd, **cmd_args)
         if res != {'return': {}}:
             vm.shutdown()
-- 
2.29.2

[PATCH v2 9/8] MAINTAINERS: update Benchmark util: add git tree

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9b2aa18e1f..642c1c8a46 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2484,6 +2484,7 @@ Benchmark util
 M: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 S: Maintained
 F: scripts/simplebench/
+T: git https://src.openvz.org/scm/~vsementsov/qemu.git simplebench
 
 QAPI
 M: Markus Armbruster <armbru@redhat.com>
-- 
2.29.2

Re: [PATCH v2 1/8] simplebench: bench_one(): add slow_limit argument

[John Snow]

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> Sometimes one of cells in a testing table runs too slow. And we really
> don't want to wait so long. Limit number of runs in this case.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/simplebench.py | 29 +++++++++++++++++++++++++----
>   1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/simplebench/simplebench.py b/scripts/simplebench/simplebench.py
> index f61513af90..b153cae274 100644
> --- a/scripts/simplebench/simplebench.py
> +++ b/scripts/simplebench/simplebench.py
> @@ -19,9 +19,11 @@
>   #
>   
>   import statistics
> +import time
>   
>   
> -def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
> +def bench_one(test_func, test_env, test_case, count=5, initial_run=True,
> +              slow_limit=100):
>       """Benchmark one test-case
>   
>       test_func   -- benchmarking function with prototype
> @@ -36,6 +38,8 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
>       test_case   -- test case - opaque second argument for test_func
>       count       -- how many times to call test_func, to calculate average
>       initial_run -- do initial run of test_func, which don't get into result
> +    slow_limit  -- reduce test runs to 2, if current run exceedes the limit
> +                   (in seconds)
>   

s/exceedes/exceeds, and you need to mention that if the initial run 
exceeds the limit, it will change the behavior to count that result.

It is also possible (conceivably) that the initial run exceeds the 
limit, but subsequent runs don't, so it might be hard to predict how 
many tests it'll actually run.

If you're OK with that behavior, maybe:

"Consider a test run 'slow' once it exceeds this limit, in seconds.
  Stop early once there are two 'slow' runs, including the initial run.
  Slow initial runs will be included in the results."

Lastly, this will change existing behavior -- do we care? Should it 
default to None instead? Should we be able to pass None or 0 to disable 
this behavior?

>       Returns dict with the following fields:
>           'runs':     list of test_func results
> @@ -47,17 +51,34 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
>           'n-failed': number of failed runs (exists only if at least one run
>                       failed)
>       """
> +    runs = []
> +    i = 0
>       if initial_run:
> +        t = time.time()
> +
>           print('  #initial run:')
> -        print('   ', test_func(test_env, test_case))
> +        res = test_func(test_env, test_case)
> +        print('   ', res)
> +
> +        if time.time() - t > slow_limit:
> +            print('    - initial run is too slow, so it counts')
> +            runs.append(res)
> +            i = 1
> +
> +    for i in range(i, count):
> +        t = time.time()
>   
> -    runs = []
> -    for i in range(count):
>           print('  #run {}'.format(i+1))
>           res = test_func(test_env, test_case)
>           print('   ', res)
>           runs.append(res)
>   
> +        if time.time() - t > slow_limit and len(runs) >= 2:
> +            print('    - run is too slow, and we have enough runs, stop here')
> +            break
> +
> +    count = len(runs)
> +
>       result = {'runs': runs}
>   
>       succeeded = [r for r in runs if ('seconds' in r or 'iops' in r)]
> 

Re: [PATCH v2 2/8] simplebench: bench_one(): support count=1

[John Snow]

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> statistics.stdev raises if sequence length is less than two. Support
> that case by hand.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/simplebench.py | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/simplebench/simplebench.py b/scripts/simplebench/simplebench.py
> index b153cae274..712e1f845b 100644
> --- a/scripts/simplebench/simplebench.py
> +++ b/scripts/simplebench/simplebench.py
> @@ -92,7 +92,10 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True,
>               dim = 'seconds'
>           result['dimension'] = dim
>           result['average'] = statistics.mean(r[dim] for r in succeeded)
> -        result['stdev'] = statistics.stdev(r[dim] for r in succeeded)
> +        if len(succeeded) == 1:
> +            result['stdev'] = 0
> +        else:
> +            result['stdev'] = statistics.stdev(r[dim] for r in succeeded)
>   
>       if len(succeeded) < count:
>           result['n-failed'] = count - len(succeeded)
> 

Omitted from patch context is that we have "if succeeded: ..." so we 
know it's at least 1 here.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[John Snow]

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> It probably may improve reliability of results when testing in cached
> mode.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/bench_block_job.py | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
> index 4f03c12169..fa45ad2655 100755
> --- a/scripts/simplebench/bench_block_job.py
> +++ b/scripts/simplebench/bench_block_job.py
> @@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
>           return {'error': 'qemu failed: ' + str(vm.get_log())}
>   
>       try:
> +        subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', shell=True,
> +                       check=True)
>           res = vm.qmp(cmd, **cmd_args)
>           if res != {'return': {}}:
>               vm.shutdown()
> 

Worth adding a conditional to allow "hot" or "cold" runs? nah?

Re: [PATCH v2 7/8] simplebench/bench-backup: add --count and --no-initial-run

[John Snow]

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add arguments to set number of test runs per table cell and to disable
> initial run that is not counted in results.
> 
> It's convenient to set --count 1 --no-initial-run to fast run test
> onece, and to set --count to some large enough number for good
> precision of the results.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/bench-backup.py | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/simplebench/bench-backup.py b/scripts/simplebench/bench-backup.py
> index a2120fcbf0..519a985a7f 100755
> --- a/scripts/simplebench/bench-backup.py
> +++ b/scripts/simplebench/bench-backup.py
> @@ -155,7 +155,9 @@ def bench(args):
>                   'qemu-binary': path
>               })
>   
> -    result = simplebench.bench(bench_func, test_envs, test_cases, count=3)
> +    result = simplebench.bench(bench_func, test_envs, test_cases,
> +                               count=args.count,
> +                               initial_run = not args.no_initial_run)

The double negative feels odd; "initial_run = args.initial_run" would 
read better and avoid changing behavior, but maybe that's intentional.

>       with open('results.json', 'w') as f:
>           json.dump(result, f, indent=4)
>       print(results_to_text(result))
> @@ -211,4 +213,10 @@ def __call__(self, parser, namespace, values, option_string=None):
>      both: generate two test cases for each src:dst pair''',
>                      default='direct', choices=('direct', 'cached', 'both'))
>   
> +    p.add_argument('--count', type=int, default=3, help='''\
> +Number of test runs per table cell''')
> +
> +    p.add_argument('--no-initial-run', action='store_true', help='''\
> +Don't do initial run of test for each cell which doesn't count''')
> +
>       bench(p.parse_args())
> 

Re: [PATCH v2 6/8] simplebench/bench-backup: support qcow2 source files

[John Snow]

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add support for qcow2 source. New option says to use test-source.qcow2
> instead of test-source. Of course, test-source.qcow2 should be
> precreated.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/bench-backup.py    | 5 +++++
>   scripts/simplebench/bench_block_job.py | 7 ++++++-
>   2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/simplebench/bench-backup.py b/scripts/simplebench/bench-backup.py
> index fbc85f266f..a2120fcbf0 100755
> --- a/scripts/simplebench/bench-backup.py
> +++ b/scripts/simplebench/bench-backup.py
> @@ -58,6 +58,8 @@ def bench(args):
>   
>           if src == 'nbd':
>               source = nbd_drv
> +        elif args.qcow2_sources:
> +            source = drv_qcow2(drv_file(dirs[src] + '/test-source.qcow2'))
>           else:
>               source = drv_file(dirs[src] + '/test-source')
>   
> @@ -199,6 +201,9 @@ def __call__(self, parser, namespace, values, option_string=None):
>   Use compressed backup. It automatically means
>   automatically creating qcow2 target with
>   lazy_refcounts for each test run''', action='store_true')
> +    p.add_argument('--qcow2-sources', help='''\
> +Use test-source.qcow2 images as sources instead of
> +test-source raw images''', action='store_true')
>       p.add_argument('--target-cache', help='''\
>   Setup cache for target nodes. Options:
>      direct: default, use O_DIRECT and aio=native
> diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
> index 71d2e489c8..4f03c12169 100755
> --- a/scripts/simplebench/bench_block_job.py
> +++ b/scripts/simplebench/bench_block_job.py
> @@ -88,6 +88,11 @@ def get_image_size(path):
>       return json.loads(out)['virtual-size']
>   
>   
> +def get_blockdev_size(obj):
> +    img = obj['filename'] if 'filename' in obj else obj['file']['filename']
> +    return get_image_size(img)
> +

Well, as long as it works :)

> +
>   # Bench backup or mirror
>   def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
>       """Helper to run bench_block_job() for mirror or backup"""
> @@ -101,7 +106,7 @@ def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
>   
>           subprocess.run(['qemu-img', 'create', '-f', 'qcow2',
>                           target['file']['filename'],
> -                        str(get_image_size(source['filename']))],
> +                        str(get_blockdev_size(source))],
>                          stdout=subprocess.DEVNULL,
>                          stderr=subprocess.DEVNULL, check=True)
>   
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH v2 5/8] simplebench/bench_block_job: handle error in BLOCK_JOB_COMPLETED

[John Snow]

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> We should not report success if there is an error in final event.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/bench_block_job.py | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
> index 8f8385ccce..71d2e489c8 100755
> --- a/scripts/simplebench/bench_block_job.py
> +++ b/scripts/simplebench/bench_block_job.py
> @@ -70,6 +70,10 @@ def bench_block_job(cmd, cmd_args, qemu_args):
>               vm.shutdown()
>               return {'error': 'block-job failed: ' + str(e),
>                       'vm-log': vm.get_log()}
> +        if 'error' in e['data']:
> +            vm.shutdown()
> +            return {'error': 'block-job failed: ' + e['data']['error'],
> +                    'vm-log': vm.get_log()}
>           end_ms = e['timestamp']['seconds'] * 1000000 + \
>               e['timestamp']['microseconds']
>       finally:
> 

"Yes, the block job completed -- but not how you wanted it to."

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH v2 4/8] simplebench/bench-backup: add target-cache argument

[John Snow]

On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> Allow benchmark with different kinds of target cache.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/bench-backup.py    | 33 ++++++++++++++++++++------
>   scripts/simplebench/bench_block_job.py | 10 +++++---
>   2 files changed, 33 insertions(+), 10 deletions(-)
> 

Admittedly just skimming a bit on this one, it looks OK.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH v2 1/8] simplebench: bench_one(): add slow_limit argument

[Vladimir Sementsov-Ogievskiy]

05.03.2021 04:22, John Snow wrote:
> On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Sometimes one of cells in a testing table runs too slow. And we really
>> don't want to wait so long. Limit number of runs in this case.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/simplebench/simplebench.py | 29 +++++++++++++++++++++++++----
>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/simplebench/simplebench.py b/scripts/simplebench/simplebench.py
>> index f61513af90..b153cae274 100644
>> --- a/scripts/simplebench/simplebench.py
>> +++ b/scripts/simplebench/simplebench.py
>> @@ -19,9 +19,11 @@
>>   #
>>   import statistics
>> +import time
>> -def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
>> +def bench_one(test_func, test_env, test_case, count=5, initial_run=True,
>> +              slow_limit=100):
>>       """Benchmark one test-case
>>       test_func   -- benchmarking function with prototype
>> @@ -36,6 +38,8 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
>>       test_case   -- test case - opaque second argument for test_func
>>       count       -- how many times to call test_func, to calculate average
>>       initial_run -- do initial run of test_func, which don't get into result
>> +    slow_limit  -- reduce test runs to 2, if current run exceedes the limit
>> +                   (in seconds)
> 
> s/exceedes/exceeds, and you need to mention that if the initial run exceeds the limit, it will change the behavior to count that result.
> 
> It is also possible (conceivably) that the initial run exceeds the limit, but subsequent runs don't, so it might be hard to predict how many tests it'll actually run.
> 
> If you're OK with that behavior, maybe:
> 
> "Consider a test run 'slow' once it exceeds this limit, in seconds.
>   Stop early once there are two 'slow' runs, including the initial run.
>   Slow initial runs will be included in the results."
> 
> Lastly, this will change existing behavior -- do we care? Should it default to None instead? Should we be able to pass None or 0 to disable this behavior?

For sure I don't care about changing the behavior. Consider simplebench in a version 0.0.1 :). Maybe, I should make a comment somewhere, but nobody will read it anyway.

The aim of the patch is to minimize waiting for too long cells of the table, which are obviously too much longer then the others. Probably the logic should be improved a bit about ignoring or using initial-run result..
Like this:

If both initial and first run are slow, count both and stop here.
Otherwise, stop at first slow normal run and don't count initial run.

Or may be even

If both initial and first run are slow, count both and stop here.
Otherwise, behave the common way.

> 
>>       Returns dict with the following fields:
>>           'runs':     list of test_func results
>> @@ -47,17 +51,34 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
>>           'n-failed': number of failed runs (exists only if at least one run
>>                       failed)
>>       """
>> +    runs = []
>> +    i = 0
>>       if initial_run:
>> +        t = time.time()
>> +
>>           print('  #initial run:')
>> -        print('   ', test_func(test_env, test_case))
>> +        res = test_func(test_env, test_case)
>> +        print('   ', res)
>> +
>> +        if time.time() - t > slow_limit:
>> +            print('    - initial run is too slow, so it counts')
>> +            runs.append(res)
>> +            i = 1
>> +
>> +    for i in range(i, count):
>> +        t = time.time()
>> -    runs = []
>> -    for i in range(count):
>>           print('  #run {}'.format(i+1))
>>           res = test_func(test_env, test_case)
>>           print('   ', res)
>>           runs.append(res)
>> +        if time.time() - t > slow_limit and len(runs) >= 2:
>> +            print('    - run is too slow, and we have enough runs, stop here')
>> +            break
>> +
>> +    count = len(runs)
>> +
>>       result = {'runs': runs}
>>       succeeded = [r for r in runs if ('seconds' in r or 'iops' in r)]
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 7/8] simplebench/bench-backup: add --count and --no-initial-run

[Vladimir Sementsov-Ogievskiy]

05.03.2021 04:37, John Snow wrote:
> On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Add arguments to set number of test runs per table cell and to disable
>> initial run that is not counted in results.
>>
>> It's convenient to set --count 1 --no-initial-run to fast run test
>> onece, and to set --count to some large enough number for good
>> precision of the results.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/simplebench/bench-backup.py | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/simplebench/bench-backup.py b/scripts/simplebench/bench-backup.py
>> index a2120fcbf0..519a985a7f 100755
>> --- a/scripts/simplebench/bench-backup.py
>> +++ b/scripts/simplebench/bench-backup.py
>> @@ -155,7 +155,9 @@ def bench(args):
>>                   'qemu-binary': path
>>               })
>> -    result = simplebench.bench(bench_func, test_envs, test_cases, count=3)
>> +    result = simplebench.bench(bench_func, test_envs, test_cases,
>> +                               count=args.count,
>> +                               initial_run = not args.no_initial_run)
> 
> The double negative feels odd; "initial_run = args.initial_run" would read better and avoid changing behavior, but maybe that's intentional.

Hmm it was simple way to add --no-initial-run. But I agree it looks strange. Will improve.

> 
>>       with open('results.json', 'w') as f:
>>           json.dump(result, f, indent=4)
>>       print(results_to_text(result))
>> @@ -211,4 +213,10 @@ def __call__(self, parser, namespace, values, option_string=None):
>>      both: generate two test cases for each src:dst pair''',
>>                      default='direct', choices=('direct', 'cached', 'both'))
>> +    p.add_argument('--count', type=int, default=3, help='''\
>> +Number of test runs per table cell''')
>> +
>> +    p.add_argument('--no-initial-run', action='store_true', help='''\
>> +Don't do initial run of test for each cell which doesn't count''')
>> +
>>       bench(p.parse_args())
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[Vladimir Sementsov-Ogievskiy]

05.03.2021 04:30, John Snow wrote:
> On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It probably may improve reliability of results when testing in cached
>> mode.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/simplebench/bench_block_job.py | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
>> index 4f03c12169..fa45ad2655 100755
>> --- a/scripts/simplebench/bench_block_job.py
>> +++ b/scripts/simplebench/bench_block_job.py
>> @@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
>>           return {'error': 'qemu failed: ' + str(vm.get_log())}
>>       try:
>> +        subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', shell=True,
>> +                       check=True)
>>           res = vm.qmp(cmd, **cmd_args)
>>           if res != {'return': {}}:
>>               vm.shutdown()
>>
> 
> Worth adding a conditional to allow "hot" or "cold" runs? nah?
> 

You mean, make this addition optional? Make sense


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/8] simplebench: bench_one(): add slow_limit argument

[John Snow]

On 3/5/21 4:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 05.03.2021 04:22, John Snow wrote:
>> On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Sometimes one of cells in a testing table runs too slow. And we really
>>> don't want to wait so long. Limit number of runs in this case.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   scripts/simplebench/simplebench.py | 29 +++++++++++++++++++++++++----
>>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/scripts/simplebench/simplebench.py 
>>> b/scripts/simplebench/simplebench.py
>>> index f61513af90..b153cae274 100644
>>> --- a/scripts/simplebench/simplebench.py
>>> +++ b/scripts/simplebench/simplebench.py
>>> @@ -19,9 +19,11 @@
>>>   #
>>>   import statistics
>>> +import time
>>> -def bench_one(test_func, test_env, test_case, count=5, 
>>> initial_run=True):
>>> +def bench_one(test_func, test_env, test_case, count=5, 
>>> initial_run=True,
>>> +              slow_limit=100):
>>>       """Benchmark one test-case
>>>       test_func   -- benchmarking function with prototype
>>> @@ -36,6 +38,8 @@ def bench_one(test_func, test_env, test_case, 
>>> count=5, initial_run=True):
>>>       test_case   -- test case - opaque second argument for test_func
>>>       count       -- how many times to call test_func, to calculate 
>>> average
>>>       initial_run -- do initial run of test_func, which don't get 
>>> into result
>>> +    slow_limit  -- reduce test runs to 2, if current run exceedes 
>>> the limit
>>> +                   (in seconds)
>>
>> s/exceedes/exceeds, and you need to mention that if the initial run 
>> exceeds the limit, it will change the behavior to count that result.
>>
>> It is also possible (conceivably) that the initial run exceeds the 
>> limit, but subsequent runs don't, so it might be hard to predict how 
>> many tests it'll actually run.
>>
>> If you're OK with that behavior, maybe:
>>
>> "Consider a test run 'slow' once it exceeds this limit, in seconds.
>>   Stop early once there are two 'slow' runs, including the initial run.
>>   Slow initial runs will be included in the results."
>>
>> Lastly, this will change existing behavior -- do we care? Should it 
>> default to None instead? Should we be able to pass None or 0 to 
>> disable this behavior?
> 
> For sure I don't care about changing the behavior. Consider simplebench 
> in a version 0.0.1 :). Maybe, I should make a comment somewhere, but 
> nobody will read it anyway.
> 

Yep, it's yours anyway. Just thought I'd mention it. It's probably the 
case that you're the only person who actually uses this at the moment.

> The aim of the patch is to minimize waiting for too long cells of the 
> table, which are obviously too much longer then the others. Probably the 
> logic should be improved a bit about ignoring or using initial-run result..
> Like this:
> 
> If both initial and first run are slow, count both and stop here.
> Otherwise, stop at first slow normal run and don't count initial run.
> 
> Or may be even
> 
> If both initial and first run are slow, count both and stop here.
> Otherwise, behave the common way.
> 

My opinion is that you can do whatever you'd like (you're the maintainer 
here!) but it'd be nice if the docstring was accurate. If changing the 
behavior makes it easier to write a good docstring, that's fine too. Go 
with whatever is most useful to you.

--js

>>
>>>       Returns dict with the following fields:
>>>           'runs':     list of test_func results
>>> @@ -47,17 +51,34 @@ def bench_one(test_func, test_env, test_case, 
>>> count=5, initial_run=True):
>>>           'n-failed': number of failed runs (exists only if at least 
>>> one run
>>>                       failed)
>>>       """
>>> +    runs = []
>>> +    i = 0
>>>       if initial_run:
>>> +        t = time.time()
>>> +
>>>           print('  #initial run:')
>>> -        print('   ', test_func(test_env, test_case))
>>> +        res = test_func(test_env, test_case)
>>> +        print('   ', res)
>>> +
>>> +        if time.time() - t > slow_limit:
>>> +            print('    - initial run is too slow, so it counts')
>>> +            runs.append(res)
>>> +            i = 1
>>> +
>>> +    for i in range(i, count):
>>> +        t = time.time()
>>> -    runs = []
>>> -    for i in range(count):
>>>           print('  #run {}'.format(i+1))
>>>           res = test_func(test_env, test_case)
>>>           print('   ', res)
>>>           runs.append(res)
>>> +        if time.time() - t > slow_limit and len(runs) >= 2:
>>> +            print('    - run is too slow, and we have enough runs, 
>>> stop here')
>>> +            break
>>> +
>>> +    count = len(runs)
>>> +
>>>       result = {'runs': runs}
>>>       succeeded = [r for r in runs if ('seconds' in r or 'iops' in r)]
>>>
>>
> 
> 

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[John Snow]

On 3/5/21 4:11 AM, Vladimir Sementsov-Ogievskiy wrote:
> 05.03.2021 04:30, John Snow wrote:
>> On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> It probably may improve reliability of results when testing in cached
>>> mode.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   scripts/simplebench/bench_block_job.py | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/scripts/simplebench/bench_block_job.py 
>>> b/scripts/simplebench/bench_block_job.py
>>> index 4f03c12169..fa45ad2655 100755
>>> --- a/scripts/simplebench/bench_block_job.py
>>> +++ b/scripts/simplebench/bench_block_job.py
>>> @@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
>>>           return {'error': 'qemu failed: ' + str(vm.get_log())}
>>>       try:
>>> +        subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', 
>>> shell=True,
>>> +                       check=True)
>>>           res = vm.qmp(cmd, **cmd_args)
>>>           if res != {'return': {}}:
>>>               vm.shutdown()
>>>
>>
>> Worth adding a conditional to allow "hot" or "cold" runs? nah?
>>
> 
> You mean, make this addition optional? Make sense
> 
> 

I was thinking (along the lines of allowing both old and new behavior, 
in case anyone except you used these scripts) of this sort of thing:

def bench_block_job(cmd, cmd_args, qemu_args, drop_cache=True): ...

I don't insist on it; I was just earnestly wondering if it had any 
utility. If it doesn't, don't respin on my account.

--js

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[Vladimir Sementsov-Ogievskiy]

05.03.2021 19:30, John Snow wrote:
> On 3/5/21 4:11 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 05.03.2021 04:30, John Snow wrote:
>>> On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> It probably may improve reliability of results when testing in cached
>>>> mode.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   scripts/simplebench/bench_block_job.py | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
>>>> index 4f03c12169..fa45ad2655 100755
>>>> --- a/scripts/simplebench/bench_block_job.py
>>>> +++ b/scripts/simplebench/bench_block_job.py
>>>> @@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
>>>>           return {'error': 'qemu failed: ' + str(vm.get_log())}
>>>>       try:
>>>> +        subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', shell=True,
>>>> +                       check=True)
>>>>           res = vm.qmp(cmd, **cmd_args)
>>>>           if res != {'return': {}}:
>>>>               vm.shutdown()
>>>>
>>>
>>> Worth adding a conditional to allow "hot" or "cold" runs? nah?
>>>
>>
>> You mean, make this addition optional? Make sense
>>
>>
> 
> I was thinking (along the lines of allowing both old and new behavior, in case anyone except you used these scripts) of this sort of thing:
> 
> def bench_block_job(cmd, cmd_args, qemu_args, drop_cache=True): ...
> 
> I don't insist on it; I was just earnestly wondering if it had any utility. If it doesn't, don't respin on my account.
> 

Ok, thanks a lot for reviewing! Still, I think, I'll resend

-- 
Best regards,
Vladimir

Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run

[John Snow]

On 3/5/21 11:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> 05.03.2021 19:30, John Snow wrote:
>> On 3/5/21 4:11 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 05.03.2021 04:30, John Snow wrote:
>>>> On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> It probably may improve reliability of results when testing in cached
>>>>> mode.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>   scripts/simplebench/bench_block_job.py | 2 ++
>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/scripts/simplebench/bench_block_job.py 
>>>>> b/scripts/simplebench/bench_block_job.py
>>>>> index 4f03c12169..fa45ad2655 100755
>>>>> --- a/scripts/simplebench/bench_block_job.py
>>>>> +++ b/scripts/simplebench/bench_block_job.py
>>>>> @@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
>>>>>           return {'error': 'qemu failed: ' + str(vm.get_log())}
>>>>>       try:
>>>>> +        subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', 
>>>>> shell=True,
>>>>> +                       check=True)
>>>>>           res = vm.qmp(cmd, **cmd_args)
>>>>>           if res != {'return': {}}:
>>>>>               vm.shutdown()
>>>>>
>>>>
>>>> Worth adding a conditional to allow "hot" or "cold" runs? nah?
>>>>
>>>
>>> You mean, make this addition optional? Make sense
>>>
>>>
>>
>> I was thinking (along the lines of allowing both old and new behavior, 
>> in case anyone except you used these scripts) of this sort of thing:
>>
>> def bench_block_job(cmd, cmd_args, qemu_args, drop_cache=True): ...
>>
>> I don't insist on it; I was just earnestly wondering if it had any 
>> utility. If it doesn't, don't respin on my account.
>>
> 
> Ok, thanks a lot for reviewing! Still, I think, I'll resend
> 

Thanks for sharing your benchmarking scripts :)

--js