| Message ID | 1469603005-5992-6-git-send-email-blp@ovn.org |
|---|---|
| State | Deferred |
| Headers | show |
"dev" <dev-bounces@openvswitch.org> wrote on 07/27/2016 02:03:25 AM: > From: Ben Pfaff <blp@ovn.org> > To: dev@openvswitch.org > Cc: Ben Pfaff <blp@ovn.org> > Date: 07/27/2016 02:04 AM > Subject: [ovs-dev] [PATCH 5/5] ovsdb-idl: Wake up ovsdb_idl_loop > when a transaction commits. > Sent by: "dev" <dev-bounces@openvswitch.org> > > There is a fair amount of code that defers modifying the database when a > transaction cannot be created (because there is already one outstanding). > This code tends to assume that the main loop will wake up again when it > becomes possible again to modify the database, but the actual ovsdb_id_loop > implementation only did this if the database had changed. This is too > conservative a policy and may account for some failures I've seen in tests. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- Ben, while I can't tell you to hold up on this if somebody else acks it, my first thought (after looking at it) is that checking it against the unit tests cases answers only half of the questions that it needs to answer. Since it is changing the cycle time, there is the question of whether it provides a measurable change in E2E performance. Therefore, with your indulgence, I'm going to try and throw it into an openstack cloud tomorrow and run some rally tests against it to see if indeed does result in a measurable change in performance. Ryan
On Thu, Jul 28, 2016 at 10:24:43PM -0500, Ryan Moats wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 07/27/2016 02:03:25 AM: > > > From: Ben Pfaff <blp@ovn.org> > > To: dev@openvswitch.org > > Cc: Ben Pfaff <blp@ovn.org> > > Date: 07/27/2016 02:04 AM > > Subject: [ovs-dev] [PATCH 5/5] ovsdb-idl: Wake up ovsdb_idl_loop > > when a transaction commits. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > There is a fair amount of code that defers modifying the database when a > > transaction cannot be created (because there is already one outstanding). > > This code tends to assume that the main loop will wake up again when it > > becomes possible again to modify the database, but the actual > ovsdb_id_loop > > implementation only did this if the database had changed. This is too > > conservative a policy and may account for some failures I've seen in > tests. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > Ben, while I can't tell you to hold up on this if somebody else acks it, > my first thought (after looking at it) is that checking it against the > unit tests cases answers only half of the questions that it needs to > answer. > Since it is changing the cycle time, there is the question of whether it > provides a measurable change in E2E performance. Therefore, with your > indulgence, I'm going to try and throw it into an openstack cloud tomorrow > and run some rally tests against it to see if indeed does result in a > measurable change in performance. This commit is not supposed to be a performance fix, it's supposed to be a correctness fix. I don't have a really good example of the kind of thing that it fixes so I'm happy to sit on it for a while until I do.
On Fri, Jul 29, 2016 at 1:46 AM, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Jul 28, 2016 at 10:24:43PM -0500, Ryan Moats wrote: > > "dev" <dev-bounces@openvswitch.org> wrote on 07/27/2016 02:03:25 AM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: dev@openvswitch.org > > > Cc: Ben Pfaff <blp@ovn.org> > > > Date: 07/27/2016 02:04 AM > > > Subject: [ovs-dev] [PATCH 5/5] ovsdb-idl: Wake up ovsdb_idl_loop > > > when a transaction commits. > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > There is a fair amount of code that defers modifying the database when > a > > > transaction cannot be created (because there is already one > outstanding). > > > This code tends to assume that the main loop will wake up again when it > > > becomes possible again to modify the database, but the actual > > ovsdb_id_loop > > > implementation only did this if the database had changed. This is too > > > conservative a policy and may account for some failures I've seen in > > tests. > > > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > --- > > > > Ben, while I can't tell you to hold up on this if somebody else acks it, > > my first thought (after looking at it) is that checking it against the > > unit tests cases answers only half of the questions that it needs to > > answer. > > Since it is changing the cycle time, there is the question of whether it > > provides a measurable change in E2E performance. Therefore, with your > > indulgence, I'm going to try and throw it into an openstack cloud > tomorrow > > and run some rally tests against it to see if indeed does result in a > > measurable change in performance. > > This commit is not supposed to be a performance fix, it's supposed to be > a correctness fix. > > I don't have a really good example of the kind of thing that it fixes so > I'm happy to sit on it for a while until I do. It makes sense to me. Acked-by: Russell Bryant <russell@ovn.org>
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 816cc70..38110c9 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -3712,13 +3712,10 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop) break; case TXN_SUCCESS: - /* If the database has already changed since we started the - * commit, re-evaluate it immediately to avoid missing a change - * for a while. */ + /* Possibly some work on the database was deferred because no + * further transaction could proceed. Wake up again. */ loop->cur_cfg = loop->next_cfg; - if (ovsdb_idl_get_seqno(loop->idl) != loop->precommit_seqno) { - poll_immediate_wake(); - } + poll_immediate_wake(); break; case TXN_UNCHANGED:
There is a fair amount of code that defers modifying the database when a transaction cannot be created (because there is already one outstanding). This code tends to assume that the main loop will wake up again when it becomes possible again to modify the database, but the actual ovsdb_id_loop implementation only did this if the database had changed. This is too conservative a policy and may account for some failures I've seen in tests. Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/ovsdb-idl.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)