On Fri, Sep 11, 2020 at 3:11 PM <[email protected]> wrote:
> From: Anton Ivanov <[email protected]> > > Signed-off-by: Anton Ivanov <[email protected]> > Hi Anton, Thanks for this series. I found most of the patches to be splitting the code into functions into proper logical blocks. However, patches 3 to 7, split the code into functions which I think can be further reorganized properly. What I mean is that if we take ARP response flows for NAT entries as an example, the function build_lrouter_flows_ip_input_od() in the patch 3 adds ARP response flows for some scenarios and the function build_lrouter_flows_ipv4_input_table_3_op() in patch 4 adds ARP response flows for other scenarios. I think it's better to revisit patches 3 to 7 and move out the code into functions which separates the lflow generation properly. I also think the function names are a bit odd and the naming can be improved. I found other patches in the series to be fine except for the function naming. Hence I reworked a bit renaming the functions and moving the comments from the function declarations to near the function definitions. I think this will be more helpful. And I applied the patches 1,2, 8 to 16 to master. Please note I also removed the marker comment while committing the patch 16. I think we can remove it as it's a bit odd to keep the comment. Thanks Numan > --- > northd/ovn-northd.c | 135 +++++++++++++++++++++++++++----------------- > 1 file changed, 82 insertions(+), 53 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index b95d6cd8a..14e4a6939 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -8482,6 +8482,26 @@ build_lrouter_force_snat_flows(struct hmap *lflows, > struct ovn_datapath *od, > ds_destroy(&actions); > } > > + > +/* Logical router ingress table 0: Admission control framework. */ > +static void > +build_lrouter_flows_ingress_table_0_od( > + struct ovn_datapath *od, struct hmap *lflows); > + > +/* Logical router ingress table 0: match (priority 50). */ > +static void > +build_lrouter_flows_ingress_table_0_op( > + struct ovn_port *op, struct hmap *lflows, > + struct ds *match, struct ds *actions); > + > +/* > + * Do not remove this comment - it is here on purpose > + * It serves as a marker so that pulling operations out > + * of build_lrouter_flows results in a clean diff with > + * a separate new operations function and clean changes > + * to build_lroute_flows > + */ > + > static void > build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > struct hmap *lflows, struct shash *meter_groups, > @@ -8493,65 +8513,14 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > struct ds match = DS_EMPTY_INITIALIZER; > struct ds actions = DS_EMPTY_INITIALIZER; > > - /* Logical router ingress table 0: Admission control framework. */ > struct ovn_datapath *od; > HMAP_FOR_EACH (od, key_node, datapaths) { > - if (!od->nbr) { > - continue; > - } > - > - /* Logical VLANs not supported. > - * Broadcast/multicast source address is invalid. */ > - ovn_lflow_add(lflows, od, S_ROUTER_IN_ADMISSION, 100, > - "vlan.present || eth.src[40]", "drop;"); > + build_lrouter_flows_ingress_table_0_od(od, lflows); > } > > - /* Logical router ingress table 0: match (priority 50). */ > struct ovn_port *op; > HMAP_FOR_EACH (op, key_node, ports) { > - if (!op->nbrp) { > - continue; > - } > - > - if (!lrport_is_enabled(op->nbrp)) { > - /* Drop packets from disabled logical ports (since logical > flow > - * tables are default-drop). */ > - continue; > - } > - > - if (op->derived) { > - /* No ingress packets should be received on a chassisredirect > - * port. */ > - continue; > - } > - > - /* Store the ethernet address of the port receiving the packet. > - * This will save us from having to match on inport further down > in > - * the pipeline. > - */ > - ds_clear(&actions); > - ds_put_format(&actions, REG_INPORT_ETH_ADDR " = %s; next;", > - op->lrp_networks.ea_s); > - > - ds_clear(&match); > - ds_put_format(&match, "eth.mcast && inport == %s", op->json_key); > - ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION, 50, > - ds_cstr(&match), ds_cstr(&actions), > - &op->nbrp->header_); > - > - ds_clear(&match); > - ds_put_format(&match, "eth.dst == %s && inport == %s", > - op->lrp_networks.ea_s, op->json_key); > - if (op->od->l3dgw_port && op == op->od->l3dgw_port > - && op->od->l3redirect_port) { > - /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s > - * should only be received on the "redirect-chassis". */ > - ds_put_format(&match, " && is_chassis_resident(%s)", > - op->od->l3redirect_port->json_key); > - } > - ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION, 50, > - ds_cstr(&match), ds_cstr(&actions), > - &op->nbrp->header_); > + build_lrouter_flows_ingress_table_0_op(op, lflows, &match, > &actions); > } > > /* Logical router ingress table 1: LOOKUP_NEIGHBOR and > @@ -10956,6 +10925,66 @@ build_lrouter_flows(struct hmap *datapaths, > struct hmap *ports, > ds_destroy(&actions); > } > > +static void > +build_lrouter_flows_ingress_table_0_od( > + struct ovn_datapath *od, struct hmap *lflows) > +{ > + if (od->nbr) { > + /* Logical VLANs not supported. > + * Broadcast/multicast source address is invalid. */ > + ovn_lflow_add(lflows, od, S_ROUTER_IN_ADMISSION, 100, > + "vlan.present || eth.src[40]", "drop;"); > + } > +} > + > +static void > +build_lrouter_flows_ingress_table_0_op( > + struct ovn_port *op, struct hmap *lflows, > + struct ds *match, struct ds *actions) > +{ > + if (op->nbrp) { > + if (!lrport_is_enabled(op->nbrp)) { > + /* Drop packets from disabled logical ports (since logical > flow > + * tables are default-drop). */ > + return; > + } > + > + if (op->derived) { > + /* No ingress packets should be received on a chassisredirect > + * port. */ > + return; > + } > + > + /* Store the ethernet address of the port receiving the packet. > + * This will save us from having to match on inport further down > in > + * the pipeline. > + */ > + ds_clear(actions); > + ds_put_format(actions, REG_INPORT_ETH_ADDR " = %s; next;", > + op->lrp_networks.ea_s); > + > + ds_clear(match); > + ds_put_format(match, "eth.mcast && inport == %s", op->json_key); > + ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION, 50, > + ds_cstr(match), ds_cstr(actions), > + &op->nbrp->header_); > + > + ds_clear(match); > + ds_put_format(match, "eth.dst == %s && inport == %s", > + op->lrp_networks.ea_s, op->json_key); > + if (op->od->l3dgw_port && op == op->od->l3dgw_port > + && op->od->l3redirect_port) { > + /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s > + * should only be received on the "redirect-chassis". */ > + ds_put_format(match, " && is_chassis_resident(%s)", > + op->od->l3redirect_port->json_key); > + } > + ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ADMISSION, 50, > + ds_cstr(match), ds_cstr(actions), > + &op->nbrp->header_); > + } > +} > + > /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB > database, > * constructing their contents based on the OVN_NB database. */ > static void > -- > 2.20.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
