1 | The following changes since commit 8c5f94cd4182753959c8be8de415120dc879d8f0: | 1 | The following changes since commit b0fbe46ad82982b289a44ee2495b59b0bad8a842: |
---|---|---|---|
2 | 2 | ||
3 | Merge tag 'pull-loong-20211221-2' of https://gitlab.com/rth7680/qemu into staging (2021-12-21 13:30:35 -0800) | 3 | Update version for v2.11.0-rc0 release (2017-11-07 16:05:28 +0000) |
4 | 4 | ||
5 | are available in the Git repository at: | 5 | are available in the git repository at: |
6 | 6 | ||
7 | https://gitlab.com/hreitz/qemu.git tags/pull-block-2021-12-22 | 7 | git://github.com/stefanha/qemu.git tags/block-pull-request |
8 | 8 | ||
9 | for you to fetch changes up to 722f87df2545b308aec49b459b028f0802b4fd9e: | 9 | for you to fetch changes up to ef6dada8b44e1e7c4bec5c1115903af9af415b50: |
10 | 10 | ||
11 | iotests: check: multiprocessing support (2021-12-22 16:29:48 +0100) | 11 | util/async: use atomic_mb_set in qemu_bh_cancel (2017-11-08 19:09:15 +0000) |
12 | 12 | ||
13 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
14 | Block patches: | 14 | Pull request |
15 | - Added support to the iotests for running tests in several parallel | 15 | |
16 | jobs (using the new -j parameter) | 16 | v2: |
17 | * v1 emails 2/3 and 3/3 weren't sent due to an email failure | ||
18 | * Included Sergio's updated wording in the commit description | ||
17 | 19 | ||
18 | ---------------------------------------------------------------- | 20 | ---------------------------------------------------------------- |
19 | Vladimir Sementsov-Ogievskiy (3): | ||
20 | iotests/testrunner.py: add doc string for run_test() | ||
21 | iotests/testrunner.py: move updating last_elapsed to run_tests | ||
22 | iotests: check: multiprocessing support | ||
23 | 21 | ||
24 | tests/qemu-iotests/check | 4 +- | 22 | Sergio Lopez (1): |
25 | tests/qemu-iotests/testrunner.py | 86 ++++++++++++++++++++++++++++---- | 23 | util/async: use atomic_mb_set in qemu_bh_cancel |
26 | 2 files changed, 80 insertions(+), 10 deletions(-) | 24 | |
25 | Stefan Hajnoczi (1): | ||
26 | tests-aio-multithread: fix /aio/multi/schedule race condition | ||
27 | |||
28 | tests/test-aio-multithread.c | 5 ++--- | ||
29 | util/async.c | 2 +- | ||
30 | 2 files changed, 3 insertions(+), 4 deletions(-) | ||
27 | 31 | ||
28 | -- | 32 | -- |
29 | 2.33.1 | 33 | 2.13.6 |
30 | 34 | ||
31 | 35 | diff view generated by jsdifflib |
1 | From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | 1 | test_multi_co_schedule_entry() set to_schedule[id] in the final loop |
---|---|---|---|
2 | iteration before terminating the coroutine. There is a race condition | ||
3 | where the main thread attempts to enter the terminating or terminated | ||
4 | coroutine when signalling coroutines to stop: | ||
2 | 5 | ||
3 | Add -j <JOBS> parameter, to run tests in several jobs simultaneously. | 6 | atomic_mb_set(&now_stopping, true); |
4 | For realization - simply utilize multiprocessing.Pool class. | 7 | for (i = 0; i < NUM_CONTEXTS; i++) { |
8 | ctx_run(i, finish_cb, NULL); <--- enters dead coroutine! | ||
9 | to_schedule[i] = NULL; | ||
10 | } | ||
5 | 11 | ||
6 | Notes: | 12 | Make sure only to set to_schedule[id] if this coroutine really needs to |
13 | be scheduled! | ||
7 | 14 | ||
8 | 1. Of course, tests can't run simultaneously in same TEST_DIR. So, | 15 | Reported-by: "R.Nageswara Sastry" <nasastry@in.ibm.com> |
9 | use subdirectories TEST_DIR/testname/ and SOCK_DIR/testname/ | 16 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
10 | instead of simply TEST_DIR and SOCK_DIR | 17 | Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> |
18 | Message-id: 20171106190233.1175-1-stefanha@redhat.com | ||
19 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
20 | --- | ||
21 | tests/test-aio-multithread.c | 5 ++--- | ||
22 | 1 file changed, 2 insertions(+), 3 deletions(-) | ||
11 | 23 | ||
12 | 2. multiprocessing.Pool.starmap function doesn't support passing | 24 | diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c |
13 | context managers, so we can't simply pass "self". Happily, we need | ||
14 | self only for read-only access, and it just works if it is defined | ||
15 | in global space. So, add a temporary link TestRunner.shared_self | ||
16 | during run_tests(). | ||
17 | |||
18 | Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
19 | Message-Id: <20211203122223.2780098-4-vsementsov@virtuozzo.com> | ||
20 | Reviewed-by: John Snow <jsnow@redhat.com> | ||
21 | Tested-by: John Snow <jsnow@redhat.com> | ||
22 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
23 | --- | ||
24 | tests/qemu-iotests/check | 4 +- | ||
25 | tests/qemu-iotests/testrunner.py | 69 ++++++++++++++++++++++++++++---- | ||
26 | 2 files changed, 64 insertions(+), 9 deletions(-) | ||
27 | |||
28 | diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check | ||
29 | index XXXXXXX..XXXXXXX 100755 | ||
30 | --- a/tests/qemu-iotests/check | ||
31 | +++ b/tests/qemu-iotests/check | ||
32 | @@ -XXX,XX +XXX,XX @@ def make_argparser() -> argparse.ArgumentParser: | ||
33 | help='show me, do not run tests') | ||
34 | p.add_argument('-makecheck', action='store_true', | ||
35 | help='pretty print output for make check') | ||
36 | + p.add_argument('-j', dest='jobs', type=int, default=1, | ||
37 | + help='run tests in multiple parallel jobs') | ||
38 | |||
39 | p.add_argument('-d', dest='debug', action='store_true', help='debug') | ||
40 | p.add_argument('-p', dest='print', action='store_true', | ||
41 | @@ -XXX,XX +XXX,XX @@ if __name__ == '__main__': | ||
42 | with TestRunner(env, makecheck=args.makecheck, | ||
43 | color=args.color) as tr: | ||
44 | paths = [os.path.join(env.source_iotests, t) for t in tests] | ||
45 | - ok = tr.run_tests(paths) | ||
46 | + ok = tr.run_tests(paths, args.jobs) | ||
47 | if not ok: | ||
48 | sys.exit(1) | ||
49 | diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py | ||
50 | index XXXXXXX..XXXXXXX 100644 | 25 | index XXXXXXX..XXXXXXX 100644 |
51 | --- a/tests/qemu-iotests/testrunner.py | 26 | --- a/tests/test-aio-multithread.c |
52 | +++ b/tests/qemu-iotests/testrunner.py | 27 | +++ b/tests/test-aio-multithread.c |
53 | @@ -XXX,XX +XXX,XX @@ | 28 | @@ -XXX,XX +XXX,XX @@ static void finish_cb(void *opaque) |
54 | import json | 29 | static coroutine_fn void test_multi_co_schedule_entry(void *opaque) |
55 | import termios | 30 | { |
56 | import sys | 31 | g_assert(to_schedule[id] == NULL); |
57 | +from multiprocessing import Pool | 32 | - atomic_mb_set(&to_schedule[id], qemu_coroutine_self()); |
58 | from contextlib import contextmanager | 33 | |
59 | from typing import List, Optional, Iterator, Any, Sequence, Dict, \ | 34 | while (!atomic_mb_read(&now_stopping)) { |
60 | ContextManager | 35 | int n; |
61 | @@ -XXX,XX +XXX,XX @@ def __init__(self, status: str, description: str = '', | 36 | |
62 | 37 | n = g_test_rand_int_range(0, NUM_CONTEXTS); | |
63 | 38 | schedule_next(n); | |
64 | class TestRunner(ContextManager['TestRunner']): | ||
65 | + shared_self = None | ||
66 | + | 39 | + |
67 | + @staticmethod | 40 | + atomic_mb_set(&to_schedule[id], qemu_coroutine_self()); |
68 | + def proc_run_test(test: str, test_field_width: int) -> TestResult: | 41 | qemu_coroutine_yield(); |
69 | + # We are in a subprocess, we can't change the runner object! | 42 | - |
70 | + runner = TestRunner.shared_self | 43 | g_assert(to_schedule[id] == NULL); |
71 | + assert runner is not None | 44 | - atomic_mb_set(&to_schedule[id], qemu_coroutine_self()); |
72 | + return runner.run_test(test, test_field_width, mp=True) | 45 | } |
73 | + | 46 | } |
74 | + def run_tests_pool(self, tests: List[str], | ||
75 | + test_field_width: int, jobs: int) -> List[TestResult]: | ||
76 | + | ||
77 | + # passing self directly to Pool.starmap() just doesn't work, because | ||
78 | + # it's a context manager. | ||
79 | + assert TestRunner.shared_self is None | ||
80 | + TestRunner.shared_self = self | ||
81 | + | ||
82 | + with Pool(jobs) as p: | ||
83 | + results = p.starmap(self.proc_run_test, | ||
84 | + zip(tests, [test_field_width] * len(tests))) | ||
85 | + | ||
86 | + TestRunner.shared_self = None | ||
87 | + | ||
88 | + return results | ||
89 | + | ||
90 | def __init__(self, env: TestEnv, makecheck: bool = False, | ||
91 | color: str = 'auto') -> None: | ||
92 | self.env = env | ||
93 | @@ -XXX,XX +XXX,XX @@ def find_reference(self, test: str) -> str: | ||
94 | |||
95 | return f'{test}.out' | ||
96 | |||
97 | - def do_run_test(self, test: str) -> TestResult: | ||
98 | + def do_run_test(self, test: str, mp: bool) -> TestResult: | ||
99 | """ | ||
100 | Run one test | ||
101 | |||
102 | :param test: test file path | ||
103 | + :param mp: if true, we are in a multiprocessing environment, use | ||
104 | + personal subdirectories for test run | ||
105 | + | ||
106 | + Note: this method may be called from subprocess, so it does not | ||
107 | + change ``self`` object in any way! | ||
108 | """ | ||
109 | |||
110 | f_test = Path(test) | ||
111 | @@ -XXX,XX +XXX,XX @@ def do_run_test(self, test: str) -> TestResult: | ||
112 | |||
113 | args = [str(f_test.resolve())] | ||
114 | env = self.env.prepare_subprocess(args) | ||
115 | + if mp: | ||
116 | + # Split test directories, so that tests running in parallel don't | ||
117 | + # break each other. | ||
118 | + for d in ['TEST_DIR', 'SOCK_DIR']: | ||
119 | + env[d] = os.path.join(env[d], f_test.name) | ||
120 | + Path(env[d]).mkdir(parents=True, exist_ok=True) | ||
121 | |||
122 | t0 = time.time() | ||
123 | with f_bad.open('w', encoding="utf-8") as f: | ||
124 | @@ -XXX,XX +XXX,XX @@ def do_run_test(self, test: str) -> TestResult: | ||
125 | casenotrun=casenotrun) | ||
126 | |||
127 | def run_test(self, test: str, | ||
128 | - test_field_width: Optional[int] = None) -> TestResult: | ||
129 | + test_field_width: Optional[int] = None, | ||
130 | + mp: bool = False) -> TestResult: | ||
131 | """ | ||
132 | Run one test and print short status | ||
133 | |||
134 | :param test: test file path | ||
135 | :param test_field_width: width for first field of status format | ||
136 | + :param mp: if true, we are in a multiprocessing environment, don't try | ||
137 | + to rewrite things in stdout | ||
138 | + | ||
139 | + Note: this method may be called from subprocess, so it does not | ||
140 | + change ``self`` object in any way! | ||
141 | """ | ||
142 | |||
143 | last_el = self.last_elapsed.get(test) | ||
144 | start = datetime.datetime.now().strftime('%H:%M:%S') | ||
145 | |||
146 | if not self.makecheck: | ||
147 | - self.test_print_one_line(test=test, starttime=start, | ||
148 | - lasttime=last_el, end='\r', | ||
149 | + self.test_print_one_line(test=test, | ||
150 | + status = 'started' if mp else '...', | ||
151 | + starttime=start, | ||
152 | + lasttime=last_el, | ||
153 | + end = '\n' if mp else '\r', | ||
154 | test_field_width=test_field_width) | ||
155 | |||
156 | - res = self.do_run_test(test) | ||
157 | + res = self.do_run_test(test, mp) | ||
158 | |||
159 | end = datetime.datetime.now().strftime('%H:%M:%S') | ||
160 | self.test_print_one_line(test=test, status=res.status, | ||
161 | @@ -XXX,XX +XXX,XX @@ def run_test(self, test: str, | ||
162 | |||
163 | return res | ||
164 | |||
165 | - def run_tests(self, tests: List[str]) -> bool: | ||
166 | + def run_tests(self, tests: List[str], jobs: int = 1) -> bool: | ||
167 | n_run = 0 | ||
168 | failed = [] | ||
169 | notrun = [] | ||
170 | @@ -XXX,XX +XXX,XX @@ def run_tests(self, tests: List[str]) -> bool: | ||
171 | |||
172 | test_field_width = max(len(os.path.basename(t)) for t in tests) + 2 | ||
173 | |||
174 | - for t in tests: | ||
175 | + if jobs > 1: | ||
176 | + results = self.run_tests_pool(tests, test_field_width, jobs) | ||
177 | + | ||
178 | + for i, t in enumerate(tests): | ||
179 | name = os.path.basename(t) | ||
180 | - res = self.run_test(t, test_field_width=test_field_width) | ||
181 | + | ||
182 | + if jobs > 1: | ||
183 | + res = results[i] | ||
184 | + else: | ||
185 | + res = self.run_test(t, test_field_width) | ||
186 | |||
187 | assert res.status in ('pass', 'fail', 'not run') | ||
188 | 47 | ||
189 | -- | 48 | -- |
190 | 2.33.1 | 49 | 2.13.6 |
191 | 50 | ||
192 | 51 | diff view generated by jsdifflib |
1 | From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | 1 | From: Sergio Lopez <slp@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | We are going to modify these methods and will add more documentation in | 3 | Commit b7a745d added a qemu_bh_cancel call to the completion function |
4 | further commit. As a preparation add basic documentation. | 4 | as an optimization to prevent it from unnecessarily rescheduling itself. |
5 | 5 | ||
6 | Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | 6 | This completion function is scheduled from worker_thread, after setting |
7 | Message-Id: <20211203122223.2780098-2-vsementsov@virtuozzo.com> | 7 | the state of a ThreadPoolElement to THREAD_DONE. |
8 | Reviewed-by: John Snow <jsnow@redhat.com> | 8 | |
9 | Tested-by: John Snow <jsnow@redhat.com> | 9 | This was considered to be safe, as the completion function restarts the |
10 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | 10 | loop just after the call to qemu_bh_cancel. But, as this loop lacks a HW |
11 | memory barrier, the read of req->state may actually happen _before_ the | ||
12 | call, seeing it still as THREAD_QUEUED, and ending the completion | ||
13 | function without having processed a pending TPE linked at pool->head: | ||
14 | |||
15 | worker thread | I/O thread | ||
16 | ------------------------------------------------------------------------ | ||
17 | | speculatively read req->state | ||
18 | req->state = THREAD_DONE; | | ||
19 | qemu_bh_schedule(p->completion_bh) | | ||
20 | bh->scheduled = 1; | | ||
21 | | qemu_bh_cancel(p->completion_bh) | ||
22 | | bh->scheduled = 0; | ||
23 | | if (req->state == THREAD_DONE) | ||
24 | | // sees THREAD_QUEUED | ||
25 | |||
26 | The source of the misunderstanding was that qemu_bh_cancel is now being | ||
27 | used by the _consumer_ rather than the producer, and therefore now needs | ||
28 | to have acquire semantics just like e.g. aio_bh_poll. | ||
29 | |||
30 | In some situations, if there are no other independent requests in the | ||
31 | same aio context that could eventually trigger the scheduling of the | ||
32 | completion function, the omitted TPE and all operations pending on it | ||
33 | will get stuck forever. | ||
34 | |||
35 | [Added Sergio's updated wording about the HW memory barrier. | ||
36 | --Stefan] | ||
37 | |||
38 | Signed-off-by: Sergio Lopez <slp@redhat.com> | ||
39 | Message-id: 20171108063447.2842-1-slp@redhat.com | ||
40 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
11 | --- | 41 | --- |
12 | tests/qemu-iotests/testrunner.py | 13 +++++++++++++ | 42 | util/async.c | 2 +- |
13 | 1 file changed, 13 insertions(+) | 43 | 1 file changed, 1 insertion(+), 1 deletion(-) |
14 | 44 | ||
15 | diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py | 45 | diff --git a/util/async.c b/util/async.c |
16 | index XXXXXXX..XXXXXXX 100644 | 46 | index XXXXXXX..XXXXXXX 100644 |
17 | --- a/tests/qemu-iotests/testrunner.py | 47 | --- a/util/async.c |
18 | +++ b/tests/qemu-iotests/testrunner.py | 48 | +++ b/util/async.c |
19 | @@ -XXX,XX +XXX,XX @@ def find_reference(self, test: str) -> str: | 49 | @@ -XXX,XX +XXX,XX @@ void qemu_bh_schedule(QEMUBH *bh) |
20 | return f'{test}.out' | 50 | */ |
21 | 51 | void qemu_bh_cancel(QEMUBH *bh) | |
22 | def do_run_test(self, test: str) -> TestResult: | 52 | { |
23 | + """ | 53 | - bh->scheduled = 0; |
24 | + Run one test | 54 | + atomic_mb_set(&bh->scheduled, 0); |
25 | + | 55 | } |
26 | + :param test: test file path | 56 | |
27 | + """ | 57 | /* This func is async.The bottom half will do the delete action at the finial |
28 | + | ||
29 | f_test = Path(test) | ||
30 | f_bad = Path(f_test.name + '.out.bad') | ||
31 | f_notrun = Path(f_test.name + '.notrun') | ||
32 | @@ -XXX,XX +XXX,XX @@ def do_run_test(self, test: str) -> TestResult: | ||
33 | |||
34 | def run_test(self, test: str, | ||
35 | test_field_width: Optional[int] = None) -> TestResult: | ||
36 | + """ | ||
37 | + Run one test and print short status | ||
38 | + | ||
39 | + :param test: test file path | ||
40 | + :param test_field_width: width for first field of status format | ||
41 | + """ | ||
42 | + | ||
43 | last_el = self.last_elapsed.get(test) | ||
44 | start = datetime.datetime.now().strftime('%H:%M:%S') | ||
45 | |||
46 | -- | 58 | -- |
47 | 2.33.1 | 59 | 2.13.6 |
48 | 60 | ||
49 | 61 | diff view generated by jsdifflib |
Deleted patch | |||
---|---|---|---|
1 | From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
2 | 1 | ||
3 | We are going to use do_run_test() in multiprocessing environment, where | ||
4 | we'll not be able to change original runner object. | ||
5 | |||
6 | Happily, the only thing we change is that last_elapsed and it's simple | ||
7 | to do it in run_tests() instead. All other accesses to self in | ||
8 | do_runt_test() and in run_test() are read-only. | ||
9 | |||
10 | Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
11 | Message-Id: <20211203122223.2780098-3-vsementsov@virtuozzo.com> | ||
12 | Reviewed-by: John Snow <jsnow@redhat.com> | ||
13 | Tested-by: John Snow <jsnow@redhat.com> | ||
14 | Signed-off-by: Hanna Reitz <hreitz@redhat.com> | ||
15 | --- | ||
16 | tests/qemu-iotests/testrunner.py | 4 +++- | ||
17 | 1 file changed, 3 insertions(+), 1 deletion(-) | ||
18 | |||
19 | diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py | ||
20 | index XXXXXXX..XXXXXXX 100644 | ||
21 | --- a/tests/qemu-iotests/testrunner.py | ||
22 | +++ b/tests/qemu-iotests/testrunner.py | ||
23 | @@ -XXX,XX +XXX,XX @@ def do_run_test(self, test: str) -> TestResult: | ||
24 | diff=diff, casenotrun=casenotrun) | ||
25 | else: | ||
26 | f_bad.unlink() | ||
27 | - self.last_elapsed.update(test, elapsed) | ||
28 | return TestResult(status='pass', elapsed=elapsed, | ||
29 | casenotrun=casenotrun) | ||
30 | |||
31 | @@ -XXX,XX +XXX,XX @@ def run_tests(self, tests: List[str]) -> bool: | ||
32 | print('\n'.join(res.diff)) | ||
33 | elif res.status == 'not run': | ||
34 | notrun.append(name) | ||
35 | + elif res.status == 'pass': | ||
36 | + assert res.elapsed is not None | ||
37 | + self.last_elapsed.update(t, res.elapsed) | ||
38 | |||
39 | sys.stdout.flush() | ||
40 | if res.interrupted: | ||
41 | -- | ||
42 | 2.33.1 | ||
43 | |||
44 | diff view generated by jsdifflib |