On Thu, Feb 23, 2017 at 11:06 PM, Mickey Spiegel <mickeys....@gmail.com> wrote: > > On Thu, Feb 23, 2017 at 6:04 AM, <nusid...@redhat.com> wrote: > >> From: Numan Siddique <nusid...@redhat.com> >> >> Having zone id per datapath is more than sufficient, because the >> CT tuple information will be unique anyway with in the logical >> datapath. >> > > This proposal conflicts with another proposal that is currently in flight ( > https://mail.openvswitch.org/pipermail/ovs-dev/2017-February/328759.html), > where we were thinking of using ct_label in SFC for load balancing between > multiple port pairs in one port pair group. In that case, for each SFC hop > we would need to pick up a different value from ct_label, so for SFC ports > we would need a different ct_zone for each logical port in one logical > switch. > > Another issue is that this breaks the current use of ct_label.blocked in > ACLs. If the ingress ACL allows a connection but the egress ACL blocks the > connection, then ingress will be clearing the bit while egress will be > setting the bit. Perhaps this could be resolved by replacing > ct_label.blocked with ct_label.blocked_ingress and ct_label.blocked_egress? > There might be other solutions, depending on the future patch that sends to > CT only once for both ingress and egress. > > Mickey > > Thanks Russel and Mickey for comments and pointing out the conflicts. I will abandon the patch for now and revisit later if it is required to optimize for the same host scenario. Or do you think it is good to optimize ? Thanks Numan >> In our testing we have observed that, the packet between two ports of >> a datapath within the same chassis is sent to the CT twice (both in >> ingress and egress pipeline) with (2 different zone ids) resulting in >> some performance hit. With this patch, the packet will use the same >> zone id. This doesn't improve the performace, but a future patch may >> optimize this scenario by sending the packet to CT only once. >> >> Signed-off-by: Numan Siddique <nusid...@redhat.com> >> --- >> ovn/controller/ovn-controller.8.xml | 8 ++++---- >> ovn/controller/ovn-controller.c | 17 +++++++---------- >> ovn/controller/physical.c | 6 ++++-- >> 3 files changed, 15 insertions(+), 16 deletions(-) >> >> diff --git a/ovn/controller/ovn-controller.8.xml >> b/ovn/controller/ovn-controller.8.xml >> index c92fd55..b8bec6c 100644 >> --- a/ovn/controller/ovn-controller.8.xml >> +++ b/ovn/controller/ovn-controller.8.xml >> @@ -209,14 +209,14 @@ >> <code>external_ids:ct-zone-*</code> in the <code>Bridge</code> >> table >> </dt> >> <dd> >> - Logical ports and gateway routers are assigned a connection >> + Logical switch and router datapaths are assigned a connection >> tracking zone by <code>ovn-controller</code> for stateful >> services. To keep state across restarts of >> <code>ovn-controller</code>, these keys are stored in the >> integration bridge's Bridge table. The name contains a prefix >> of <code>ct-zone-</code> followed by the name of the logical >> - port or gateway router's zone key. The value for this key >> - identifies the zone used for this port. >> + datapath's zone key. The value for this key identifies the zone >> used >> + for the datapath. >> </dd> >> >> <dt> >> @@ -309,7 +309,7 @@ >> >> <dt><code>ct-zone-list</code></dt> >> <dd> >> - Lists each local logical port and its connection tracking zone. >> + Lists each local logical datapath and its connection tracking >> zone. >> </dd> >> >> <dt><code>inject-pkt</code> <var>microflow</var></dt> >> diff --git a/ovn/controller/ovn-controller.c >> b/ovn/controller/ovn-controller.c >> index ea299da..696723d 100644 >> --- a/ovn/controller/ovn-controller.c >> +++ b/ovn/controller/ovn-controller.c >> @@ -307,7 +307,7 @@ get_ovnsb_remote(struct ovsdb_idl *ovs_idl) >> } >> >> static void >> -update_ct_zones(struct sset *lports, const struct hmap *local_datapaths, >> +update_ct_zones(const struct hmap *local_datapaths, >> struct simap *ct_zones, unsigned long *ct_zone_bitmap, >> struct shash *pending_ct_zones) >> { >> @@ -316,15 +316,15 @@ update_ct_zones(struct sset *lports, const struct >> hmap *local_datapaths, >> const char *user; >> struct sset all_users = SSET_INITIALIZER(&all_users); >> >> - SSET_FOR_EACH(user, lports) { >> - sset_add(&all_users, user); >> - } >> - >> /* Local patched datapath (gateway routers) need zones assigned. */ >> const struct local_datapath *ld; >> HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { >> /* XXX Add method to limit zone assignment to logical router >> * datapaths with NAT */ >> + char *dp_user = xasprintf(UUID_FMT, >> + UUID_ARGS(&ld->datapath->heade >> r_.uuid)); >> + sset_add(&all_users, dp_user); >> + free(dp_user); >> char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, >> "dnat"); >> char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, >> "snat"); >> sset_add(&all_users, dnat); >> @@ -350,10 +350,7 @@ update_ct_zones(struct sset *lports, const struct >> hmap *local_datapaths, >> } >> } >> >> - /* xxx This is wasteful to assign a zone to each port--even if no >> - * xxx security policy is applied. */ >> - >> - /* Assign a unique zone id for each logical port and two zones >> + /* Assign a unique zone id for each local datapath and two zones >> * to a gateway router. */ >> SSET_FOR_EACH(user, &all_users) { >> int zone; >> @@ -616,7 +613,7 @@ main(int argc, char *argv[]) >> >> &pending_ct_zones); >> >> pinctrl_run(&ctx, &lports, br_int, chassis, >> &local_datapaths); >> - update_ct_zones(&local_lports, &local_datapaths, &ct_zones, >> + update_ct_zones(&local_datapaths, &ct_zones, >> ct_zone_bitmap, &pending_ct_zones); >> if (ctx.ovs_idl_txn) { >> >> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c >> index 0f1aa63..7d44485 100644 >> --- a/ovn/controller/physical.c >> +++ b/ovn/controller/physical.c >> @@ -167,8 +167,10 @@ get_zone_ids(const struct sbrec_port_binding >> *binding, >> { >> struct zone_ids zone_ids; >> >> - zone_ids.ct = simap_get(ct_zones, binding->logical_port); >> - >> + char *dp_uuid = xasprintf(UUID_FMT, >> + UUID_ARGS(&binding->datapath-> >> header_.uuid)); >> + zone_ids.ct = simap_get(ct_zones, dp_uuid); >> + free(dp_uuid); >> const struct uuid *key = &binding->datapath->header_.uuid; >> >> char *dnat = alloc_nat_zone_key(key, "dnat"); >> -- >> 2.9.3 >> >> _______________________________________________ >> 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
