On Tue, Sep 15, 2020 at 4:42 PM Numan Siddique <num...@ovn.org> wrote:
> > > > On Fri, Sep 11, 2020 at 3:11 PM <anton.iva...@cambridgegreys.com> wrote: > >> From: Anton Ivanov <anton.iva...@cambridgegreys.com> >> >> Signed-off-by: Anton Ivanov <anton.iva...@cambridgegreys.com> >> > > 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. > Another thing which I forgot to mention is that I modified the commit messages for most of the commits. Thanks Numan > 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 >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev