| Message ID | 1466726451-6253-1-git-send-email-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
"dev" <dev-bounces@openvswitch.org> wrote on 06/23/2016 07:00:51 PM: > From: Ben Pfaff <blp@ovn.org> > To: dev@openvswitch.org > Cc: Ben Pfaff <blp@ovn.org> > Date: 06/23/2016 07:01 PM > Subject: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl helpers > to make logic more readable. > Sent by: "dev" <dev-bounces@openvswitch.org> > > Also there were lots of 'continue's sprinkled around that didn't seem to > be needed given some simple code rearrangement. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > ovn/controller/binding.c | 11 ++++------- > ovn/controller/encaps.c | 19 +++++-------------- > ovn/controller/lport.c | 18 ++++++------------ > ovsdb/ovsdb-idlc.in | 12 ++++++++++++ > 4 files changed, 27 insertions(+), 33 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index e36c8f4..9921a49 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -266,15 +266,12 @@ binding_run(struct controller_ctx *ctx, const > struct ovsrec_bridge *br_int, > process_full_binding = false; > } else { > SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) { > - bool is_deleted = sbrec_port_binding_row_get_seqno (binding_rec, > - OVSDB_IDL_CHANGE_DELETE) > 0; > - > - if (is_deleted) { > + if (sbrec_port_binding_is_deleted(binding_rec)) { > remove_local_datapath_by_binding(local_datapaths, > binding_rec); > - continue; > + } else { > + consider_local_datapath(ctx, &lports, chassis_rec, > binding_rec, > + local_datapaths); > } > - consider_local_datapath(ctx, &lports, chassis_rec, binding_rec, > - local_datapaths); > } > } > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c > index 495f8f6..18268a6 100644 > --- a/ovn/controller/encaps.c > +++ b/ovn/controller/encaps.c > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2015 Nicira, Inc. > +/* Copyright (c) 2015, 2016 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -404,12 +404,7 @@ encaps_run(struct controller_ctx *ctx, const > struct ovsrec_bridge *br_int, > process_full_encaps = false; > } else { > SBREC_CHASSIS_FOR_EACH_TRACKED (chassis_rec, ctx->ovnsb_idl) { > - bool is_deleted = sbrec_chassis_row_get_seqno(chassis_rec, > - > OVSDB_IDL_CHANGE_DELETE) > 0; > - bool is_new = sbrec_chassis_row_get_seqno(chassis_rec, > - > OVSDB_IDL_CHANGE_MODIFY) == 0; > - > - if (is_deleted) { > + if (sbrec_chassis_is_deleted(chassis_rec)) { > /* Lookup the tunnel by row uuid and remove it. */ > struct port_hash_node *port_hash = > port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid, > @@ -424,14 +419,10 @@ encaps_run(struct controller_ctx *ctx, const > struct ovsrec_bridge *br_int, > free(port_hash); > binding_reset_processing(); > } > - continue; > - } > - if (!is_new) { > - check_and_update_tunnel(chassis_rec); > - continue; > - } else { > + } else if (sbrec_chassis_is_new(chassis_rec)) { > check_and_add_tunnel(chassis_rec, chassis_id); > - continue; > + } else { > + check_and_update_tunnel(chassis_rec); > } > } > } > diff --git a/ovn/controller/lport.c b/ovn/controller/lport.c > index 2ce6387..a5e9ad3 100644 > --- a/ovn/controller/lport.c > +++ b/ovn/controller/lport.c > @@ -107,14 +107,11 @@ lport_index_fill(struct lport_index *lports, > struct ovsdb_idl *ovnsb_idl) > full_lport_rebuild = false; > } else { > SBREC_PORT_BINDING_FOR_EACH_TRACKED (pb, ovnsb_idl) { > - bool is_delete = sbrec_port_binding_row_get_seqno(pb, > - OVSDB_IDL_CHANGE_DELETE) > 0; > - > - if (is_delete) { > + if (sbrec_port_binding_is_deleted(pb)) { > lport_index_remove(lports, &pb->header_.uuid); > - continue; > + } else { > + consider_lport_index(lports, pb); > } > - consider_lport_index(lports, pb); > } > } > } > @@ -248,14 +245,11 @@ mcgroup_index_fill(struct mcgroup_index > *mcgroups, struct ovsdb_idl *ovnsb_idl) > full_mc_rebuild = false; > } else { > SBREC_MULTICAST_GROUP_FOR_EACH_TRACKED (mg, ovnsb_idl) { > - bool is_delete = sbrec_multicast_group_row_get_seqno(mg, > - OVSDB_IDL_CHANGE_DELETE) > 0; > - > - if (is_delete) { > + if (sbrec_multicast_group_is_deleted(mg)) { > mcgroup_index_remove(mcgroups, &mg->header_.uuid); > - continue; > + } else { > + consider_mcgroup_index(mcgroups, mg); > } > - consider_mcgroup_index(mcgroups, mg); > } > } > } > diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in > index 19a86dc..0d836c0 100755 > --- a/ovsdb/ovsdb-idlc.in > +++ b/ovsdb/ovsdb-idlc.in > @@ -186,6 +186,18 @@ const struct %(s)s *%(s)s_track_get_next(const > struct %(s)s *); > (ROW); \\ > (ROW) = %(s)s_track_get_next(ROW)) > > +/* Returns true if 'row' was inserted since the last change > tracking reset. */ > +static inline bool %(s)s_is_new(const struct %(s)s *row) > +{ > + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) == 0; > +} > + > +/* Returns true if 'row' was deleted since the last change trackingreset. */ > +static inline bool %(s)s_is_deleted(const struct %(s)s *row) > +{ > + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > 0; > +} > + > void %(s)s_init(struct %(s)s *); > void %(s)s_delete(const struct %(s)s *); > struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *); > -- > 2.1.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev I'll not only ack this, I'll pull it in and base my residual incremental processing patches on it because it makes it *MUCH* cleaner: Acked-by: Ryan Moats <rmoats@us.ibm.com> In fact .... Happily-acked-by :)
On Thu, Jun 23, 2016 at 08:01:52PM -0500, Ryan Moats wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 06/23/2016 07:00:51 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: dev@openvswitch.org > > Cc: Ben Pfaff <blp@ovn.org> > > Date: 06/23/2016 07:01 PM > > Subject: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl helpers > > to make logic more readable. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > Also there were lots of 'continue's sprinkled around that didn't seem to > > be needed given some simple code rearrangement. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > I'll not only ack this, I'll pull it in and base my residual incremental > processing patches on it because it makes it *MUCH* cleaner: > > Acked-by: Ryan Moats <rmoats@us.ibm.com> > > In fact .... Happily-acked-by :) Such enthusiasm! And speed! Thanks, I applied this to master.
Ben Pfaff <blp@ovn.org> wrote on 06/23/2016 10:33:53 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 06/23/2016 10:34 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl > helpers to make logic more readable. > > On Thu, Jun 23, 2016 at 08:01:52PM -0500, Ryan Moats wrote: > > "dev" <dev-bounces@openvswitch.org> wrote on 06/23/2016 07:00:51 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: dev@openvswitch.org > > > Cc: Ben Pfaff <blp@ovn.org> > > > Date: 06/23/2016 07:01 PM > > > Subject: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl helpers > > > to make logic more readable. > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > Also there were lots of 'continue's sprinkled around that didn't seem to > > > be needed given some simple code rearrangement. > > > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > > I'll not only ack this, I'll pull it in and base my residual incremental > > processing patches on it because it makes it *MUCH* cleaner: > > > > Acked-by: Ryan Moats <rmoats@us.ibm.com> > > > > In fact .... Happily-acked-by :) > > Such enthusiasm! And speed! > > Thanks, I applied this to master. > Are you kidding, I was thinking "man, I should have thought of that!!!" Ryan
On Thu, Jun 23, 2016 at 10:48:31PM -0500, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 06/23/2016 10:33:53 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: dev@openvswitch.org > > Date: 06/23/2016 10:34 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl > > helpers to make logic more readable. > > > > On Thu, Jun 23, 2016 at 08:01:52PM -0500, Ryan Moats wrote: > > > "dev" <dev-bounces@openvswitch.org> wrote on 06/23/2016 07:00:51 PM: > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > To: dev@openvswitch.org > > > > Cc: Ben Pfaff <blp@ovn.org> > > > > Date: 06/23/2016 07:01 PM > > > > Subject: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl helpers > > > > to make logic more readable. > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > Also there were lots of 'continue's sprinkled around that didn't seem > to > > > > be needed given some simple code rearrangement. > > > > > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > > > > I'll not only ack this, I'll pull it in and base my residual > incremental > > > processing patches on it because it makes it *MUCH* cleaner: > > > > > > Acked-by: Ryan Moats <rmoats@us.ibm.com> > > > > > > In fact .... Happily-acked-by :) > > > > Such enthusiasm! And speed! > > > > Thanks, I applied this to master. > > > > Are you kidding, I was thinking "man, I should have thought of that!!!" I swear I suggested it once. At this point I figured an example would make it more obvious ;-)
Ben Pfaff <blp@ovn.org> wrote on 06/23/2016 10:58:03 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 06/23/2016 10:58 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl > helpers to make logic more readable. > > On Thu, Jun 23, 2016 at 10:48:31PM -0500, Ryan Moats wrote: > > Ben Pfaff <blp@ovn.org> wrote on 06/23/2016 10:33:53 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: dev@openvswitch.org > > > Date: 06/23/2016 10:34 PM > > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl > > > helpers to make logic more readable. > > > > > > On Thu, Jun 23, 2016 at 08:01:52PM -0500, Ryan Moats wrote: > > > > "dev" <dev-bounces@openvswitch.org> wrote on 06/23/2016 07:00:51 PM: > > > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > > To: dev@openvswitch.org > > > > > Cc: Ben Pfaff <blp@ovn.org> > > > > > Date: 06/23/2016 07:01 PM > > > > > Subject: [ovs-dev] [PATCH] ovn-controller: Use new ovsdb-idl helpers > > > > > to make logic more readable. > > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > > > Also there were lots of 'continue's sprinkled around that didn't seem > > to > > > > > be needed given some simple code rearrangement. > > > > > > > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > > > > > > I'll not only ack this, I'll pull it in and base my residual > > incremental > > > > processing patches on it because it makes it *MUCH* cleaner: > > > > > > > > Acked-by: Ryan Moats <rmoats@us.ibm.com> > > > > > > > > In fact .... Happily-acked-by :) > > > > > > Such enthusiasm! And speed! > > > > > > Thanks, I applied this to master. > > > > > > > Are you kidding, I was thinking "man, I should have thought of that!!!" > > I swear I suggested it once. At this point I figured an example would > make it more obvious ;-) > You probably did and I misunderstood you... Incoming v19 before I call it a night - I have all the changes/rebases ready to roll... Ryan
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index e36c8f4..9921a49 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -266,15 +266,12 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, process_full_binding = false; } else { SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) { - bool is_deleted = sbrec_port_binding_row_get_seqno(binding_rec, - OVSDB_IDL_CHANGE_DELETE) > 0; - - if (is_deleted) { + if (sbrec_port_binding_is_deleted(binding_rec)) { remove_local_datapath_by_binding(local_datapaths, binding_rec); - continue; + } else { + consider_local_datapath(ctx, &lports, chassis_rec, binding_rec, + local_datapaths); } - consider_local_datapath(ctx, &lports, chassis_rec, binding_rec, - local_datapaths); } } diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c index 495f8f6..18268a6 100644 --- a/ovn/controller/encaps.c +++ b/ovn/controller/encaps.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2015 Nicira, Inc. +/* Copyright (c) 2015, 2016 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -404,12 +404,7 @@ encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, process_full_encaps = false; } else { SBREC_CHASSIS_FOR_EACH_TRACKED (chassis_rec, ctx->ovnsb_idl) { - bool is_deleted = sbrec_chassis_row_get_seqno(chassis_rec, - OVSDB_IDL_CHANGE_DELETE) > 0; - bool is_new = sbrec_chassis_row_get_seqno(chassis_rec, - OVSDB_IDL_CHANGE_MODIFY) == 0; - - if (is_deleted) { + if (sbrec_chassis_is_deleted(chassis_rec)) { /* Lookup the tunnel by row uuid and remove it. */ struct port_hash_node *port_hash = port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid, @@ -424,14 +419,10 @@ encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, free(port_hash); binding_reset_processing(); } - continue; - } - if (!is_new) { - check_and_update_tunnel(chassis_rec); - continue; - } else { + } else if (sbrec_chassis_is_new(chassis_rec)) { check_and_add_tunnel(chassis_rec, chassis_id); - continue; + } else { + check_and_update_tunnel(chassis_rec); } } } diff --git a/ovn/controller/lport.c b/ovn/controller/lport.c index 2ce6387..a5e9ad3 100644 --- a/ovn/controller/lport.c +++ b/ovn/controller/lport.c @@ -107,14 +107,11 @@ lport_index_fill(struct lport_index *lports, struct ovsdb_idl *ovnsb_idl) full_lport_rebuild = false; } else { SBREC_PORT_BINDING_FOR_EACH_TRACKED (pb, ovnsb_idl) { - bool is_delete = sbrec_port_binding_row_get_seqno(pb, - OVSDB_IDL_CHANGE_DELETE) > 0; - - if (is_delete) { + if (sbrec_port_binding_is_deleted(pb)) { lport_index_remove(lports, &pb->header_.uuid); - continue; + } else { + consider_lport_index(lports, pb); } - consider_lport_index(lports, pb); } } } @@ -248,14 +245,11 @@ mcgroup_index_fill(struct mcgroup_index *mcgroups, struct ovsdb_idl *ovnsb_idl) full_mc_rebuild = false; } else { SBREC_MULTICAST_GROUP_FOR_EACH_TRACKED (mg, ovnsb_idl) { - bool is_delete = sbrec_multicast_group_row_get_seqno(mg, - OVSDB_IDL_CHANGE_DELETE) > 0; - - if (is_delete) { + if (sbrec_multicast_group_is_deleted(mg)) { mcgroup_index_remove(mcgroups, &mg->header_.uuid); - continue; + } else { + consider_mcgroup_index(mcgroups, mg); } - consider_mcgroup_index(mcgroups, mg); } } } diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in index 19a86dc..0d836c0 100755 --- a/ovsdb/ovsdb-idlc.in +++ b/ovsdb/ovsdb-idlc.in @@ -186,6 +186,18 @@ const struct %(s)s *%(s)s_track_get_next(const struct %(s)s *); (ROW); \\ (ROW) = %(s)s_track_get_next(ROW)) +/* Returns true if 'row' was inserted since the last change tracking reset. */ +static inline bool %(s)s_is_new(const struct %(s)s *row) +{ + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) == 0; +} + +/* Returns true if 'row' was deleted since the last change tracking reset. */ +static inline bool %(s)s_is_deleted(const struct %(s)s *row) +{ + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > 0; +} + void %(s)s_init(struct %(s)s *); void %(s)s_delete(const struct %(s)s *); struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *);
Also there were lots of 'continue's sprinkled around that didn't seem to be needed given some simple code rearrangement. Signed-off-by: Ben Pfaff <blp@ovn.org> --- ovn/controller/binding.c | 11 ++++------- ovn/controller/encaps.c | 19 +++++-------------- ovn/controller/lport.c | 18 ++++++------------ ovsdb/ovsdb-idlc.in | 12 ++++++++++++ 4 files changed, 27 insertions(+), 33 deletions(-)