| Message ID | CAN0FunvBCMdasL=JP3NEB6eH9ysM5PLrLtsW=gYOt6QqZy6cuQ@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
"dev" <dev-bounces@openvswitch.org> wrote on 07/07/2016 12:07:16 PM: > From: Zong Kai Li <zealokii@gmail.com> > To: ovs dev <dev@openvswitch.org> > Date: 07/07/2016 12:07 PM > Subject: [ovs-dev] [PATCH] [RFC Patch] ovn-controller: ignore lflow > matching remote VM port > Sent by: "dev" <dev-bounces@openvswitch.org> > > Currently, ovn-controller will install all lflows for a logical > switch, when ovn-controller determines not to skip processing of > that logical switch. > > This will install too many OVS flows. We have 11 tables for logical > switch ingress pipeline, 8 tables for logical switch egress pipeline > now, and more in futrue. > > There are two kind lflows in for logical switch. One has no > inport/outport matching, such as lflows in table ls_in_arp_rsp and > ls_in_l2_lkup. The other one has, and for now, lflows in the following > tables belong to this type: > - ls_in_port_sec_l2 > - ls_in_port_sec_ip > - ls_in_port_sec_nd > - ls_in_acl > - ls_out_pre_acl > - ls_out_acl > - ls_out_port_sec_ip > - ls_out_port_sec_l2 > > Consider how packet trip through flows in network topology > (P: port, S: switch, R: router. > Two VM(or VIF) ports are on different chassis): > - P-S-P: only flows matching remote inport, local VM port as "inport" and > local VM port as "outport" will be matched. There is no chance for > flows matching remote VM port as "inport" or "outport" to be > matched. > - P-S-R-S-P and P-S-R...R-S-P: all these cases seem different from the > above one, but they have the same "last jump". No matter how > many routers(with or without switches) are used, before packet > leaves current chassis, the next jump will be: > destination_switch_gateway -> destination_switch_port, > so it will become a P-S-P case again. > And sinse this patch will not change ingress pipeline for > logical routers, so traffic between router port to router port > will not be impacted. > So, as we can see, we don't need to install flow for a lflow with inport > or outport matching in logical switch ingress pipeline, when it tries to > match > a VM(or VIF) port that doesn't belong to current chassis. > This can help ovn-controller to avoid to install many unnecessary flows. > > Signed-off-by: Zong Kai LI <zealokii@gmail.com> > --- First, how much does this reduce the number of installed flows? Some statistics would be useful... Second, assuming that conditional monitoring lands, will this have any further effect? Ryan
On Thu, Jul 7, 2016 at 12:07 PM, Zong Kai Li <zealokii@gmail.com> wrote: > Currently, ovn-controller will install all lflows for a logical > switch, when ovn-controller determines not to skip processing of > that logical switch. > > This will install too many OVS flows. We have 11 tables for logical > switch ingress pipeline, 8 tables for logical switch egress pipeline > now, and more in futrue. > > There are two kind lflows in for logical switch. One has no > inport/outport matching, such as lflows in table ls_in_arp_rsp and > ls_in_l2_lkup. The other one has, and for now, lflows in the following > tables belong to this type: > - ls_in_port_sec_l2 > - ls_in_port_sec_ip > - ls_in_port_sec_nd > - ls_in_acl > - ls_out_pre_acl > - ls_out_acl > - ls_out_port_sec_ip > - ls_out_port_sec_l2 > > Consider how packet trip through flows in network topology > (P: port, S: switch, R: router. > Two VM(or VIF) ports are on different chassis): > - P-S-P: only flows matching remote inport, local VM port as "inport" and > local VM port as "outport" will be matched. There is no chance for > flows matching remote VM port as "inport" or "outport" to be > matched. > - P-S-R-S-P and P-S-R...R-S-P: all these cases seem different from the > above one, but they have the same "last jump". No matter how > many routers(with or without switches) are used, before packet > leaves current chassis, the next jump will be: > destination_switch_gateway -> destination_switch_port, > so it will become a P-S-P case again. > And sinse this patch will not change ingress pipeline for > logical routers, so traffic between router port to router port > will not be impacted. > So, as we can see, we don't need to install flow for a lflow with inport > or outport matching in logical switch ingress pipeline, when it tries to > match > a VM(or VIF) port that doesn't belong to current chassis. > This can help ovn-controller to avoid to install many unnecessary flows. > > Signed-off-by: Zong Kai LI <zealokii@gmail.com> > --- The patch appears to be corrupt, as trying to apply it fails like so: Kyles-MacBook-Pro-3:ovs mestery$ git apply ~/Downloads/ovs-dev-RFC-ovn-controller-ignore-lflow-matching-remote-VM-port\(1\).patch fatal: corrupt patch at line 6 Kyles-MacBook-Pro-3:ovs mestery$ Further, addressing that fix in the patch leads to others which need to be fixed. > ovn/controller/lflow.c | 42 > +++++++++++++++++++++++++++++++++++------ > ovn/controller/lflow.h | 3 ++- > ovn/controller/ovn-controller.c | 2 +- > 3 files changed, 39 insertions(+), 8 deletions(-) > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > index 05e1eaf..b0602b0 100644 > --- a/ovn/controller/lflow.c > +++ b/ovn/controller/lflow.c > @@ -323,7 +323,8 @@ static void consider_logical_flow(const struct > lport_index *lports, > const struct simap *ct_zones, > struct hmap *dhcp_opts_p, > uint32_t *conj_id_ofs_p, > - struct hmap *flow_table); > + struct hmap *flow_table, > + const char* chassis_id); > > static bool > lookup_port_cb(const void *aux_, const char *port_name, unsigned int > *portp) > @@ -361,7 +362,8 @@ add_logical_flows(struct controller_ctx *ctx, const > struct lport_index *lports, > const struct hmap *local_datapaths, > const struct hmap *patched_datapaths, > struct group_table *group_table, > - const struct simap *ct_zones, struct hmap *flow_table) > + const struct simap *ct_zones, struct hmap *flow_table, > + const char* chassis_id) > { > uint32_t conj_id_ofs = 1; > > @@ -376,7 +378,8 @@ add_logical_flows(struct controller_ctx *ctx, const > struct lport_index *lports, > SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) { > consider_logical_flow(lports, mcgroups, lflow, local_datapaths, > patched_datapaths, group_table, ct_zones, > - &dhcp_opts, &conj_id_ofs, flow_table); > + &dhcp_opts, &conj_id_ofs, flow_table, > + chassis_id); > } > > dhcp_opts_destroy(&dhcp_opts); > @@ -392,7 +395,8 @@ consider_logical_flow(const struct lport_index *lports, > const struct simap *ct_zones, > struct hmap *dhcp_opts_p, > uint32_t *conj_id_ofs_p, > - struct hmap *flow_table) > + struct hmap *flow_table, > + const char* chassis_id) > { > /* Determine translation of logical table IDs to physical table IDs. */ > bool ingress = !strcmp(lflow->pipeline, "ingress"); > @@ -436,6 +440,30 @@ consider_logical_flow(const struct lport_index *lports, > return; > } > } > + > + /* Skip logical flow when it has an 'inport' or 'outport' to match, > + * and the port is a VM or VIF interface, but not a local port to > + * current chassis. */ > + if (strstr(lflow->match, "inport") > + || strstr(lflow->match, "outport")) { > + struct lexer lexer; > + lexer_init(&lexer, lflow->match); > + do { > + lexer_get(&lexer); > + } while (lexer.token.type != LEX_T_ID > + || (strcmp(lexer.token.s, "inport") > + && strcmp(lexer.token.s, "outport"))); > + /* Skip "==", then get logical port name. */ > + lexer_get(&lexer); > + lexer_get(&lexer); > + const struct sbrec_port_binding *pb > + = lport_lookup_by_name(lports, lexer.token.s); > + lexer_destroy(&lexer); > + if (pb && pb->chassis && !strcmp(pb->type, "") > + && strcmp(chassis_id, pb->chassis->name)){ > + return; > + } > + } > } > > /* Determine translation of logical table IDs to physical table IDs. */ > @@ -627,11 +655,13 @@ lflow_run(struct controller_ctx *ctx, const struct > lport_index *lports, > const struct hmap *local_datapaths, > const struct hmap *patched_datapaths, > struct group_table *group_table, > - const struct simap *ct_zones, struct hmap *flow_table) > + const struct simap *ct_zones, struct hmap *flow_table, > + const char* chassis_id) > { > update_address_sets(ctx); > add_logical_flows(ctx, lports, mcgroups, local_datapaths, > - patched_datapaths, group_table, ct_zones, > flow_table); > + patched_datapaths, group_table, ct_zones, flow_table, > + chassis_id); > add_neighbor_flows(ctx, lports, flow_table); > } > > diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h > index e96a24b..859e614 100644 > --- a/ovn/controller/lflow.h > +++ b/ovn/controller/lflow.h > @@ -66,7 +66,8 @@ void lflow_run(struct controller_ctx *, const struct > lport_index *, > const struct hmap *patched_datapaths, > struct group_table *group_table, > const struct simap *ct_zones, > - struct hmap *flow_table); > + struct hmap *flow_table, > + const char* chassis_id); > void lflow_destroy(void); > > #endif /* ovn/lflow.h */ > diff --git a/ovn/controller/ovn-controller.c > b/ovn/controller/ovn-controller.c > index 8471f64..c20949f 100644 > --- a/ovn/controller/ovn-controller.c > +++ b/ovn/controller/ovn-controller.c > @@ -444,7 +444,7 @@ main(int argc, char *argv[]) > struct hmap flow_table = HMAP_INITIALIZER(&flow_table); > lflow_run(&ctx, &lports, &mcgroups, &local_datapaths, > &patched_datapaths, &group_table, &ct_zones, > - &flow_table); > + &flow_table, chassis_id); > if (chassis_id) { > physical_run(&ctx, mff_ovn_geneve, > br_int, chassis_id, &ct_zones, &flow_table, > -- > 1.9.1 > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index 05e1eaf..b0602b0 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -323,7 +323,8 @@ static void consider_logical_flow(const struct lport_index *lports, const struct simap *ct_zones, struct hmap *dhcp_opts_p, uint32_t *conj_id_ofs_p, - struct hmap *flow_table); + struct hmap *flow_table, + const char* chassis_id); static bool lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) @@ -361,7 +362,8 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, const struct hmap *local_datapaths, const struct hmap *patched_datapaths, struct group_table *group_table, - const struct simap *ct_zones, struct hmap *flow_table) + const struct simap *ct_zones, struct hmap *flow_table, + const char* chassis_id) { uint32_t conj_id_ofs = 1; @@ -376,7 +378,8 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) { consider_logical_flow(lports, mcgroups, lflow, local_datapaths, patched_datapaths, group_table, ct_zones, - &dhcp_opts, &conj_id_ofs, flow_table); + &dhcp_opts, &conj_id_ofs, flow_table, + chassis_id); } dhcp_opts_destroy(&dhcp_opts); @@ -392,7 +395,8 @@ consider_logical_flow(const struct lport_index *lports, const struct simap *ct_zones, struct hmap *dhcp_opts_p, uint32_t *conj_id_ofs_p, - struct hmap *flow_table) + struct hmap *flow_table, + const char* chassis_id) { /* Determine translation of logical table IDs to physical table IDs. */ bool ingress = !strcmp(lflow->pipeline, "ingress"); @@ -436,6 +440,30 @@ consider_logical_flow(const struct lport_index *lports, return; } } + + /* Skip logical flow when it has an 'inport' or 'outport' to match, + * and the port is a VM or VIF interface, but not a local port to + * current chassis. */ + if (strstr(lflow->match, "inport") + || strstr(lflow->match, "outport")) { + struct lexer lexer; + lexer_init(&lexer, lflow->match); + do { + lexer_get(&lexer); + } while (lexer.token.type != LEX_T_ID + || (strcmp(lexer.token.s, "inport") + && strcmp(lexer.token.s, "outport"))); + /* Skip "==", then get logical port name. */ + lexer_get(&lexer); + lexer_get(&lexer); + const struct sbrec_port_binding *pb + = lport_lookup_by_name(lports, lexer.token.s); + lexer_destroy(&lexer); + if (pb && pb->chassis && !strcmp(pb->type, "") + && strcmp(chassis_id, pb->chassis->name)){ + return; + } + } } /* Determine translation of logical table IDs to physical table IDs. */ @@ -627,11 +655,13 @@ lflow_run(struct controller_ctx *ctx, const struct lport_index *lports, const struct hmap *local_datapaths, const struct hmap *patched_datapaths, struct group_table *group_table, - const struct simap *ct_zones, struct hmap *flow_table) + const struct simap *ct_zones, struct hmap *flow_table, + const char* chassis_id) { update_address_sets(ctx); add_logical_flows(ctx, lports, mcgroups, local_datapaths, - patched_datapaths, group_table, ct_zones, flow_table); + patched_datapaths, group_table, ct_zones, flow_table, + chassis_id); add_neighbor_flows(ctx, lports, flow_table); } diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h index e96a24b..859e614 100644 --- a/ovn/controller/lflow.h +++ b/ovn/controller/lflow.h @@ -66,7 +66,8 @@ void lflow_run(struct controller_ctx *, const struct lport_index *, const struct hmap *patched_datapaths, struct group_table *group_table, const struct simap *ct_zones, - struct hmap *flow_table); + struct hmap *flow_table, + const char* chassis_id); void lflow_destroy(void); #endif /* ovn/lflow.h */ diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 8471f64..c20949f 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -444,7 +444,7 @@ main(int argc, char *argv[]) struct hmap flow_table = HMAP_INITIALIZER(&flow_table); lflow_run(&ctx, &lports, &mcgroups, &local_datapaths, &patched_datapaths, &group_table, &ct_zones, - &flow_table); + &flow_table, chassis_id); if (chassis_id) { physical_run(&ctx, mff_ovn_geneve, br_int, chassis_id, &ct_zones, &flow_table,
Currently, ovn-controller will install all lflows for a logical switch, when ovn-controller determines not to skip processing of that logical switch. This will install too many OVS flows. We have 11 tables for logical switch ingress pipeline, 8 tables for logical switch egress pipeline now, and more in futrue. There are two kind lflows in for logical switch. One has no inport/outport matching, such as lflows in table ls_in_arp_rsp and ls_in_l2_lkup. The other one has, and for now, lflows in the following tables belong to this type: - ls_in_port_sec_l2 - ls_in_port_sec_ip - ls_in_port_sec_nd - ls_in_acl - ls_out_pre_acl - ls_out_acl - ls_out_port_sec_ip - ls_out_port_sec_l2 Consider how packet trip through flows in network topology (P: port, S: switch, R: router. Two VM(or VIF) ports are on different chassis): - P-S-P: only flows matching remote inport, local VM port as "inport" and local VM port as "outport" will be matched. There is no chance for flows matching remote VM port as "inport" or "outport" to be matched. - P-S-R-S-P and P-S-R...R-S-P: all these cases seem different from the above one, but they have the same "last jump". No matter how many routers(with or without switches) are used, before packet leaves current chassis, the next jump will be: destination_switch_gateway -> destination_switch_port, so it will become a P-S-P case again. And sinse this patch will not change ingress pipeline for logical routers, so traffic between router port to router port will not be impacted. So, as we can see, we don't need to install flow for a lflow with inport or outport matching in logical switch ingress pipeline, when it tries to match a VM(or VIF) port that doesn't belong to current chassis. This can help ovn-controller to avoid to install many unnecessary flows. Signed-off-by: Zong Kai LI <zealokii@gmail.com> --- ovn/controller/lflow.c | 42 +++++++++++++++++++++++++++++++++++------ ovn/controller/lflow.h | 3 ++- ovn/controller/ovn-controller.c | 2 +- 3 files changed, 39 insertions(+), 8 deletions(-)