| Message ID | 1503471071-2233-5-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Peter Xu (peterx@redhat.com) wrote: > Introducing this new parameter for QMP commands in general to mark out > when the command does not need BQL. Normally QMP command executions are > done with the protection of BQL in QEMU. However the truth is that not > all the QMP commands require the BQL. > > This new parameter provides a way to allow QMP commands to run in > parallel when possible, without the contention on the BQL. > > Since the default value of "without-bql" is still false, so now all QMP > commands are still protected by BQL still. > > Signed-off-by: Peter Xu <peterx@redhat.com> We should define what a 'without-bql' command is allowed to do: 'Commands that have without-bql set _may_ be called without the bql being taken. They must not take the bql or any other lock that may become dependent on the bql.' (Do we need to say anything about RCU?) Also, 'no-bql' is shorter :-) Dave > --- > docs/devel/qapi-code-gen.txt | 10 +++++++++- > include/qapi/qmp/dispatch.h | 1 + > qapi/qmp-dispatch.c | 11 +++++++++++ > scripts/qapi-commands.py | 18 +++++++++++++----- > scripts/qapi-introspect.py | 2 +- > scripts/qapi.py | 15 ++++++++++----- > scripts/qapi2texi.py | 2 +- > tests/qapi-schema/test-qapi.py | 2 +- > 8 files changed, 47 insertions(+), 14 deletions(-) > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > index 9903ac4..4960d00 100644 > --- a/docs/devel/qapi-code-gen.txt > +++ b/docs/devel/qapi-code-gen.txt > @@ -556,7 +556,8 @@ following example objects: > > Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, > '*returns': TYPE-NAME, '*boxed': true, > - '*gen': false, '*success-response': false } > + '*gen': false, '*success-response': false, > + '*without-bql': false } > > Commands are defined by using a dictionary containing several members, > where three members are most common. The 'command' member is a > @@ -636,6 +637,13 @@ possible, the command expression should include the optional key > 'success-response' with boolean value false. So far, only QGA makes > use of this member. > > +Most of the commands require the Big QEMU Lock (BQL) be held during > +execution. However, there is a small subset of the commands that may > +not really need BQL at all. To mark out this kind of commands, we can > +specify "without-bql" to "true". This parameter is only a hint for > +internal QMP implementation to provide possiblility to allow commands > +be run in parallel, or reduce the contention of the lock. Users of QMP > +should not really be aware of such information. Well, I think users of these commands might select them specifically because they know that they won't block. Those who care about latency might look to use commands that don't take the lock because of a reduced effect on the performance as well. Dave > === Events === > > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h > index 20578dc..ec5c620 100644 > --- a/include/qapi/qmp/dispatch.h > +++ b/include/qapi/qmp/dispatch.h > @@ -23,6 +23,7 @@ typedef enum QmpCommandOptions > { > QCO_NO_OPTIONS = 0x0, > QCO_NO_SUCCESS_RESP = 0x1, > + QCO_WITHOUT_BQL = 0x2, > } QmpCommandOptions; > > typedef struct QmpCommand > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c > index 3b6b224..b7fba5e 100644 > --- a/qapi/qmp-dispatch.c > +++ b/qapi/qmp-dispatch.c > @@ -107,6 +107,17 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, > QINCREF(args); > } > > + if (cmd->options & QCO_WITHOUT_BQL) { > + /* > + * If this command can live without BQL, then we don't take > + * it. One thing to mention: we may have already taken the > + * BQL before reaching here. If so, we just keep it. So > + * generally speaking we are trying our best on reducing the > + * contention of BQL. > + */ > + take_bql = false; > + } > + > if (take_bql) { > qemu_mutex_lock_iothread(); > } > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 974d0a4..155a0a4 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -192,10 +192,17 @@ out: > return ret > > > -def gen_register_command(name, success_response): > - options = 'QCO_NO_OPTIONS' > +def gen_register_command(name, success_response, without_bql): > + options = [] > + > if not success_response: > - options = 'QCO_NO_SUCCESS_RESP' > + options += ['QCO_NO_SUCCESS_RESP'] > + if without_bql: > + options += ['QCO_WITHOUT_BQL'] > + > + if not options: > + options = ['QCO_NO_OPTIONS'] > + options = " | ".join(options) > > ret = mcgen(''' > qmp_register_command(cmds, "%(name)s", > @@ -241,7 +248,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor): > self._visited_ret_types = None > > def visit_command(self, name, info, arg_type, ret_type, > - gen, success_response, boxed): > + gen, success_response, boxed, without_bql): > if not gen: > return > self.decl += gen_command_decl(name, arg_type, boxed, ret_type) > @@ -250,7 +257,8 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor): > self.defn += gen_marshal_output(ret_type) > self.decl += gen_marshal_decl(name) > self.defn += gen_marshal(name, arg_type, boxed, ret_type) > - self._regy += gen_register_command(name, success_response) > + self._regy += gen_register_command(name, success_response, > + without_bql) > > > (input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line() > diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py > index 032bcea..a523544 100644 > --- a/scripts/qapi-introspect.py > +++ b/scripts/qapi-introspect.py > @@ -154,7 +154,7 @@ const char %(c_name)s[] = %(c_string)s; > for m in variants.variants]}) > > def visit_command(self, name, info, arg_type, ret_type, > - gen, success_response, boxed): > + gen, success_response, boxed, without_bql): > arg_type = arg_type or self._schema.the_empty_object_type > ret_type = ret_type or self._schema.the_empty_object_type > self._gen_json(name, 'command', > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 8aa2775..3951143 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -920,7 +920,8 @@ def check_exprs(exprs): > elif 'command' in expr: > meta = 'command' > check_keys(expr_elem, 'command', [], > - ['data', 'returns', 'gen', 'success-response', 'boxed']) > + ['data', 'returns', 'gen', 'success-response', > + 'boxed', 'without-bql']) > elif 'event' in expr: > meta = 'event' > check_keys(expr_elem, 'event', [], ['data', 'boxed']) > @@ -1031,7 +1032,7 @@ class QAPISchemaVisitor(object): > pass > > def visit_command(self, name, info, arg_type, ret_type, > - gen, success_response, boxed): > + gen, success_response, boxed, without_bql): > pass > > def visit_event(self, name, info, arg_type, boxed): > @@ -1398,7 +1399,7 @@ class QAPISchemaAlternateType(QAPISchemaType): > > class QAPISchemaCommand(QAPISchemaEntity): > def __init__(self, name, info, doc, arg_type, ret_type, > - gen, success_response, boxed): > + gen, success_response, boxed, without_bql): > QAPISchemaEntity.__init__(self, name, info, doc) > assert not arg_type or isinstance(arg_type, str) > assert not ret_type or isinstance(ret_type, str) > @@ -1408,6 +1409,7 @@ class QAPISchemaCommand(QAPISchemaEntity): > self.ret_type = None > self.gen = gen > self.success_response = success_response > + self.without_bql = without_bql > self.boxed = boxed > > def check(self, schema): > @@ -1432,7 +1434,8 @@ class QAPISchemaCommand(QAPISchemaEntity): > def visit(self, visitor): > visitor.visit_command(self.name, self.info, > self.arg_type, self.ret_type, > - self.gen, self.success_response, self.boxed) > + self.gen, self.success_response, > + self.boxed, self.without_bql) > > > class QAPISchemaEvent(QAPISchemaEntity): > @@ -1639,6 +1642,7 @@ class QAPISchema(object): > rets = expr.get('returns') > gen = expr.get('gen', True) > success_response = expr.get('success-response', True) > + without_bql = expr.get('without-bql', False) > boxed = expr.get('boxed', False) > if isinstance(data, OrderedDict): > data = self._make_implicit_object_type( > @@ -1647,7 +1651,8 @@ class QAPISchema(object): > assert len(rets) == 1 > rets = self._make_array_type(rets[0], info) > self._def_entity(QAPISchemaCommand(name, info, doc, data, rets, > - gen, success_response, boxed)) > + gen, success_response, > + boxed, without_bql)) > > def _def_event(self, expr, info, doc): > name = expr['event'] > diff --git a/scripts/qapi2texi.py b/scripts/qapi2texi.py > index a317526..659bd83 100755 > --- a/scripts/qapi2texi.py > +++ b/scripts/qapi2texi.py > @@ -236,7 +236,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor): > body=texi_entity(doc, 'Members')) > > def visit_command(self, name, info, arg_type, ret_type, > - gen, success_response, boxed): > + gen, success_response, boxed, without_bql): > doc = self.cur_doc > if self.out: > self.out += '\n' > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py > index c7724d3..15aff29 100644 > --- a/tests/qapi-schema/test-qapi.py > +++ b/tests/qapi-schema/test-qapi.py > @@ -36,7 +36,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): > self._print_variants(variants) > > def visit_command(self, name, info, arg_type, ret_type, > - gen, success_response, boxed): > + gen, success_response, boxed, without_bql): > print 'command %s %s -> %s' % \ > (name, arg_type and arg_type.name, ret_type and ret_type.name) > print ' gen=%s success_response=%s boxed=%s' % \ > -- > 2.7.4 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, 08/23 18:44, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > Introducing this new parameter for QMP commands in general to mark out > > when the command does not need BQL. Normally QMP command executions are > > done with the protection of BQL in QEMU. However the truth is that not > > all the QMP commands require the BQL. > > > > This new parameter provides a way to allow QMP commands to run in > > parallel when possible, without the contention on the BQL. > > > > Since the default value of "without-bql" is still false, so now all QMP > > commands are still protected by BQL still. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > We should define what a 'without-bql' command is allowed to do: > 'Commands that have without-bql set _may_ be called without the bql > being taken. They must not take the bql or any other lock that may > become dependent on the bql.' > (Do we need to say anything about RCU?) > > Also, 'no-bql' is shorter :-) Or rather "need-bql" that defaults to true to avoid double negative (TM) with "no-bql = false"? Fam
On Wed, Aug 23, 2017 at 06:44:12PM +0100, Dr. David Alan Gilbert wrote: [...] > > +Most of the commands require the Big QEMU Lock (BQL) be held during > > +execution. However, there is a small subset of the commands that may > > +not really need BQL at all. To mark out this kind of commands, we can > > +specify "without-bql" to "true". This parameter is only a hint for > > +internal QMP implementation to provide possiblility to allow commands > > +be run in parallel, or reduce the contention of the lock. Users of QMP > > +should not really be aware of such information. > > Well, I think users of these commands might select them specifically > because they know that they won't block. Those who care about latency might > look to use commands that don't take the lock because of a reduced > effect on the performance as well. What would be the best way to tell user? I think again this should mostly for HMP only, right? Maybe we can add a new command to list these lock-free commands. Or, I can dump something in "help" and "help info" like: (qemu) help migrate_incoming migrate_incoming uri -- Continue an incoming migration from an -incoming defer (BQL-less)
On Thu, Aug 24, 2017 at 07:37:32AM +0800, Fam Zheng wrote: > On Wed, 08/23 18:44, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > Introducing this new parameter for QMP commands in general to mark out > > > when the command does not need BQL. Normally QMP command executions are > > > done with the protection of BQL in QEMU. However the truth is that not > > > all the QMP commands require the BQL. > > > > > > This new parameter provides a way to allow QMP commands to run in > > > parallel when possible, without the contention on the BQL. > > > > > > Since the default value of "without-bql" is still false, so now all QMP > > > commands are still protected by BQL still. > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > We should define what a 'without-bql' command is allowed to do: > > 'Commands that have without-bql set _may_ be called without the bql > > being taken. They must not take the bql or any other lock that may > > become dependent on the bql.' Sure. > > (Do we need to say anything about RCU?) Could I ask how is RCU related? > > > > Also, 'no-bql' is shorter :-) > > Or rather "need-bql" that defaults to true to avoid double negative (TM) with > "no-bql = false"? Ok let me use "need-bql". :)
* Peter Xu (peterx@redhat.com) wrote: > On Wed, Aug 23, 2017 at 06:44:12PM +0100, Dr. David Alan Gilbert wrote: > > [...] > > > > +Most of the commands require the Big QEMU Lock (BQL) be held during > > > +execution. However, there is a small subset of the commands that may > > > +not really need BQL at all. To mark out this kind of commands, we can > > > +specify "without-bql" to "true". This parameter is only a hint for > > > +internal QMP implementation to provide possiblility to allow commands > > > +be run in parallel, or reduce the contention of the lock. Users of QMP > > > +should not really be aware of such information. > > > > Well, I think users of these commands might select them specifically > > because they know that they won't block. Those who care about latency might > > look to use commands that don't take the lock because of a reduced > > effect on the performance as well. > > What would be the best way to tell user? I think again this should > mostly for HMP only, right? It needs to be docuemnted for QMP users as well so that those developing management code know what's safe. > Maybe we can add a new command to list these lock-free commands. Or, > I can dump something in "help" and "help info" like: > > (qemu) help migrate_incoming > migrate_incoming uri -- Continue an incoming migration from an -incoming defer (BQL-less) 'lock free' might be better? Dave > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Peter Xu (peterx@redhat.com) wrote: > On Thu, Aug 24, 2017 at 07:37:32AM +0800, Fam Zheng wrote: > > On Wed, 08/23 18:44, Dr. David Alan Gilbert wrote: > > > * Peter Xu (peterx@redhat.com) wrote: > > > > Introducing this new parameter for QMP commands in general to mark out > > > > when the command does not need BQL. Normally QMP command executions are > > > > done with the protection of BQL in QEMU. However the truth is that not > > > > all the QMP commands require the BQL. > > > > > > > > This new parameter provides a way to allow QMP commands to run in > > > > parallel when possible, without the contention on the BQL. > > > > > > > > Since the default value of "without-bql" is still false, so now all QMP > > > > commands are still protected by BQL still. > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > > We should define what a 'without-bql' command is allowed to do: > > > 'Commands that have without-bql set _may_ be called without the bql > > > being taken. They must not take the bql or any other lock that may > > > become dependent on the bql.' > > Sure. > > > > (Do we need to say anything about RCU?) > > Could I ask how is RCU related? My definition above said that anything declared without bql couldn't take the bql, so couldn't block on any other thread holding the bql. But is our command allowed to use synchronize_rcu or rcu_read_lock that could wait for or block other threads doing rcu stuff? Because if it did is there any guarantee that it wouldn't block? > > > > > > > Also, 'no-bql' is shorter :-) > > > > Or rather "need-bql" that defaults to true to avoid double negative (TM) with > > "no-bql = false"? > > Ok let me use "need-bql". :) Fine by me. Dave > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Aug 25, 2017 at 10:14:12AM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > On Thu, Aug 24, 2017 at 07:37:32AM +0800, Fam Zheng wrote: > > > On Wed, 08/23 18:44, Dr. David Alan Gilbert wrote: > > > > * Peter Xu (peterx@redhat.com) wrote: > > > > > Introducing this new parameter for QMP commands in general to mark out > > > > > when the command does not need BQL. Normally QMP command executions are > > > > > done with the protection of BQL in QEMU. However the truth is that not > > > > > all the QMP commands require the BQL. > > > > > > > > > > This new parameter provides a way to allow QMP commands to run in > > > > > parallel when possible, without the contention on the BQL. > > > > > > > > > > Since the default value of "without-bql" is still false, so now all QMP > > > > > commands are still protected by BQL still. > > > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > > > > We should define what a 'without-bql' command is allowed to do: > > > > 'Commands that have without-bql set _may_ be called without the bql > > > > being taken. They must not take the bql or any other lock that may > > > > become dependent on the bql.' > > > > Sure. > > > > > > (Do we need to say anything about RCU?) > > > > Could I ask how is RCU related? > > My definition above said that anything declared without bql couldn't > take the bql, so couldn't block on any other thread holding the bql. > But is our command allowed to use synchronize_rcu or rcu_read_lock > that could wait for or block other threads doing rcu stuff? > Because if it did is there any guarantee that it wouldn't block? I see. Shall we just ignore RCU for now? Since currently I don't see a real synchronize_rcu() user yet in QEMU, except the RCU thread. And rcu_read_lock() should not block itself, so IMHO calling it only in monitor command handlers should always be fine? > > > > > > > > > > > > Also, 'no-bql' is shorter :-) > > > > > > Or rather "need-bql" that defaults to true to avoid double negative (TM) with > > > "no-bql = false"? > > > > Ok let me use "need-bql". :) > > Fine by me. I'm switching to "need-bql" for QMP only, and used "no-bql" in HMP, since I failed to find a good way to init mon_cmd_t field to true by default.
On Fri, Aug 25, 2017 at 10:06:27AM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > On Wed, Aug 23, 2017 at 06:44:12PM +0100, Dr. David Alan Gilbert wrote: > > > > [...] > > > > > > +Most of the commands require the Big QEMU Lock (BQL) be held during > > > > +execution. However, there is a small subset of the commands that may > > > > +not really need BQL at all. To mark out this kind of commands, we can > > > > +specify "without-bql" to "true". This parameter is only a hint for > > > > +internal QMP implementation to provide possiblility to allow commands > > > > +be run in parallel, or reduce the contention of the lock. Users of QMP > > > > +should not really be aware of such information. > > > > > > Well, I think users of these commands might select them specifically > > > because they know that they won't block. Those who care about latency might > > > look to use commands that don't take the lock because of a reduced > > > effect on the performance as well. > > > > What would be the best way to tell user? I think again this should > > mostly for HMP only, right? > > It needs to be docuemnted for QMP users as well so that those developing > management code know what's safe. I see. What's the corresponding QMP documentation I should touch up? > > > Maybe we can add a new command to list these lock-free commands. Or, > > I can dump something in "help" and "help info" like: > > > > (qemu) help migrate_incoming > > migrate_incoming uri -- Continue an incoming migration from an -incoming defer (BQL-less) > > 'lock free' might be better? I'm ok with it. But would the word "lock" too general?
* Peter Xu (peterx@redhat.com) wrote: > On Fri, Aug 25, 2017 at 10:14:12AM +0100, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > On Thu, Aug 24, 2017 at 07:37:32AM +0800, Fam Zheng wrote: > > > > On Wed, 08/23 18:44, Dr. David Alan Gilbert wrote: > > > > > * Peter Xu (peterx@redhat.com) wrote: > > > > > > Introducing this new parameter for QMP commands in general to mark out > > > > > > when the command does not need BQL. Normally QMP command executions are > > > > > > done with the protection of BQL in QEMU. However the truth is that not > > > > > > all the QMP commands require the BQL. > > > > > > > > > > > > This new parameter provides a way to allow QMP commands to run in > > > > > > parallel when possible, without the contention on the BQL. > > > > > > > > > > > > Since the default value of "without-bql" is still false, so now all QMP > > > > > > commands are still protected by BQL still. > > > > > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > > > > > > We should define what a 'without-bql' command is allowed to do: > > > > > 'Commands that have without-bql set _may_ be called without the bql > > > > > being taken. They must not take the bql or any other lock that may > > > > > become dependent on the bql.' > > > > > > Sure. > > > > > > > > (Do we need to say anything about RCU?) > > > > > > Could I ask how is RCU related? > > > > My definition above said that anything declared without bql couldn't > > take the bql, so couldn't block on any other thread holding the bql. > > But is our command allowed to use synchronize_rcu or rcu_read_lock > > that could wait for or block other threads doing rcu stuff? > > Because if it did is there any guarantee that it wouldn't block? > > I see. Shall we just ignore RCU for now? Since currently I don't see > a real synchronize_rcu() user yet in QEMU, except the RCU thread. And > rcu_read_lock() should not block itself, so IMHO calling it only in > monitor command handlers should always be fine? Yes, I think you're right that the rcu_read_lock is OK; just something to keep in mind. Dave > > > > > > > > > > > > > > > > > Also, 'no-bql' is shorter :-) > > > > > > > > Or rather "need-bql" that defaults to true to avoid double negative (TM) with > > > > "no-bql = false"? > > > > > > Ok let me use "need-bql". :) > > > > Fine by me. > > I'm switching to "need-bql" for QMP only, and used "no-bql" in HMP, > since I failed to find a good way to init mon_cmd_t field to true by > default. > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Peter Xu (peterx@redhat.com) wrote: > On Fri, Aug 25, 2017 at 10:06:27AM +0100, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > On Wed, Aug 23, 2017 at 06:44:12PM +0100, Dr. David Alan Gilbert wrote: > > > > > > [...] > > > > > > > > +Most of the commands require the Big QEMU Lock (BQL) be held during > > > > > +execution. However, there is a small subset of the commands that may > > > > > +not really need BQL at all. To mark out this kind of commands, we can > > > > > +specify "without-bql" to "true". This parameter is only a hint for > > > > > +internal QMP implementation to provide possiblility to allow commands > > > > > +be run in parallel, or reduce the contention of the lock. Users of QMP > > > > > +should not really be aware of such information. > > > > > > > > Well, I think users of these commands might select them specifically > > > > because they know that they won't block. Those who care about latency might > > > > look to use commands that don't take the lock because of a reduced > > > > effect on the performance as well. > > > > > > What would be the best way to tell user? I think again this should > > > mostly for HMP only, right? > > > > It needs to be docuemnted for QMP users as well so that those developing > > management code know what's safe. > > I see. What's the corresponding QMP documentation I should touch up? I'm not sure, but based on the long thread; I think the idea is to add something to the schema so the flag appears in the introspection. I'll leave the details of how to Markus. > > > > > Maybe we can add a new command to list these lock-free commands. Or, > > > I can dump something in "help" and "help info" like: > > > > > > (qemu) help migrate_incoming > > > migrate_incoming uri -- Continue an incoming migration from an -incoming defer (BQL-less) > > > > 'lock free' might be better? > > I'm ok with it. But would the word "lock" too general? Maybe, but it's probably not just BQL. Dave > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 9903ac4..4960d00 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -556,7 +556,8 @@ following example objects: Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, '*returns': TYPE-NAME, '*boxed': true, - '*gen': false, '*success-response': false } + '*gen': false, '*success-response': false, + '*without-bql': false } Commands are defined by using a dictionary containing several members, where three members are most common. The 'command' member is a @@ -636,6 +637,13 @@ possible, the command expression should include the optional key 'success-response' with boolean value false. So far, only QGA makes use of this member. +Most of the commands require the Big QEMU Lock (BQL) be held during +execution. However, there is a small subset of the commands that may +not really need BQL at all. To mark out this kind of commands, we can +specify "without-bql" to "true". This parameter is only a hint for +internal QMP implementation to provide possiblility to allow commands +be run in parallel, or reduce the contention of the lock. Users of QMP +should not really be aware of such information. === Events === diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 20578dc..ec5c620 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -23,6 +23,7 @@ typedef enum QmpCommandOptions { QCO_NO_OPTIONS = 0x0, QCO_NO_SUCCESS_RESP = 0x1, + QCO_WITHOUT_BQL = 0x2, } QmpCommandOptions; typedef struct QmpCommand diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 3b6b224..b7fba5e 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -107,6 +107,17 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, QINCREF(args); } + if (cmd->options & QCO_WITHOUT_BQL) { + /* + * If this command can live without BQL, then we don't take + * it. One thing to mention: we may have already taken the + * BQL before reaching here. If so, we just keep it. So + * generally speaking we are trying our best on reducing the + * contention of BQL. + */ + take_bql = false; + } + if (take_bql) { qemu_mutex_lock_iothread(); } diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 974d0a4..155a0a4 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -192,10 +192,17 @@ out: return ret -def gen_register_command(name, success_response): - options = 'QCO_NO_OPTIONS' +def gen_register_command(name, success_response, without_bql): + options = [] + if not success_response: - options = 'QCO_NO_SUCCESS_RESP' + options += ['QCO_NO_SUCCESS_RESP'] + if without_bql: + options += ['QCO_WITHOUT_BQL'] + + if not options: + options = ['QCO_NO_OPTIONS'] + options = " | ".join(options) ret = mcgen(''' qmp_register_command(cmds, "%(name)s", @@ -241,7 +248,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor): self._visited_ret_types = None def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, without_bql): if not gen: return self.decl += gen_command_decl(name, arg_type, boxed, ret_type) @@ -250,7 +257,8 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor): self.defn += gen_marshal_output(ret_type) self.decl += gen_marshal_decl(name) self.defn += gen_marshal(name, arg_type, boxed, ret_type) - self._regy += gen_register_command(name, success_response) + self._regy += gen_register_command(name, success_response, + without_bql) (input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line() diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py index 032bcea..a523544 100644 --- a/scripts/qapi-introspect.py +++ b/scripts/qapi-introspect.py @@ -154,7 +154,7 @@ const char %(c_name)s[] = %(c_string)s; for m in variants.variants]}) def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, without_bql): arg_type = arg_type or self._schema.the_empty_object_type ret_type = ret_type or self._schema.the_empty_object_type self._gen_json(name, 'command', diff --git a/scripts/qapi.py b/scripts/qapi.py index 8aa2775..3951143 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -920,7 +920,8 @@ def check_exprs(exprs): elif 'command' in expr: meta = 'command' check_keys(expr_elem, 'command', [], - ['data', 'returns', 'gen', 'success-response', 'boxed']) + ['data', 'returns', 'gen', 'success-response', + 'boxed', 'without-bql']) elif 'event' in expr: meta = 'event' check_keys(expr_elem, 'event', [], ['data', 'boxed']) @@ -1031,7 +1032,7 @@ class QAPISchemaVisitor(object): pass def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, without_bql): pass def visit_event(self, name, info, arg_type, boxed): @@ -1398,7 +1399,7 @@ class QAPISchemaAlternateType(QAPISchemaType): class QAPISchemaCommand(QAPISchemaEntity): def __init__(self, name, info, doc, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, without_bql): QAPISchemaEntity.__init__(self, name, info, doc) assert not arg_type or isinstance(arg_type, str) assert not ret_type or isinstance(ret_type, str) @@ -1408,6 +1409,7 @@ class QAPISchemaCommand(QAPISchemaEntity): self.ret_type = None self.gen = gen self.success_response = success_response + self.without_bql = without_bql self.boxed = boxed def check(self, schema): @@ -1432,7 +1434,8 @@ class QAPISchemaCommand(QAPISchemaEntity): def visit(self, visitor): visitor.visit_command(self.name, self.info, self.arg_type, self.ret_type, - self.gen, self.success_response, self.boxed) + self.gen, self.success_response, + self.boxed, self.without_bql) class QAPISchemaEvent(QAPISchemaEntity): @@ -1639,6 +1642,7 @@ class QAPISchema(object): rets = expr.get('returns') gen = expr.get('gen', True) success_response = expr.get('success-response', True) + without_bql = expr.get('without-bql', False) boxed = expr.get('boxed', False) if isinstance(data, OrderedDict): data = self._make_implicit_object_type( @@ -1647,7 +1651,8 @@ class QAPISchema(object): assert len(rets) == 1 rets = self._make_array_type(rets[0], info) self._def_entity(QAPISchemaCommand(name, info, doc, data, rets, - gen, success_response, boxed)) + gen, success_response, + boxed, without_bql)) def _def_event(self, expr, info, doc): name = expr['event'] diff --git a/scripts/qapi2texi.py b/scripts/qapi2texi.py index a317526..659bd83 100755 --- a/scripts/qapi2texi.py +++ b/scripts/qapi2texi.py @@ -236,7 +236,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor): body=texi_entity(doc, 'Members')) def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, without_bql): doc = self.cur_doc if self.out: self.out += '\n' diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index c7724d3..15aff29 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -36,7 +36,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): self._print_variants(variants) def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, without_bql): print 'command %s %s -> %s' % \ (name, arg_type and arg_type.name, ret_type and ret_type.name) print ' gen=%s success_response=%s boxed=%s' % \
Introducing this new parameter for QMP commands in general to mark out when the command does not need BQL. Normally QMP command executions are done with the protection of BQL in QEMU. However the truth is that not all the QMP commands require the BQL. This new parameter provides a way to allow QMP commands to run in parallel when possible, without the contention on the BQL. Since the default value of "without-bql" is still false, so now all QMP commands are still protected by BQL still. Signed-off-by: Peter Xu <peterx@redhat.com> --- docs/devel/qapi-code-gen.txt | 10 +++++++++- include/qapi/qmp/dispatch.h | 1 + qapi/qmp-dispatch.c | 11 +++++++++++ scripts/qapi-commands.py | 18 +++++++++++++----- scripts/qapi-introspect.py | 2 +- scripts/qapi.py | 15 ++++++++++----- scripts/qapi2texi.py | 2 +- tests/qapi-schema/test-qapi.py | 2 +- 8 files changed, 47 insertions(+), 14 deletions(-)