| Message ID | 20200923180017.22380-2-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3,1/6] ovn-northd: reorganize processing of lflows | expand |
On 9/23/20 8:00 PM, anton.ivanov@cambridgegreys.com wrote: > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > 1. Merge lrouter and lswitch processing. > 2. Move lrouter and lswitch lflow generation which uses the > same iterator variables into common helpers > 3. Set up structures to be used in parallel and sequential mode This patch-set doesn't introduce any parallel procesing, so please, do not mention it or make it clear that it's not yet implemented, so the person who will look throught the commit history will not be confused. > > Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> > --- Some style commnets inline. All of them are applicable, actually, to all other patches of the set. So, I'm only reviewing this one. Best regards, Ilya Maximents. > northd/ovn-northd.c | 199 +++++++++++++++++++++++++++----------------- > 1 file changed, 123 insertions(+), 76 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 3324c9e81..5faa6cee6 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -8874,24 +8874,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > struct ds actions = DS_EMPTY_INITIALIZER; > > struct ovn_datapath *od; > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_adm_ctrl_flows_for_lrouter(od, lflows); > - } > - > struct ovn_port *op; > - HMAP_FOR_EACH (op, key_node, ports) { > - build_adm_ctrl_flows_for_lrouter_port(op, lflows, &match, &actions); > - } > - > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_neigh_learning_flows_for_lrouter( > - od, lflows, &match, &actions); > - } > - > - HMAP_FOR_EACH (op, key_node, ports) { > - build_neigh_learning_flows_for_lrouter_port( > - op, lflows, &match, &actions); > - } > > /* Drop IP traffic destined to router owned IPs. Part of it is dropped > * in stage "lr_in_ip_input" but traffic that could have been unSNATed > @@ -9935,63 +9918,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > sset_destroy(&nat_entries); > } > > - HMAP_FOR_EACH (op, key_node, ports) { > - build_ND_RA_flows_for_lrouter_port(op, lflows, &match, &actions); > - } > - > - /* Logical router ingress table ND_RA_OPTIONS & ND_RA_RESPONSE: RS > - * responder, by default goto next. (priority 0). */ You lost this comment. > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_ND_RA_flows_for_lrouter(od, lflows); > - } > - > - HMAP_FOR_EACH (op, key_node, ports) { > - build_ip_routing_flows_for_lrouter_port(op, lflows); > - } > - > - /* Convert the static routes to flows. */ > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_static_route_flows_for_lrouter(od, lflows, ports); > - } > - > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_mcast_lookup_flows_for_lrouter(od, lflows, &match, &actions); > - } > - > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_ingress_policy_flows_for_lrouter(od, lflows, ports); > - } > - > - /* XXX destination unreachable */ And this comment also disappeared. > - > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_arp_resolve_flows_for_lrouter(od, lflows); > - } > - > - HMAP_FOR_EACH (op, key_node, ports) { > - build_arp_resolve_flows_for_lrouter_port( > - op, lflows, ports, &match, &actions); > - } > - > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_check_pkt_len_flows_for_lrouter( > - od, lflows, ports, &match, &actions); > - } > - > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_gateway_redirect_flows_for_lrouter( > - od, lflows, &match, &actions); > - } > - > - HMAP_FOR_EACH (od, key_node, datapaths) { > - build_arp_request_flows_for_lrouter(od, lflows, &match, &actions); > - } > - > - HMAP_FOR_EACH (op, key_node, ports) { > - build_egress_delivery_flows_for_lrouter_port( > - op, lflows, &match, &actions); > - } > - > ds_destroy(&match); > ds_destroy(&actions); > } > @@ -11389,6 +11315,128 @@ build_ipv6_input_flows_for_lrouter_port( > > } > > +struct lswitch_flow_build_info { > + struct hmap *datapaths; > + struct hmap *ports; > + struct hmap *port_groups; > + struct hmap *lflows; > + struct hmap *mcgroups; > + struct hmap *igmp_groups; > + struct shash *meter_groups; > + struct hmap *lbs; > + char *svc_check_match; > + struct ds match; > + struct ds actions; > +}; > + > +/* Helper function to combine all lflow generation which is iterated by > + * datapath. It can be invoked with a lsi argument containing "all work" > + * in single threaded mode or an lsi argument containing a "chunk of work" > + * in parallel. Please, don't mention parallel execution anywhere in the code until we actually have it. This is really confusing for someone who reads that. > + */ > + Please, do not put an empty line between a comment and a function in belongs. > +static void > + build_converged_iterate_by_od( And, please, do not ever format lines like this. There is a purpose why function names are on their own line and starts right from the start of the line. And the reason is that you could grep for function definition, i.e. git grep '^build_lrouter_flows' or to quickly find definition within a text editor. > + struct ovn_datapath *od, struct lswitch_flow_build_info *lsi) One of these arguments could be easily placed on the same line with the function name. So, please, place it there. It's easier to read and saves a line of code. > +{ > + > + /* Build Logical Router Flows */ Please, add period to the end of the comment. Same for all other comments in all other patches. > + build_adm_ctrl_flows_for_lrouter(od, lsi->lflows); > + build_neigh_learning_flows_for_lrouter( > + od, lsi->lflows, &lsi->match, &lsi->actions); > + build_ND_RA_flows_for_lrouter(od, lsi->lflows); > + build_static_route_flows_for_lrouter(od, lsi->lflows, lsi->ports); > + build_mcast_lookup_flows_for_lrouter( > + od, lsi->lflows, &lsi->match, &lsi->actions); > + build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->ports); > + build_arp_resolve_flows_for_lrouter(od, lsi->lflows); > + build_check_pkt_len_flows_for_lrouter( > + od, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions); > + build_gateway_redirect_flows_for_lrouter( > + od, lsi->lflows, &lsi->match, &lsi->actions); > + build_arp_request_flows_for_lrouter( > + od, lsi->lflows, &lsi->match, &lsi->actions); > +} > + > +/* Helper function to combine all lflow generation which is iterated by port. > + * It can be invoked with a lsi argument containing "all work" in single > + * threaded mode or an lsi argument containing a "chunk of work" in parallel. Ditto. > + */ > + Ditto. > +static void > + build_converged_iterate_by_op( Ditto. > + struct ovn_port *op, > + struct lswitch_flow_build_info *lsi) Ditto. > +{ > + /* Build Logical Router Flows */ Ditto. > + > + build_adm_ctrl_flows_for_lrouter_port( > + op, lsi->lflows, &lsi->match, &lsi->actions); > + build_neigh_learning_flows_for_lrouter_port( > + op, lsi->lflows, &lsi->match, &lsi->actions); > + build_ip_routing_flows_for_lrouter_port(op, lsi->lflows); > + build_ND_RA_flows_for_lrouter_port( > + op, lsi->lflows, &lsi->match, &lsi->actions); > + build_arp_resolve_flows_for_lrouter_port( > + op, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions); > + build_egress_delivery_flows_for_lrouter_port( > + op, lsi->lflows, &lsi->match, &lsi->actions); > +} > + > +/* > + * Combined LFLOW processing function. Intended to iterate over Why LFLOW capitalized? > + * datapaths, ports, lbs and igmp_groups in single threaded mode > + * or prepare and invoke a thread pool in multi-threaded mode. Ditto. > + * Must not contain any direct flow ops - all actual flow ops > + * should be invoked out the per-iterable helper functions. > + */ > + > +static void > +build_converged_flows(struct hmap *datapaths, struct hmap *ports, This function name looks really strange and also names of both other fucntions in this patch. I'd say we need a better name. Current one makes an impression that we have some kind of entity named 'converged flow'. > + struct hmap *port_groups, struct hmap *lflows, > + struct hmap *mcgroups, struct hmap *igmp_groups, > + struct shash *meter_groups, > + struct hmap *lbs) Please, fix up intents. Above lines should be shifted 2 spaces to the right. > +{ > + struct lswitch_flow_build_info lsi; > + Please, don't add extra empty lines between definitions. > + struct ovn_datapath *od; > + struct ovn_port *op; > + > + char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac); > + > + lsi.datapaths = datapaths; > + lsi.ports = ports; > + lsi.port_groups = port_groups; > + lsi.lflows = lflows; > + lsi.mcgroups = mcgroups; > + lsi.igmp_groups = igmp_groups; > + lsi.meter_groups = meter_groups; > + lsi.lbs = lbs; > + lsi.svc_check_match = svc_check_match; > + lsi.match = (struct ds) DS_EMPTY_INITIALIZER; > + lsi.actions = (struct ds) DS_EMPTY_INITIALIZER; Why cast? Can all of this be just an initializer? i.e. struct lswitch_flow_build_info lsi = { .datapaths = datapaths, ... }; > + > + /* Converged build - all lflow generation from lswitch and lrouter > + * will move here and will be reogranized by iterator type. > + * This allows it to be run (optionally) in parallel */ 'parallel', period. > + HMAP_FOR_EACH (od, key_node, datapaths) { > + build_converged_iterate_by_od(od, &lsi); > + } > + HMAP_FOR_EACH (op, key_node, ports) { > + build_converged_iterate_by_op(op, &lsi); > + } > + free(svc_check_match); > + > + /* Legacy lswitch build */ period. > + build_lswitch_flows(datapaths, ports, port_groups, lflows, mcgroups, > + igmp_groups, meter_groups, lbs); > + > + /* Legacy lrouter build */ period. > + build_lrouter_flows(datapaths, ports, lflows, meter_groups, lbs); > +} > + > + > /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database, > * constructing their contents based on the OVN_NB database. */ > static void > @@ -11400,9 +11448,8 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, > { > struct hmap lflows = HMAP_INITIALIZER(&lflows); > > - build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups, > + build_converged_flows(datapaths, ports, port_groups, &lflows, mcgroups, > igmp_groups, meter_groups, lbs); Please, fix up indentation on this line. > - build_lrouter_flows(datapaths, ports, &lflows, meter_groups, lbs); > > /* Push changes to the Logical_Flow table to database. */ > const struct sbrec_logical_flow *sbflow, *next_sbflow; >
On 23/09/2020 21:50, Ilya Maximets wrote: > On 9/23/20 8:00 PM, anton.ivanov@cambridgegreys.com wrote: >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> 1. Merge lrouter and lswitch processing. >> 2. Move lrouter and lswitch lflow generation which uses the >> same iterator variables into common helpers >> 3. Set up structures to be used in parallel and sequential mode > > This patch-set doesn't introduce any parallel procesing, so please, > do not mention it or make it clear that it's not yet implemented, so > the person who will look throught the commit history will not be > confused. > >> >> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> --- > > Some style commnets inline. > All of them are applicable, actually, to all other patches of the set. > So, I'm only reviewing this one. > > Best regards, Ilya Maximents. > >> northd/ovn-northd.c | 199 +++++++++++++++++++++++++++----------------- >> 1 file changed, 123 insertions(+), 76 deletions(-) >> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index 3324c9e81..5faa6cee6 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -8874,24 +8874,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, >> struct ds actions = DS_EMPTY_INITIALIZER; >> >> struct ovn_datapath *od; >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_adm_ctrl_flows_for_lrouter(od, lflows); >> - } >> - >> struct ovn_port *op; >> - HMAP_FOR_EACH (op, key_node, ports) { >> - build_adm_ctrl_flows_for_lrouter_port(op, lflows, &match, &actions); >> - } >> - >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_neigh_learning_flows_for_lrouter( >> - od, lflows, &match, &actions); >> - } >> - >> - HMAP_FOR_EACH (op, key_node, ports) { >> - build_neigh_learning_flows_for_lrouter_port( >> - op, lflows, &match, &actions); >> - } >> >> /* Drop IP traffic destined to router owned IPs. Part of it is dropped >> * in stage "lr_in_ip_input" but traffic that could have been unSNATed >> @@ -9935,63 +9918,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, >> sset_destroy(&nat_entries); >> } >> >> - HMAP_FOR_EACH (op, key_node, ports) { >> - build_ND_RA_flows_for_lrouter_port(op, lflows, &match, &actions); >> - } >> - >> - /* Logical router ingress table ND_RA_OPTIONS & ND_RA_RESPONSE: RS >> - * responder, by default goto next. (priority 0). */ > > You lost this comment. All comments documenting functions are at the funciton declarations - this was a leftover duplicate. This comment is on line 8776 in the patched version now to fit with all others. > >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_ND_RA_flows_for_lrouter(od, lflows); >> - } >> - >> - HMAP_FOR_EACH (op, key_node, ports) { >> - build_ip_routing_flows_for_lrouter_port(op, lflows); >> - } >> - >> - /* Convert the static routes to flows. */ >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_static_route_flows_for_lrouter(od, lflows, ports); >> - } >> - >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_mcast_lookup_flows_for_lrouter(od, lflows, &match, &actions); >> - } >> - >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_ingress_policy_flows_for_lrouter(od, lflows, ports); >> - } >> - >> - /* XXX destination unreachable */ > > And this comment also disappeared. This one was unclear - it was stand alone and not relating to the actual code which followed and had its own comments. I can restore it following the same convention as the previous one. > >> - >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_arp_resolve_flows_for_lrouter(od, lflows); >> - } >> - >> - HMAP_FOR_EACH (op, key_node, ports) { >> - build_arp_resolve_flows_for_lrouter_port( >> - op, lflows, ports, &match, &actions); >> - } >> - >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_check_pkt_len_flows_for_lrouter( >> - od, lflows, ports, &match, &actions); >> - } >> - >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_gateway_redirect_flows_for_lrouter( >> - od, lflows, &match, &actions); >> - } >> - >> - HMAP_FOR_EACH (od, key_node, datapaths) { >> - build_arp_request_flows_for_lrouter(od, lflows, &match, &actions); >> - } >> - >> - HMAP_FOR_EACH (op, key_node, ports) { >> - build_egress_delivery_flows_for_lrouter_port( >> - op, lflows, &match, &actions); >> - } >> - >> ds_destroy(&match); >> ds_destroy(&actions); >> } >> @@ -11389,6 +11315,128 @@ build_ipv6_input_flows_for_lrouter_port( >> >> } >> >> +struct lswitch_flow_build_info { >> + struct hmap *datapaths; >> + struct hmap *ports; >> + struct hmap *port_groups; >> + struct hmap *lflows; >> + struct hmap *mcgroups; >> + struct hmap *igmp_groups; >> + struct shash *meter_groups; >> + struct hmap *lbs; >> + char *svc_check_match; >> + struct ds match; >> + struct ds actions; >> +}; >> + >> +/* Helper function to combine all lflow generation which is iterated by >> + * datapath. It can be invoked with a lsi argument containing "all work" >> + * in single threaded mode or an lsi argument containing a "chunk of work" >> + * in parallel. > > Please, don't mention parallel execution anywhere in the code until we actually > have it. This is really confusing for someone who reads that. > >> + */ >> + > > Please, do not put an empty line between a comment and a function in belongs. > >> +static void >> + build_converged_iterate_by_od( > > And, please, do not ever format lines like this. There is a purpose why > function names are on their own line and starts right from the start of > the line. And the reason is that you could grep for function definition, > i.e. git grep '^build_lrouter_flows' or to quickly find definition within > a text editor. > >> + struct ovn_datapath *od, struct lswitch_flow_build_info *lsi) > > One of these arguments could be easily placed on the same line with the > function name. So, please, place it there. It's easier to read and > saves a line of code. > >> +{ >> + >> + /* Build Logical Router Flows */ > > Please, add period to the end of the comment. Same for all other comments > in all other patches. > >> + build_adm_ctrl_flows_for_lrouter(od, lsi->lflows); >> + build_neigh_learning_flows_for_lrouter( >> + od, lsi->lflows, &lsi->match, &lsi->actions); >> + build_ND_RA_flows_for_lrouter(od, lsi->lflows); >> + build_static_route_flows_for_lrouter(od, lsi->lflows, lsi->ports); >> + build_mcast_lookup_flows_for_lrouter( >> + od, lsi->lflows, &lsi->match, &lsi->actions); >> + build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->ports); >> + build_arp_resolve_flows_for_lrouter(od, lsi->lflows); >> + build_check_pkt_len_flows_for_lrouter( >> + od, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions); >> + build_gateway_redirect_flows_for_lrouter( >> + od, lsi->lflows, &lsi->match, &lsi->actions); >> + build_arp_request_flows_for_lrouter( >> + od, lsi->lflows, &lsi->match, &lsi->actions); >> +} >> + >> +/* Helper function to combine all lflow generation which is iterated by port. >> + * It can be invoked with a lsi argument containing "all work" in single >> + * threaded mode or an lsi argument containing a "chunk of work" in parallel. > > Ditto. > >> + */ >> + > > Ditto. > >> +static void >> + build_converged_iterate_by_op( > > Ditto. > >> + struct ovn_port *op, >> + struct lswitch_flow_build_info *lsi) > > Ditto. > >> +{ >> + /* Build Logical Router Flows */ > > Ditto. > >> + >> + build_adm_ctrl_flows_for_lrouter_port( >> + op, lsi->lflows, &lsi->match, &lsi->actions); >> + build_neigh_learning_flows_for_lrouter_port( >> + op, lsi->lflows, &lsi->match, &lsi->actions); >> + build_ip_routing_flows_for_lrouter_port(op, lsi->lflows); >> + build_ND_RA_flows_for_lrouter_port( >> + op, lsi->lflows, &lsi->match, &lsi->actions); >> + build_arp_resolve_flows_for_lrouter_port( >> + op, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions); >> + build_egress_delivery_flows_for_lrouter_port( >> + op, lsi->lflows, &lsi->match, &lsi->actions); >> +} >> + >> +/* >> + * Combined LFLOW processing function. Intended to iterate over > > Why LFLOW capitalized? > >> + * datapaths, ports, lbs and igmp_groups in single threaded mode >> + * or prepare and invoke a thread pool in multi-threaded mode. > > Ditto. > >> + * Must not contain any direct flow ops - all actual flow ops >> + * should be invoked out the per-iterable helper functions. >> + */ >> + >> +static void >> +build_converged_flows(struct hmap *datapaths, struct hmap *ports, > > This function name looks really strange and also names of both other fucntions > in this patch. I'd say we need a better name. Current one makes an impression > that we have some kind of entity named 'converged flow'. > >> + struct hmap *port_groups, struct hmap *lflows, >> + struct hmap *mcgroups, struct hmap *igmp_groups, >> + struct shash *meter_groups, >> + struct hmap *lbs) > > Please, fix up intents. Above lines should be shifted 2 spaces to the right. > >> +{ >> + struct lswitch_flow_build_info lsi; >> + > > Please, don't add extra empty lines between definitions. > >> + struct ovn_datapath *od; >> + struct ovn_port *op; >> + >> + char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac); >> + >> + lsi.datapaths = datapaths; >> + lsi.ports = ports; >> + lsi.port_groups = port_groups; >> + lsi.lflows = lflows; >> + lsi.mcgroups = mcgroups; >> + lsi.igmp_groups = igmp_groups; >> + lsi.meter_groups = meter_groups; >> + lsi.lbs = lbs; >> + lsi.svc_check_match = svc_check_match; >> + lsi.match = (struct ds) DS_EMPTY_INITIALIZER; >> + lsi.actions = (struct ds) DS_EMPTY_INITIALIZER; > > Why cast? GCC being stupid - barfs without. Why? No idea lsi.actions = DS_EMPTY_INITIALIZER; is syntactically correct, but it does not want to consume it. > > Can all of this be just an initializer? i.e. > > struct lswitch_flow_build_info lsi = { > .datapaths = datapaths, > ... > }; > >> + >> + /* Converged build - all lflow generation from lswitch and lrouter >> + * will move here and will be reogranized by iterator type. >> + * This allows it to be run (optionally) in parallel */ > > 'parallel', period. > >> + HMAP_FOR_EACH (od, key_node, datapaths) { >> + build_converged_iterate_by_od(od, &lsi); >> + } >> + HMAP_FOR_EACH (op, key_node, ports) { >> + build_converged_iterate_by_op(op, &lsi); >> + } >> + free(svc_check_match); >> + >> + /* Legacy lswitch build */ > > period. > >> + build_lswitch_flows(datapaths, ports, port_groups, lflows, mcgroups, >> + igmp_groups, meter_groups, lbs); >> + >> + /* Legacy lrouter build */ > > period. > >> + build_lrouter_flows(datapaths, ports, lflows, meter_groups, lbs); >> +} >> + >> + >> /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database, >> * constructing their contents based on the OVN_NB database. */ >> static void >> @@ -11400,9 +11448,8 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, >> { >> struct hmap lflows = HMAP_INITIALIZER(&lflows); >> >> - build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups, >> + build_converged_flows(datapaths, ports, port_groups, &lflows, mcgroups, >> igmp_groups, meter_groups, lbs); > > Please, fix up indentation on this line. > >> - build_lrouter_flows(datapaths, ports, &lflows, meter_groups, lbs); >> >> /* Push changes to the Logical_Flow table to database. */ >> const struct sbrec_logical_flow *sbflow, *next_sbflow; >> > >
On 9/24/20 11:27 AM, Anton Ivanov wrote: > > > On 23/09/2020 21:50, Ilya Maximets wrote: >> On 9/23/20 8:00 PM, anton.ivanov@cambridgegreys.com wrote: >>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>> >>> 1. Merge lrouter and lswitch processing. >>> 2. Move lrouter and lswitch lflow generation which uses the >>> same iterator variables into common helpers >>> 3. Set up structures to be used in parallel and sequential mode >> >> This patch-set doesn't introduce any parallel procesing, so please, >> do not mention it or make it clear that it's not yet implemented, so >> the person who will look throught the commit history will not be >> confused. >> >>> >>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>> --- >> >> Some style commnets inline. >> All of them are applicable, actually, to all other patches of the set. >> So, I'm only reviewing this one. >> >> Best regards, Ilya Maximents. >> >>> northd/ovn-northd.c | 199 +++++++++++++++++++++++++++----------------- >>> 1 file changed, 123 insertions(+), 76 deletions(-) >>> >>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >>> index 3324c9e81..5faa6cee6 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -8874,24 +8874,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, >>> struct ds actions = DS_EMPTY_INITIALIZER; >>> struct ovn_datapath *od; >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_adm_ctrl_flows_for_lrouter(od, lflows); >>> - } >>> - >>> struct ovn_port *op; >>> - HMAP_FOR_EACH (op, key_node, ports) { >>> - build_adm_ctrl_flows_for_lrouter_port(op, lflows, &match, &actions); >>> - } >>> - >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_neigh_learning_flows_for_lrouter( >>> - od, lflows, &match, &actions); >>> - } >>> - >>> - HMAP_FOR_EACH (op, key_node, ports) { >>> - build_neigh_learning_flows_for_lrouter_port( >>> - op, lflows, &match, &actions); >>> - } >>> /* Drop IP traffic destined to router owned IPs. Part of it is dropped >>> * in stage "lr_in_ip_input" but traffic that could have been unSNATed >>> @@ -9935,63 +9918,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, >>> sset_destroy(&nat_entries); >>> } >>> - HMAP_FOR_EACH (op, key_node, ports) { >>> - build_ND_RA_flows_for_lrouter_port(op, lflows, &match, &actions); >>> - } >>> - >>> - /* Logical router ingress table ND_RA_OPTIONS & ND_RA_RESPONSE: RS >>> - * responder, by default goto next. (priority 0). */ >> >> You lost this comment. > > All comments documenting functions are at the funciton declarations - this > was a leftover duplicate. > > This comment is on line 8776 in the patched version now to fit with all others. > >> >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_ND_RA_flows_for_lrouter(od, lflows); >>> - } >>> - >>> - HMAP_FOR_EACH (op, key_node, ports) { >>> - build_ip_routing_flows_for_lrouter_port(op, lflows); >>> - } >>> - >>> - /* Convert the static routes to flows. */ >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_static_route_flows_for_lrouter(od, lflows, ports); >>> - } >>> - >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_mcast_lookup_flows_for_lrouter(od, lflows, &match, &actions); >>> - } >>> - >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_ingress_policy_flows_for_lrouter(od, lflows, ports); >>> - } >>> - >>> - /* XXX destination unreachable */ >> >> And this comment also disappeared. > > This one was unclear - it was stand alone and not relating to the actual code which followed and had its own comments. > > I can restore it following the same convention as the previous one. > >> >>> - >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_arp_resolve_flows_for_lrouter(od, lflows); >>> - } >>> - >>> - HMAP_FOR_EACH (op, key_node, ports) { >>> - build_arp_resolve_flows_for_lrouter_port( >>> - op, lflows, ports, &match, &actions); >>> - } >>> - >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_check_pkt_len_flows_for_lrouter( >>> - od, lflows, ports, &match, &actions); >>> - } >>> - >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_gateway_redirect_flows_for_lrouter( >>> - od, lflows, &match, &actions); >>> - } >>> - >>> - HMAP_FOR_EACH (od, key_node, datapaths) { >>> - build_arp_request_flows_for_lrouter(od, lflows, &match, &actions); >>> - } >>> - >>> - HMAP_FOR_EACH (op, key_node, ports) { >>> - build_egress_delivery_flows_for_lrouter_port( >>> - op, lflows, &match, &actions); >>> - } >>> - >>> ds_destroy(&match); >>> ds_destroy(&actions); >>> } >>> @@ -11389,6 +11315,128 @@ build_ipv6_input_flows_for_lrouter_port( >>> } >>> +struct lswitch_flow_build_info { >>> + struct hmap *datapaths; >>> + struct hmap *ports; >>> + struct hmap *port_groups; >>> + struct hmap *lflows; >>> + struct hmap *mcgroups; >>> + struct hmap *igmp_groups; >>> + struct shash *meter_groups; >>> + struct hmap *lbs; >>> + char *svc_check_match; >>> + struct ds match; >>> + struct ds actions; >>> +}; >>> + >>> +/* Helper function to combine all lflow generation which is iterated by >>> + * datapath. It can be invoked with a lsi argument containing "all work" >>> + * in single threaded mode or an lsi argument containing a "chunk of work" >>> + * in parallel. >> >> Please, don't mention parallel execution anywhere in the code until we actually >> have it. This is really confusing for someone who reads that. >> >>> + */ >>> + >> >> Please, do not put an empty line between a comment and a function in belongs. >> >>> +static void >>> + build_converged_iterate_by_od( >> >> And, please, do not ever format lines like this. There is a purpose why >> function names are on their own line and starts right from the start of >> the line. And the reason is that you could grep for function definition, >> i.e. git grep '^build_lrouter_flows' or to quickly find definition within >> a text editor. >> >>> + struct ovn_datapath *od, struct lswitch_flow_build_info *lsi) >> >> One of these arguments could be easily placed on the same line with the >> function name. So, please, place it there. It's easier to read and >> saves a line of code. >> >>> +{ >>> + >>> + /* Build Logical Router Flows */ >> >> Please, add period to the end of the comment. Same for all other comments >> in all other patches. >> >>> + build_adm_ctrl_flows_for_lrouter(od, lsi->lflows); >>> + build_neigh_learning_flows_for_lrouter( >>> + od, lsi->lflows, &lsi->match, &lsi->actions); >>> + build_ND_RA_flows_for_lrouter(od, lsi->lflows); >>> + build_static_route_flows_for_lrouter(od, lsi->lflows, lsi->ports); >>> + build_mcast_lookup_flows_for_lrouter( >>> + od, lsi->lflows, &lsi->match, &lsi->actions); >>> + build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->ports); >>> + build_arp_resolve_flows_for_lrouter(od, lsi->lflows); >>> + build_check_pkt_len_flows_for_lrouter( >>> + od, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions); >>> + build_gateway_redirect_flows_for_lrouter( >>> + od, lsi->lflows, &lsi->match, &lsi->actions); >>> + build_arp_request_flows_for_lrouter( >>> + od, lsi->lflows, &lsi->match, &lsi->actions); >>> +} >>> + >>> +/* Helper function to combine all lflow generation which is iterated by port. >>> + * It can be invoked with a lsi argument containing "all work" in single >>> + * threaded mode or an lsi argument containing a "chunk of work" in parallel. >> >> Ditto. >> >>> + */ >>> + >> >> Ditto. >> >>> +static void >>> + build_converged_iterate_by_op( >> >> Ditto. >> >>> + struct ovn_port *op, >>> + struct lswitch_flow_build_info *lsi) >> >> Ditto. >> >>> +{ >>> + /* Build Logical Router Flows */ >> >> Ditto. >> >>> + >>> + build_adm_ctrl_flows_for_lrouter_port( >>> + op, lsi->lflows, &lsi->match, &lsi->actions); >>> + build_neigh_learning_flows_for_lrouter_port( >>> + op, lsi->lflows, &lsi->match, &lsi->actions); >>> + build_ip_routing_flows_for_lrouter_port(op, lsi->lflows); >>> + build_ND_RA_flows_for_lrouter_port( >>> + op, lsi->lflows, &lsi->match, &lsi->actions); >>> + build_arp_resolve_flows_for_lrouter_port( >>> + op, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions); >>> + build_egress_delivery_flows_for_lrouter_port( >>> + op, lsi->lflows, &lsi->match, &lsi->actions); >>> +} >>> + >>> +/* >>> + * Combined LFLOW processing function. Intended to iterate over >> >> Why LFLOW capitalized? >> >>> + * datapaths, ports, lbs and igmp_groups in single threaded mode >>> + * or prepare and invoke a thread pool in multi-threaded mode. >> >> Ditto. >> >>> + * Must not contain any direct flow ops - all actual flow ops >>> + * should be invoked out the per-iterable helper functions. >>> + */ >>> + >>> +static void >>> +build_converged_flows(struct hmap *datapaths, struct hmap *ports, >> >> This function name looks really strange and also names of both other fucntions >> in this patch. I'd say we need a better name. Current one makes an impression >> that we have some kind of entity named 'converged flow'. >> >>> + struct hmap *port_groups, struct hmap *lflows, >>> + struct hmap *mcgroups, struct hmap *igmp_groups, >>> + struct shash *meter_groups, >>> + struct hmap *lbs) >> >> Please, fix up intents. Above lines should be shifted 2 spaces to the right. >> >>> +{ >>> + struct lswitch_flow_build_info lsi; >>> + >> >> Please, don't add extra empty lines between definitions. >> >>> + struct ovn_datapath *od; >>> + struct ovn_port *op; >>> + >>> + char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac); >>> + >>> + lsi.datapaths = datapaths; >>> + lsi.ports = ports; >>> + lsi.port_groups = port_groups; >>> + lsi.lflows = lflows; >>> + lsi.mcgroups = mcgroups; >>> + lsi.igmp_groups = igmp_groups; >>> + lsi.meter_groups = meter_groups; >>> + lsi.lbs = lbs; >>> + lsi.svc_check_match = svc_check_match; >>> + lsi.match = (struct ds) DS_EMPTY_INITIALIZER; >>> + lsi.actions = (struct ds) DS_EMPTY_INITIALIZER; >> >> Why cast? > > GCC being stupid - barfs without. Why? No idea > > lsi.actions = DS_EMPTY_INITIALIZER; > > is syntactically correct, but it does not want to consume it. I see. DS_EMPTY_INITIALIZER expands to '{ NULL, 0, 0 }' and this form is not an expression, i.e. there is a syntax error. Cast makes the right-hand value to be a compound literal that is an expression and could be used in assignment. Please, use a designated initializer instead like I showed below and it will work fine without need to cast, i.e. struct lswitch_flow_build_info lsi = { .datapaths = datapaths, ... .match = DS_EMPTY_INITIALIZER, .actions = DS_EMPTY_INITIALIZER, };
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 3324c9e81..5faa6cee6 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -8874,24 +8874,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, struct ds actions = DS_EMPTY_INITIALIZER; struct ovn_datapath *od; - HMAP_FOR_EACH (od, key_node, datapaths) { - build_adm_ctrl_flows_for_lrouter(od, lflows); - } - struct ovn_port *op; - HMAP_FOR_EACH (op, key_node, ports) { - build_adm_ctrl_flows_for_lrouter_port(op, lflows, &match, &actions); - } - - HMAP_FOR_EACH (od, key_node, datapaths) { - build_neigh_learning_flows_for_lrouter( - od, lflows, &match, &actions); - } - - HMAP_FOR_EACH (op, key_node, ports) { - build_neigh_learning_flows_for_lrouter_port( - op, lflows, &match, &actions); - } /* Drop IP traffic destined to router owned IPs. Part of it is dropped * in stage "lr_in_ip_input" but traffic that could have been unSNATed @@ -9935,63 +9918,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, sset_destroy(&nat_entries); } - HMAP_FOR_EACH (op, key_node, ports) { - build_ND_RA_flows_for_lrouter_port(op, lflows, &match, &actions); - } - - /* Logical router ingress table ND_RA_OPTIONS & ND_RA_RESPONSE: RS - * responder, by default goto next. (priority 0). */ - HMAP_FOR_EACH (od, key_node, datapaths) { - build_ND_RA_flows_for_lrouter(od, lflows); - } - - HMAP_FOR_EACH (op, key_node, ports) { - build_ip_routing_flows_for_lrouter_port(op, lflows); - } - - /* Convert the static routes to flows. */ - HMAP_FOR_EACH (od, key_node, datapaths) { - build_static_route_flows_for_lrouter(od, lflows, ports); - } - - HMAP_FOR_EACH (od, key_node, datapaths) { - build_mcast_lookup_flows_for_lrouter(od, lflows, &match, &actions); - } - - HMAP_FOR_EACH (od, key_node, datapaths) { - build_ingress_policy_flows_for_lrouter(od, lflows, ports); - } - - /* XXX destination unreachable */ - - HMAP_FOR_EACH (od, key_node, datapaths) { - build_arp_resolve_flows_for_lrouter(od, lflows); - } - - HMAP_FOR_EACH (op, key_node, ports) { - build_arp_resolve_flows_for_lrouter_port( - op, lflows, ports, &match, &actions); - } - - HMAP_FOR_EACH (od, key_node, datapaths) { - build_check_pkt_len_flows_for_lrouter( - od, lflows, ports, &match, &actions); - } - - HMAP_FOR_EACH (od, key_node, datapaths) { - build_gateway_redirect_flows_for_lrouter( - od, lflows, &match, &actions); - } - - HMAP_FOR_EACH (od, key_node, datapaths) { - build_arp_request_flows_for_lrouter(od, lflows, &match, &actions); - } - - HMAP_FOR_EACH (op, key_node, ports) { - build_egress_delivery_flows_for_lrouter_port( - op, lflows, &match, &actions); - } - ds_destroy(&match); ds_destroy(&actions); } @@ -11389,6 +11315,128 @@ build_ipv6_input_flows_for_lrouter_port( } +struct lswitch_flow_build_info { + struct hmap *datapaths; + struct hmap *ports; + struct hmap *port_groups; + struct hmap *lflows; + struct hmap *mcgroups; + struct hmap *igmp_groups; + struct shash *meter_groups; + struct hmap *lbs; + char *svc_check_match; + struct ds match; + struct ds actions; +}; + +/* Helper function to combine all lflow generation which is iterated by + * datapath. It can be invoked with a lsi argument containing "all work" + * in single threaded mode or an lsi argument containing a "chunk of work" + * in parallel. + */ + +static void + build_converged_iterate_by_od( + struct ovn_datapath *od, struct lswitch_flow_build_info *lsi) +{ + + /* Build Logical Router Flows */ + build_adm_ctrl_flows_for_lrouter(od, lsi->lflows); + build_neigh_learning_flows_for_lrouter( + od, lsi->lflows, &lsi->match, &lsi->actions); + build_ND_RA_flows_for_lrouter(od, lsi->lflows); + build_static_route_flows_for_lrouter(od, lsi->lflows, lsi->ports); + build_mcast_lookup_flows_for_lrouter( + od, lsi->lflows, &lsi->match, &lsi->actions); + build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->ports); + build_arp_resolve_flows_for_lrouter(od, lsi->lflows); + build_check_pkt_len_flows_for_lrouter( + od, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions); + build_gateway_redirect_flows_for_lrouter( + od, lsi->lflows, &lsi->match, &lsi->actions); + build_arp_request_flows_for_lrouter( + od, lsi->lflows, &lsi->match, &lsi->actions); +} + +/* Helper function to combine all lflow generation which is iterated by port. + * It can be invoked with a lsi argument containing "all work" in single + * threaded mode or an lsi argument containing a "chunk of work" in parallel. + */ + +static void + build_converged_iterate_by_op( + struct ovn_port *op, + struct lswitch_flow_build_info *lsi) +{ + /* Build Logical Router Flows */ + + build_adm_ctrl_flows_for_lrouter_port( + op, lsi->lflows, &lsi->match, &lsi->actions); + build_neigh_learning_flows_for_lrouter_port( + op, lsi->lflows, &lsi->match, &lsi->actions); + build_ip_routing_flows_for_lrouter_port(op, lsi->lflows); + build_ND_RA_flows_for_lrouter_port( + op, lsi->lflows, &lsi->match, &lsi->actions); + build_arp_resolve_flows_for_lrouter_port( + op, lsi->lflows, lsi->ports, &lsi->match, &lsi->actions); + build_egress_delivery_flows_for_lrouter_port( + op, lsi->lflows, &lsi->match, &lsi->actions); +} + +/* + * Combined LFLOW processing function. Intended to iterate over + * datapaths, ports, lbs and igmp_groups in single threaded mode + * or prepare and invoke a thread pool in multi-threaded mode. + * Must not contain any direct flow ops - all actual flow ops + * should be invoked out the per-iterable helper functions. + */ + +static void +build_converged_flows(struct hmap *datapaths, struct hmap *ports, + struct hmap *port_groups, struct hmap *lflows, + struct hmap *mcgroups, struct hmap *igmp_groups, + struct shash *meter_groups, + struct hmap *lbs) +{ + struct lswitch_flow_build_info lsi; + + struct ovn_datapath *od; + struct ovn_port *op; + + char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac); + + lsi.datapaths = datapaths; + lsi.ports = ports; + lsi.port_groups = port_groups; + lsi.lflows = lflows; + lsi.mcgroups = mcgroups; + lsi.igmp_groups = igmp_groups; + lsi.meter_groups = meter_groups; + lsi.lbs = lbs; + lsi.svc_check_match = svc_check_match; + lsi.match = (struct ds) DS_EMPTY_INITIALIZER; + lsi.actions = (struct ds) DS_EMPTY_INITIALIZER; + + /* Converged build - all lflow generation from lswitch and lrouter + * will move here and will be reogranized by iterator type. + * This allows it to be run (optionally) in parallel */ + HMAP_FOR_EACH (od, key_node, datapaths) { + build_converged_iterate_by_od(od, &lsi); + } + HMAP_FOR_EACH (op, key_node, ports) { + build_converged_iterate_by_op(op, &lsi); + } + free(svc_check_match); + + /* Legacy lswitch build */ + build_lswitch_flows(datapaths, ports, port_groups, lflows, mcgroups, + igmp_groups, meter_groups, lbs); + + /* Legacy lrouter build */ + build_lrouter_flows(datapaths, ports, lflows, meter_groups, lbs); +} + + /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database, * constructing their contents based on the OVN_NB database. */ static void @@ -11400,9 +11448,8 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, { struct hmap lflows = HMAP_INITIALIZER(&lflows); - build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups, + build_converged_flows(datapaths, ports, port_groups, &lflows, mcgroups, igmp_groups, meter_groups, lbs); - build_lrouter_flows(datapaths, ports, &lflows, meter_groups, lbs); /* Push changes to the Logical_Flow table to database. */ const struct sbrec_logical_flow *sbflow, *next_sbflow;