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 > > 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-> > header_.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
