Re: [ovs-dev] [PATCH] ovn-controller: Refactor function of consider_port_binding

[Ben Pfaff]

On Fri, Aug 04, 2017 at 10:31:14AM +0800, wang.qia...@zte.com.cn wrote:
> The function of consider_port_binding is redundant. This patch split the 
> function to some sub-function by the port type.
> 
> Signed-off-by: wang qianyu 

Can you explain how it is redundant, and how this patch reduces it?

This patch makes two identical copies of the code for handling L3
gateway ports and patch ports, as patch_port_handle() and
l3gateway_port_handle(), so it's hard for me to see what redundancy it
eliminates.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovn-controller: Refactor function of consider_port_binding

[wang . qianyu]

The function of consider_port_binding is redundant. This patch split the 
function to some sub-function by the port type.

Signed-off-by: wang qianyu 
---
 ovn/controller/physical.c | 816 
+++---
 1 file changed, 482 insertions(+), 334 deletions(-)

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 719b020..48b6165 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -291,144 +291,200 @@ load_logical_ingress_metadata(const struct 
sbrec_port_binding *binding,
 }
 
 static void
-consider_port_binding(enum mf_field_id mff_ovn_geneve,
-  const struct simap *ct_zones,
+patch_port_handle(const struct simap *ct_zones,
   const struct lport_index *lports,
-  const struct chassis_index *chassis_index,
-  struct sset *active_tunnels,
-  struct hmap *local_datapaths,
   const struct sbrec_port_binding *binding,
-  const struct sbrec_chassis *chassis,
   struct ofpbuf *ofpacts_p,
-  struct hmap *flow_table)
+  struct hmap *flow_table,
+  uint32_t dp_key,
+  uint32_t port_key)
 {
-uint32_t dp_key = binding->datapath->tunnel_key;
-uint32_t port_key = binding->tunnel_key;
-if (!get_local_datapath(local_datapaths, dp_key)) {
+const char *peer_name = smap_get(>options, "peer");
+if (!peer_name) {
+return;
+}
+
+const struct sbrec_port_binding *peer = lport_lookup_by_name(
+lports, peer_name);
+if (!peer || strcmp(peer->type, binding->type)) {
+return;
+}
+const char *peer_peer_name = smap_get(>options, "peer");
+if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) 
{
 return;
 }
 
+struct zone_ids binding_zones = get_zone_ids(binding, ct_zones);
+put_local_common_flows(dp_key, port_key, false, _zones,
+   ofpacts_p, flow_table);
+
 struct match match;
-if (!strcmp(binding->type, "patch")
-|| (!strcmp(binding->type, "l3gateway")
-&& binding->chassis == chassis)) {
-const char *peer_name = smap_get(>options, "peer");
-if (!peer_name) {
-return;
-}
+match_init_catchall();
+ofpbuf_clear(ofpacts_p);
+match_set_metadata(, htonll(dp_key));
+match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key);
 
-const struct sbrec_port_binding *peer = lport_lookup_by_name(
-lports, peer_name);
-if (!peer || strcmp(peer->type, binding->type)) {
-return;
-}
-const char *peer_peer_name = smap_get(>options, "peer");
-if (!peer_peer_name || strcmp(peer_peer_name, 
binding->logical_port)) {
-return;
-}
+size_t clone_ofs = ofpacts_p->size;
+struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts_p);
+ofpact_put_CT_CLEAR(ofpacts_p);
+put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
+struct zone_ids peer_zones = get_zone_ids(peer, ct_zones);
+load_logical_ingress_metadata(peer, _zones, ofpacts_p);
+put_load(0, MFF_LOG_FLAGS, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
+for (int i = 0; i < MFF_N_LOG_REGS; i++) {
+put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p);
+}
+put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
+put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
+clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone);
+ofpacts_p->header = clone;
+ofpact_finish_CLONE(ofpacts_p, );
 
-struct zone_ids binding_zones = get_zone_ids(binding, ct_zones);
-put_local_common_flows(dp_key, port_key, false, _zones,
-   ofpacts_p, flow_table);
-
-match_init_catchall();
-ofpbuf_clear(ofpacts_p);
-match_set_metadata(, htonll(dp_key));
-match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key);
-
-size_t clone_ofs = ofpacts_p->size;
-struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts_p);
-ofpact_put_CT_CLEAR(ofpacts_p);
-put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
-put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
-put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
-struct zone_ids peer_zones = get_zone_ids(peer, ct_zones);
-load_logical_ingress_metadata(peer, _zones, ofpacts_p);
-put_load(0, MFF_LOG_FLAGS, 0, 32, ofpacts_p);
-put_load(0, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
-for (int i = 0; i < MFF_N_LOG_REGS; i++) {
-put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p);
-}
-put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
-

Re: [ovs-dev] [PATCH] ovn-controller: Refactor function of consider_port_binding

