| Message ID | 20200911094113.5991-1-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v5,01/16] ovn-northd: Move out Table 0 (ingress) operations to functions | expand |
On Fri, Sep 11, 2020 at 3:11 PM <anton.ivanov@cambridgegreys.com> wrote: > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > Signed-off-by: Anton Ivanov <anton.ivanov@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. 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Tue, Sep 15, 2020 at 4:42 PM Numan Siddique <numans@ovn.org> wrote: > > > > On Fri, Sep 11, 2020 at 3:11 PM <anton.ivanov@cambridgegreys.com> wrote: > >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> Signed-off-by: Anton Ivanov <anton.ivanov@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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >>
On 15/09/2020 12:12, Numan Siddique wrote: > > > > On Fri, Sep 11, 2020 at 3:11 PM <anton.ivanov@cambridgegreys.com > <mailto:anton.ivanov@cambridgegreys.com>> wrote: > > From: Anton Ivanov <anton.ivanov@cambridgegreys.com > <mailto:anton.ivanov@cambridgegreys.com>> > > Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com > <mailto:anton.ivanov@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. Cool, thanks. I will rebase my branch off that. > > 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. Cool. I found that in its absence, diff generates stuff that is not readable and impossible to rebase even after minimal changes. In the meantime, I reworked the actual multi-threaded processing patch so that it unifies lswitch and lrouter processing and runs the worker trheadpool once per iteration. I am going to re-test that - it should improve performance and scalability a bit further. A. > > 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 > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 15/09/2020 13:02, Numan Siddique wrote: > > > On Tue, Sep 15, 2020 at 4:42 PM Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org>> wrote: > > > > > On Fri, Sep 11, 2020 at 3:11 PM <anton.ivanov@cambridgegreys.com > <mailto:anton.ivanov@cambridgegreys.com>> wrote: > > From: Anton Ivanov <anton.ivanov@cambridgegreys.com > <mailto:anton.ivanov@cambridgegreys.com>> > > Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com > <mailto:anton.ivanov@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. Cool. Thanks. I have pushed the parallelised ovn-northd branch on top of my changes (I have not rebased it yet). https://github.com/kot-begemot-uk/ovn/tree/reintroduce-parallel-processing You can see some of the rationale for the naming there - in the last few commits the actual processing is re-arranged and then the naming convention starts making a bit more sense as it fits the way things are iterated. A. > 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 > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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