[Ben Pfaff]

On Fri, Jul 28, 2017 at 11:32:29AM +0800, wang.qia...@zte.com.cn wrote:
> The function of consider_port_binding is redundant. This patch divide the 
> function to some sub-function by the port type.
> 
> Change-Id: I86a408e97e6d6211f3695cf42fc5b858352ac7ff
> Signed-off-by: wang qianyu 

Thank you for the contribution.

This patch is word-wrapped and cannot be applied.  Can you re-post it,
please?  "git send-email" always works properly.  Or you can use a
github pull request if that can't work.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovn-controller: Refactor function of consider_port_binding

[wang . qianyu]

The function of consider_port_binding is redundant. This patch divide the 
function to some sub-function by the port type.

Change-Id: I86a408e97e6d6211f3695cf42fc5b858352ac7ff
Signed-off-by: wang qianyu 
---
 ovn/controller/physical.c | 819 
+++---
 1 file changed, 484 insertions(+), 335 deletions(-)

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 719b020..28dcd5e 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2015, 2016, 2017 Nicira, Inc.
+锘?* Copyright (c) 2015, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -291,144 +291,200 @@ load_logical_ingress_metadata(const struct 
sbrec_port_binding *binding,
 }
 
 static void
-consider_port_binding(enum mf_field_id mff_ovn_geneve,
-  const struct simap *ct_zones,
+patch_port_handle(const struct simap *ct_zones,
   const struct lport_index *lports,
-  const struct chassis_index *chassis_index,
-  struct sset *active_tunnels,
-  struct hmap *local_datapaths,
   const struct sbrec_port_binding *binding,
-  const struct sbrec_chassis *chassis,
   struct ofpbuf *ofpacts_p,
-  struct hmap *flow_table)
+  struct hmap *flow_table,
+  uint32_t dp_key,
+  uint32_t port_key)
 {
-uint32_t dp_key = binding->datapath->tunnel_key;
-uint32_t port_key = binding->tunnel_key;
-if (!get_local_datapath(local_datapaths, dp_key)) {
+const char *peer_name = smap_get(>options, "peer");
+if (!peer_name) {
+return;
+}
+
+const struct sbrec_port_binding *peer = lport_lookup_by_name(
+lports, peer_name);
+if (!peer || strcmp(peer->type, binding->type)) {
+return;
+}
+const char *peer_peer_name = smap_get(>options, "peer");
+if (!peer_peer_name || strcmp(peer_peer_name, binding->logical_port)) 
{
 return;
 }
 
+struct zone_ids binding_zones = get_zone_ids(binding, ct_zones);
+put_local_common_flows(dp_key, port_key, false, _zones,
+   ofpacts_p, flow_table);
+
 struct match match;
-if (!strcmp(binding->type, "patch")
-|| (!strcmp(binding->type, "l3gateway")
-&& binding->chassis == chassis)) {
-const char *peer_name = smap_get(>options, "peer");
-if (!peer_name) {
-return;
-}
+match_init_catchall();
+ofpbuf_clear(ofpacts_p);
+match_set_metadata(, htonll(dp_key));
+match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key);
 
-const struct sbrec_port_binding *peer = lport_lookup_by_name(
-lports, peer_name);
-if (!peer || strcmp(peer->type, binding->type)) {
-return;
-}
-const char *peer_peer_name = smap_get(>options, "peer");
-if (!peer_peer_name || strcmp(peer_peer_name, 
binding->logical_port)) {
-return;
-}
+size_t clone_ofs = ofpacts_p->size;
+struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts_p);
+ofpact_put_CT_CLEAR(ofpacts_p);
+put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
+struct zone_ids peer_zones = get_zone_ids(peer, ct_zones);
+load_logical_ingress_metadata(peer, _zones, ofpacts_p);
+put_load(0, MFF_LOG_FLAGS, 0, 32, ofpacts_p);
+put_load(0, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
+for (int i = 0; i < MFF_N_LOG_REGS; i++) {
+put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p);
+}
+put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
+put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
+clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone);
+ofpacts_p->header = clone;
+ofpact_finish_CLONE(ofpacts_p, );
 
-struct zone_ids binding_zones = get_zone_ids(binding, ct_zones);
-put_local_common_flows(dp_key, port_key, false, _zones,
-   ofpacts_p, flow_table);
-
-match_init_catchall();
-ofpbuf_clear(ofpacts_p);
-match_set_metadata(, htonll(dp_key));
-match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key);
-
-size_t clone_ofs = ofpacts_p->size;
-struct ofpact_nest *clone = ofpact_put_CLONE(ofpacts_p);
-ofpact_put_CT_CLEAR(ofpacts_p);
-put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
-put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
-put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
-struct zone_ids peer_zones = get_zone_ids(peer, ct_zones);
-load_logical_ingress_metadata(peer, _zones, ofpacts_p);
-