[ovs-dev] [PATCH ovn v2 4/5] I-P: Handle runtime data changes for plow_output engine.
Han Zhou
hzhou at ovn.org
Tue Jul 27 19:59:40 UTC 2021
On Tue, Jul 27, 2021 at 11:43 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Tue, Jul 27, 2021 at 3:36 AM Han Zhou <hzhou at ovn.org> wrote:
> >
> > Thanks Numan. The commit title has a typo: s/plow/pflow.
> >
> > On Fri, Jul 16, 2021 at 4:55 AM <numans at ovn.org> wrote:
> > >
> > > From: Numan Siddique <numans at ovn.org>
> > >
> > > physical_run() maintains a local copy of local vif to ofports
> > > in a simap along with the chassis tunnel information. This patch
> > > removes this from the physical module and now stores it in the
> > > runtime_data engine node. This makes it easier to handle runtime
> > > data changes in pflow_output engine.
> > >
> > > The newly added handler pflow_output_runtime_data_handler() returns
> > > false if a datapath is added or removed from the local_datapaths
> > > and handles the logical port claims and releases incrementally.
> > >
> > > Signed-off-by: Numan Siddique <numans at ovn.org>
> > > ---
> > > controller/binding.c | 39 ++--
> > > controller/binding.h | 23 +++
> > > controller/ldata.c | 169 ++++++++++++++++
> > > controller/ldata.h | 44 +++++
> > > controller/lflow.c | 5 +-
> > > controller/lflow.h | 1 +
> > > controller/ovn-controller.c | 68 ++++++-
> > > controller/ovn-controller.h | 8 -
> > > controller/physical.c | 382
+++++++++---------------------------
> > > controller/physical.h | 13 +-
> > > 10 files changed, 422 insertions(+), 330 deletions(-)
> > >
> > > diff --git a/controller/binding.c b/controller/binding.c
> > > index 0fd951ad7..b50139726 100644
> > > --- a/controller/binding.c
> > > +++ b/controller/binding.c
> > > @@ -546,23 +546,6 @@ update_active_pb_ras_pd(const struct
> > sbrec_port_binding *pb,
> > > }
> > > }
> > >
> > > -/* Corresponds to each Port_Binding.type. */
> > > -enum en_lport_type {
> > > - LP_UNKNOWN,
> > > - LP_VIF,
> > > - LP_CONTAINER,
> > > - LP_PATCH,
> > > - LP_L3GATEWAY,
> > > - LP_LOCALNET,
> > > - LP_LOCALPORT,
> > > - LP_L2GATEWAY,
> > > - LP_VTEP,
> > > - LP_CHASSISREDIRECT,
> > > - LP_VIRTUAL,
> > > - LP_EXTERNAL,
> > > - LP_REMOTE
> > > -};
> > > -
> > > /* Local bindings. binding.c module binds the logical port
(represented
> > by
> > > * Port_Binding rows) and sets the 'chassis' column when it sees the
> > > * OVS interface row (of type "" or "internal") with the
> > > @@ -616,7 +599,7 @@ static struct local_binding *local_binding_create(
> > > static void local_binding_add(struct shash *local_bindings,
> > > struct local_binding *);
> > > static struct local_binding *local_binding_find(
> > > - struct shash *local_bindings, const char *name);
> > > + const struct shash *local_bindings, const char *name);
> > > static void local_binding_destroy(struct local_binding *,
> > > struct shash *binding_lports);
> > > static void local_binding_delete(struct local_binding *,
> > > @@ -701,7 +684,8 @@ local_binding_data_destroy(struct
local_binding_data
> > *lbinding_data)
> > > }
> > >
> > > const struct sbrec_port_binding *
> > > -local_binding_get_primary_pb(struct shash *local_bindings, const char
> > *pb_name)
> > > +local_binding_get_primary_pb(struct shash *local_bindings,
> > > + const char *pb_name)
> > > {
> > > struct local_binding *lbinding =
> > > local_binding_find(local_bindings, pb_name);
> > > @@ -710,6 +694,19 @@ local_binding_get_primary_pb(struct shash
> > *local_bindings, const char *pb_name)
> > > return b_lport ? b_lport->pb : NULL;
> > > }
> > >
> > > +ofp_port_t
> > > +local_binding_get_lport_ofport(const struct shash *local_bindings,
> > > + const char *pb_name)
> > > +{
> > > + struct local_binding *lbinding =
> > > + local_binding_find(local_bindings, pb_name);
> > > + struct binding_lport *b_lport =
> > > + local_binding_get_primary_or_localport_lport(lbinding);
> > > +
> > > + return (b_lport && lbinding->iface && lbinding->iface->n_ofport)
?
> > > + u16_to_ofp(lbinding->iface->ofport[0]) : 0;
> > > +}
> > > +
> > > bool
> > > local_binding_is_up(struct shash *local_bindings, const char
*pb_name)
> > > {
> > > @@ -871,7 +868,7 @@ is_lport_vif(const struct sbrec_port_binding *pb)
> > > return !pb->type[0];
> > > }
> > >
> > > -static enum en_lport_type
> > > +enum en_lport_type
> > > get_lport_type(const struct sbrec_port_binding *pb)
> > > {
> > > if (is_lport_vif(pb)) {
> > > @@ -2555,7 +2552,7 @@ local_binding_create(const char *name, const
struct
> > ovsrec_interface *iface)
> > > }
> > >
> > > static struct local_binding *
> > > -local_binding_find(struct shash *local_bindings, const char *name)
> > > +local_binding_find(const struct shash *local_bindings, const char
*name)
> > > {
> > > return shash_find_data(local_bindings, name);
> > > }
> > > diff --git a/controller/binding.h b/controller/binding.h
> > > index b1717bd2b..f1abc4b9c 100644
> > > --- a/controller/binding.h
> > > +++ b/controller/binding.h
> > > @@ -114,6 +114,9 @@ void local_binding_data_destroy(struct
> > local_binding_data *);
> > >
> > > const struct sbrec_port_binding *local_binding_get_primary_pb(
> > > struct shash *local_bindings, const char *pb_name);
> > > +ofp_port_t local_binding_get_lport_ofport(const struct shash
> > *local_bindings,
> > > + const char *pb_name);
> > > +
> > > bool local_binding_is_up(struct shash *local_bindings, const char
> > *pb_name);
> > > bool local_binding_is_down(struct shash *local_bindings, const char
> > *pb_name);
> > > void local_binding_set_up(struct shash *local_bindings, const char
> > *pb_name,
> > > @@ -134,4 +137,24 @@ bool binding_handle_port_binding_changes(struct
> > binding_ctx_in *,
> > > void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);
> > >
> > > void binding_dump_local_bindings(struct local_binding_data *, struct
ds
> > *);
> > > +
> > > +/* Corresponds to each Port_Binding.type. */
> > > +enum en_lport_type {
> > > + LP_UNKNOWN,
> > > + LP_VIF,
> > > + LP_CONTAINER,
> > > + LP_PATCH,
> > > + LP_L3GATEWAY,
> > > + LP_LOCALNET,
> > > + LP_LOCALPORT,
> > > + LP_L2GATEWAY,
> > > + LP_VTEP,
> > > + LP_CHASSISREDIRECT,
> > > + LP_VIRTUAL,
> > > + LP_EXTERNAL,
> > > + LP_REMOTE
> > > +};
> > > +
> > > +enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
> > > +
> > > #endif /* controller/binding.h */
> > > diff --git a/controller/ldata.c b/controller/ldata.c
> > > index a6df9b1da..f55905551 100644
> > > --- a/controller/ldata.c
> > > +++ b/controller/ldata.c
> > > @@ -18,10 +18,13 @@
> > > /* OVS includes. */
> > > #include "include/openvswitch/json.h"
> > > #include "lib/hmapx.h"
> > > +#include "lib/flow.h"
> > > #include "lib/util.h"
> > > +#include "lib/vswitch-idl.h"
> > > #include "openvswitch/vlog.h"
> > >
> > > /* OVN includes. */
> > > +#include "encaps.h"
> > > #include "ldata.h"
> > > #include "lport.h"
> > > #include "lib/ovn-util.h"
> > > @@ -275,6 +278,172 @@ tracked_datapaths_destroy(struct hmap
> > *tracked_datapaths)
> > > hmap_destroy(tracked_datapaths);
> > > }
> > >
> > > +/* Iterates the br_int ports and build the simap of patch to ofports
> > > + * and chassis tunnels. */
> > > +void
> > > +ldata_run(const struct ovsrec_bridge *br_int,
> > > + const struct sbrec_chassis *chassis_rec,
> > > + struct local_nonvif_data *nonvif_data)
> > > +{
> > > + for (int i = 0; i < br_int->n_ports; i++) {
> > > + const struct ovsrec_port *port_rec = br_int->ports[i];
> > > + if (!strcmp(port_rec->name, br_int->name)) {
> > > + continue;
> > > + }
> > > +
> > > + const char *tunnel_id = smap_get(&port_rec->external_ids,
> > > + "ovn-chassis-id");
> > > + if (tunnel_id && encaps_tunnel_id_match(tunnel_id,
> > > + chassis_rec->name,
> > > + NULL)) {
> > > + continue;
> > > + }
> > > +
> > > + const char *localnet = smap_get(&port_rec->external_ids,
> > > + "ovn-localnet-port");
> > > + const char *l2gateway = smap_get(&port_rec->external_ids,
> > > + "ovn-l2gateway-port");
> > > +
> > > + for (int j = 0; j < port_rec->n_interfaces; j++) {
> > > + const struct ovsrec_interface *iface_rec =
> > port_rec->interfaces[j];
> > > +
> > > + /* Get OpenFlow port number. */
> > > + if (!iface_rec->n_ofport) {
> > > + continue;
> > > + }
> > > + int64_t ofport = iface_rec->ofport[0];
> > > + if (ofport < 1 || ofport > ofp_to_u16(OFPP_MAX)) {
> > > + continue;
> > > + }
> > > +
> > > + bool is_patch = !strcmp(iface_rec->type, "patch");
> > > + if (is_patch && localnet) {
> > > + simap_put(&nonvif_data->patch_ofports, localnet,
ofport);
> > > + break;
> > > + } else if (is_patch && l2gateway) {
> > > + /* L2 gateway patch ports can be handled just like
VIFs.
> > */
> > > + simap_put(&nonvif_data->patch_ofports, l2gateway,
> > ofport);
> > > + break;
> > > + } else if (tunnel_id) {
> > > + enum chassis_tunnel_type tunnel_type;
> > > + if (!strcmp(iface_rec->type, "geneve")) {
> > > + tunnel_type = GENEVE;
> > > + } else if (!strcmp(iface_rec->type, "stt")) {
> > > + tunnel_type = STT;
> > > + } else if (!strcmp(iface_rec->type, "vxlan")) {
> > > + tunnel_type = VXLAN;
> > > + } else {
> > > + continue;
> > > + }
> > > +
> > > + /* We split the tunnel_id to get the chassis-id
> > > + * and hash the tunnel list on the chassis-id. The
> > > + * reason to use the chassis-id alone is because
> > > + * there might be cases (multicast, gateway chassis)
> > > + * where we need to tunnel to the chassis, but won't
> > > + * have the encap-ip specifically.
> > > + */
> > > + char *hash_id = NULL;
> > > + char *ip = NULL;
> > > +
> > > + if (!encaps_tunnel_id_parse(tunnel_id, &hash_id,
&ip)) {
> > > + continue;
> > > + }
> > > + struct chassis_tunnel *tun = xmalloc(sizeof *tun);
> > > + hmap_insert(&nonvif_data->chassis_tunnels,
> > &tun->hmap_node,
> > > + hash_string(hash_id, 0));
> > > + tun->chassis_id = xstrdup(tunnel_id);
> > > + tun->ofport = u16_to_ofp(ofport);
> > > + tun->type = tunnel_type;
> > > +
> > > + free(hash_id);
> > > + free(ip);
> > > + break;
> > > + }
> > > + }
> > > + }
> > > +}
> > > +
> > > +bool
> > > +ldata_handle_ovs_iface_changes(
> > > + const struct ovsrec_interface_table *iface_table)
> > > +{
> > > + const struct ovsrec_interface *iface_rec;
> > > + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, iface_table)
{
> > > + if (!strcmp(iface_rec->type, "geneve") ||
> > > + !strcmp(iface_rec->type, "patch") ||
> > > + !strcmp(iface_rec->type, "vxlan") ||
> > > + !strcmp(iface_rec->type, "stt")) {
> > > + return false;
> > > + }
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +bool
> > > +get_chassis_tunnel_ofport(const struct hmap *chassis_tunnels,
> > > + const char *chassis_name, char *encap_ip,
> > > + ofp_port_t *ofport)
> > > +{
> > > + struct chassis_tunnel *tun = NULL;
> > > + tun = chassis_tunnel_find(chassis_tunnels, chassis_name,
encap_ip);
> > > + if (!tun) {
> > > + return false;
> > > + }
> > > +
> > > + *ofport = tun->ofport;
> > > + return true;
> > > +}
> > > +
> > > +void
> > > +local_nonvif_data_init(struct local_nonvif_data *nonvif_data)
> > > +{
> > > + simap_init(&nonvif_data->patch_ofports);
> > > + hmap_init(&nonvif_data->chassis_tunnels);
> > > +}
> > > +
> > > +void
> > > +local_nonvif_data_destroy(struct local_nonvif_data *nonvif_data)
> > > +{
> > > + simap_destroy(&nonvif_data->patch_ofports);
> > > + struct chassis_tunnel *tun;
> > > + HMAP_FOR_EACH_POP (tun, hmap_node,
&nonvif_data->chassis_tunnels) {
> > > + free(tun->chassis_id);
> > > + free(tun);
> > > + }
> > > + hmap_destroy(&nonvif_data->chassis_tunnels);
> > > +}
> > > +
> > > +
> > > +/*
> > > + * This function looks up the list of tunnel ports (provided by
> > > + * ovn-chassis-id ports) and returns the tunnel for the given
chassid-id
> > and
> > > + * encap-ip. The ovn-chassis-id is formed using the chassis-id and
> > encap-ip.
> > > + * The list is hashed using the chassis-id. If the encap-ip is not
> > specified,
> > > + * it means we'll just return a tunnel for that chassis-id, i.e. we
just
> > check
> > > + * for chassis-id and if there is a match, we'll return the tunnel.
> > > + * If encap-ip is also provided we use both chassis-id and encap-ip
to do
> > > + * a more specific lookup.
> > > + */
> > > +struct chassis_tunnel *
> > > +chassis_tunnel_find(const struct hmap *chassis_tunnels, const char
> > *chassis_id,
> > > + char *encap_ip)
> > > +{
> > > + /*
> > > + * If the specific encap_ip is given, look for the chassisid_ip
> > entry,
> > > + * else return the 1st found entry for the chassis.
> > > + */
> > > + struct chassis_tunnel *tun = NULL;
> > > + HMAP_FOR_EACH_WITH_HASH (tun, hmap_node, hash_string(chassis_id,
0),
> > > + chassis_tunnels) {
> > > + if (encaps_tunnel_id_match(tun->chassis_id, chassis_id,
> > encap_ip)) {
> > > + return tun;
> > > + }
> > > + }
> > > + return NULL;
> > > +}
> > > +
> > > /* static functions. */
> > > static struct local_datapath *
> > > local_datapath_add__(struct hmap *local_datapaths,
> > > diff --git a/controller/ldata.h b/controller/ldata.h
> > > index 16ad43c8f..91624d0b4 100644
> > > --- a/controller/ldata.h
> > > +++ b/controller/ldata.h
> > > @@ -19,10 +19,14 @@
> > > /* OVS includes. */
> > > #include "include/openvswitch/shash.h"
> > > #include "lib/smap.h"
> > > +#include "lib/simap.h"
> > >
> > > struct sbrec_datapath_binding;
> > > struct sbrec_port_binding;
> > > +struct sbrec_chassis;
> > > struct ovsdb_idl_index;
> > > +struct ovsrec_bridge;
> > > +struct ovsrec_interface_table;
> > >
> > > /* A logical datapath that has some relevance to this hypervisor. A
> > logical
> > > * datapath D is relevant to hypervisor H if:
> > > @@ -117,4 +121,44 @@ void tracked_datapath_lport_add(const struct
> > sbrec_port_binding *,
> > > struct hmap *tracked_datapaths);
> > > void tracked_datapaths_destroy(struct hmap *tracked_datapaths);
> > >
> > > +/* Must be a bit-field ordered from most-preferred (higher number) to
> > > + * least-preferred (lower number). */
> > > +enum chassis_tunnel_type {
> > > + GENEVE = 1 << 2,
> > > + STT = 1 << 1,
> > > + VXLAN = 1 << 0
> > > +};
> > > +
> > > +/* Maps from a chassis to the OpenFlow port number of the tunnel that
> > can be
> > > + * used to reach that chassis. */
> > > +struct chassis_tunnel {
> > > + struct hmap_node hmap_node;
> > > + char *chassis_id;
> > > + ofp_port_t ofport;
> > > + enum chassis_tunnel_type type;
> > > +};
> > > +
> > > +struct local_nonvif_data {
> > > + struct simap patch_ofports; /* simap of patch ovs ports. */
> > > + struct hmap chassis_tunnels; /* hmap of 'struct chassis_tunnel'
from
> > the
> > > + * tunnel OVS ports. */
> > > +};
> > > +
> > > +void ldata_run(const struct ovsrec_bridge *br_int,
> > > + const struct sbrec_chassis *,
> > > + struct local_nonvif_data *nonvif_data);
> > > +
> > > +bool ldata_handle_ovs_iface_changes(const struct
ovsrec_interface_table
> > *);
> > > +
> > > +struct chassis_tunnel *chassis_tunnel_find(const struct hmap
> > *chassis_tunnels,
> > > + const char *chassis_id,
> > > + char *encap_ip);
> > > +
> > > +bool get_chassis_tunnel_ofport(const struct hmap *chassis_tunnels,
> > > + const char *chassis_name, char
*encap_ip,
> > > + ofp_port_t *ofport);
> > > +
> > > +void local_nonvif_data_init(struct local_nonvif_data *nonvif_data);
> > > +void local_nonvif_data_destroy(struct local_nonvif_data
*nonvif_data);
> > > +
> > > #endif /* controller/ldata.h */
> > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > index d1f32077b..4ac671e40 100644
> > > --- a/controller/lflow.c
> > > +++ b/controller/lflow.c
> > > @@ -58,6 +58,7 @@ struct lookup_port_aux {
> > > const struct sbrec_datapath_binding *dp;
> > > const struct sbrec_logical_flow *lflow;
> > > struct lflow_resource_ref *lfrr;
> > > + const struct hmap *chassis_tunnels;
> > > };
> > >
> > > struct condition_aux {
> > > @@ -145,7 +146,8 @@ tunnel_ofport_cb(const void *aux_, const char
> > *port_name, ofp_port_t *ofport)
> > > return false;
> > > }
> > >
> > > - if (!get_tunnel_ofport(pb->chassis->name, NULL, ofport)) {
> > > + if (!get_chassis_tunnel_ofport(aux->chassis_tunnels,
> > pb->chassis->name,
> > > + NULL, ofport)) {
> > > return false;
> > > }
> > >
> > > @@ -591,6 +593,7 @@ add_matches_to_flow_table(const struct
> > sbrec_logical_flow *lflow,
> > > .dp = dp,
> > > .lflow = lflow,
> > > .lfrr = l_ctx_out->lfrr,
> > > + .chassis_tunnels = l_ctx_in->chassis_tunnels,
> > > };
> > >
> > > /* Encode OVN logical actions into OpenFlow. */
> > > diff --git a/controller/lflow.h b/controller/lflow.h
> > > index c17ff6dd4..e7dd31289 100644
> > > --- a/controller/lflow.h
> > > +++ b/controller/lflow.h
> > > @@ -146,6 +146,7 @@ struct lflow_ctx_in {
> > > const struct shash *port_groups;
> > > const struct sset *active_tunnels;
> > > const struct sset *related_lport_ids;
> > > + const struct hmap *chassis_tunnels;
> > > };
> > >
> > > struct lflow_ctx_out {
> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > index 8f620e4ad..69d135046 100644
> > > --- a/controller/ovn-controller.c
> > > +++ b/controller/ovn-controller.c
> > > @@ -1039,6 +1039,11 @@ struct ed_type_runtime_data {
> > > struct related_lports related_lports;
> > > struct sset active_tunnels;
> > >
> > > + /* Non VIF OVS interface information (mainly patch OVS interfacs
> > > + * and tunnel interfaces) that are relevant to the local
> > > + * chassis (generated by ldata_run()). */
> > > + struct local_nonvif_data nonvif_data;
> > > +
> > I don't see a benefit of adding this to runtime_data. It is unrelated to
> > the other data in the current runtime_data node. Now if we combine it to
> > runtime_data, it means we are coupling those data unnecessarily, which
> > leads to unflexible handler implementation. An example is that now
> > recompute of nonvif_data would trigger recompute of runtime_data, which
> > would trigger recompute of lflow_output. In fact, this can be just in a
> > separate engine node, only as input to pflow_output. Would that be more
> > straightforward and efficient?
>
> Hi Han,
>
> Thanks for the review comments.
>
> I see your point. I'll explore if we can add a new engine node for this.
>
> My understanding of engine processing is this and runtime data engine
> in particular is this:
>
> 1. The first level of engine nodes would handle the db changes and
> generates/transforms the data
> which we can probably call - runtime_data. This was the
> motivation for me to add the nonvif_data
> in the runtime data engine. Perhaps we need to further divide
> the runtime_data engine or rename it
> so that we only store the VIF related data in this.
Hi Numan, it seems we had different models for I-P engine nodes in our
minds. Sorry that I didn't document these thoughts initially for the I-P
engine. I don't think we should restrict the engine nodes in such layers.
It should be flexible enough based on the real computation dependencies -
just thinking about how DDlog programs would be written. I am not against
the idea of a separate encapsulation layer for OVSDB IDL, but runtime_data
was not for that purpose. Initially the I-P engine was very coarse grained,
so I just created a big "runtime_data" node trying to contain all the
intermediate data, just enough to support the first version of I-P for the
most frequent changes. Now that we are making the I-P engine more and more
fine-grained, we definitely cannot simply put everything in a single node.
The current runtime_data may be renamed to binding_data, or be further
split if necessary, and we don't need to put more data into it unless it is
tightly related to the bindings handling.
> 2. And the flow output engines ideally would use the runtime data for
> flow generation.
The output nodes should only depend on whatever data that is necessary for
flow computation. What you said is true if "runtime data" here contains
everything that is needed for flow computation, but it should never be a
goal to keep all these data in a single I-P engine node, because that would
not really benefit from the I-P engine.
> 3. Ideally I'd think (or prefer) to engine nodes one or two level
> down to not access any db changes at all.
> For example, lfow_output or pflow_output engine handles should
> not call IDL TRACKED loops.
>
> Maybe we could try to achieve follow up patches if that makes sense.
>
Encapsulating IDL to a separate layer may be reasonable, if it provides
values, but I think it is another topic. I don't think it changes the I-P
model.
Thanks,
Han
>
>
> >
> > As to the code organization, I think now I understand why you added the
new
> > module ldata.c (suppose it is going to be renamed to local_data.c to be
> > more clear). Although it is not harmful to have that new module, it
seems
> > not quite necessary to me, either. The original localvif_to_ofport data
is
> > now split into two parts, VIF related and nonVIF related. I don't see
> > benefit of keeping these two parts in the same file. It is reasonable to
> > combine the VIF related data in runtime_data (which could be renamed,
> > because the name was vague), to avoid maintaining redundant data. I
think
> > it is ok to stay in binding.c. For the nonVIF part, I think the new
engine
> > node can be defined in ovn-controller.c and implemented in physical.c
> > because that data is used by physical flow processing only. I don't
have a
> > strong argument for this - maybe just less change is needed for this
series
> > (and so the change history would be easier to follow).
>
> I'll explore and see if this can be separated. Although I think
> having a separate file local_data.c would uncomplicate the binding.c a
> bit.
>
>
> >
> >
> > > /* runtime data engine private data. */
> > > struct sset egress_ifaces;
> > > struct smap local_iface_ids;
> > > @@ -1139,6 +1144,7 @@ en_runtime_data_init(struct engine_node *node
> > OVS_UNUSED,
> > > local_binding_data_init(&data->lbinding_data);
> > > shash_init(&data->local_active_ports_ipv6_pd);
> > > shash_init(&data->local_active_ports_ras);
> > > + local_nonvif_data_init(&data->nonvif_data);
> > >
> > > /* Init the tracked data. */
> > > hmap_init(&data->tracked_dp_bindings);
> > > @@ -1160,6 +1166,7 @@ en_runtime_data_cleanup(void *data)
> > > shash_destroy_free_data(&rt_data->local_active_ports_ipv6_pd);
> > > shash_destroy_free_data(&rt_data->local_active_ports_ras);
> > > local_binding_data_destroy(&rt_data->lbinding_data);
> > > + local_nonvif_data_destroy(&rt_data->nonvif_data);
> > > }
> > >
> > > static void
> > > @@ -1272,6 +1279,7 @@ en_runtime_data_run(struct engine_node *node,
void
> > *data)
> > > shash_clear_free_data(local_active_ipv6_pd);
> > > shash_clear_free_data(local_active_ras);
> > > local_binding_data_destroy(&rt_data->lbinding_data);
> > > + local_nonvif_data_destroy(&rt_data->nonvif_data);
> > > sset_destroy(local_lports);
> > > related_lports_destroy(&rt_data->related_lports);
> > > sset_destroy(active_tunnels);
> > > @@ -1284,6 +1292,7 @@ en_runtime_data_run(struct engine_node *node,
void
> > *data)
> > > sset_init(&rt_data->egress_ifaces);
> > > smap_init(&rt_data->local_iface_ids);
> > > local_binding_data_init(&rt_data->lbinding_data);
> > > + local_nonvif_data_init(&rt_data->nonvif_data);
> > > }
> > >
> > > struct binding_ctx_in b_ctx_in;
> > > @@ -1303,6 +1312,7 @@ en_runtime_data_run(struct engine_node *node,
void
> > *data)
> > > bfd_calculate_active_tunnels(b_ctx_in.br_int,
active_tunnels);
> > > }
> > >
> > > + ldata_run(b_ctx_in.br_int, b_ctx_in.chassis_rec,
> > &rt_data->nonvif_data);
> > > binding_run(&b_ctx_in, &b_ctx_out);
> > >
> > > engine_set_node_state(node, EN_UPDATED);
> > > @@ -1318,6 +1328,10 @@ runtime_data_ovs_interface_handler(struct
> > engine_node *node, void *data)
> > > rt_data->tracked = true;
> > > b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> > >
> > > + if (!ldata_handle_ovs_iface_changes(b_ctx_in.iface_table)) {
> > > + return false;
> > > + }
> > > +
> >
> > This is the performance problem I mentioned above. Before this change,
> > tunnel/patch interface changes only triggered physical_run(), now it
would
> > trigger lflow_run().
>
> Actually that's not the case. Any change to tunnel/patch OVS interfaces
results
> in the function binding_handle_ovs_interface_changes() to return false
and hence
> lflow_run() would be called -
> https://github.com/ovn-org/ovn/blob/master/controller/binding.c#L2198
>
>
>
> >
> > > if (!binding_handle_ovs_interface_changes(&b_ctx_in,
&b_ctx_out)) {
> > > return false;
> > > }
> > > @@ -2060,6 +2074,7 @@ init_lflow_ctx(struct engine_node *node,
> > > l_ctx_in->port_groups = port_groups;
> > > l_ctx_in->active_tunnels = &rt_data->active_tunnels;
> > > l_ctx_in->related_lport_ids = &rt_data->related_lports.lport_ids;
> > > + l_ctx_in->chassis_tunnels =
&rt_data->nonvif_data.chassis_tunnels;
> > >
> > > l_ctx_out->flow_table = &fo->flow_table;
> > > l_ctx_out->group_table = &fo->group_table;
> > > @@ -2527,6 +2542,7 @@ static void init_physical_ctx(struct engine_node
> > *node,
> > > p_ctx->ct_zones = ct_zones;
> > > p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve;
> > > p_ctx->local_bindings = &rt_data->lbinding_data.bindings;
> > > + p_ctx->nonvif_data = &rt_data->nonvif_data;
> > > }
> > >
> > > static void *
> > > @@ -2583,7 +2599,11 @@ pflow_output_sb_port_binding_handler(struct
> > engine_node *node,
> > > * only. flow_output runtime data handler takes care of
processing
> > > * logical flows for any port binding changes.
> > > */
> > > - physical_handle_port_binding_changes(&p_ctx, &pfo->flow_table);
> > > + const struct sbrec_port_binding *pb;
> > > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > p_ctx.port_binding_table) {
> > > + bool removed = sbrec_port_binding_is_deleted(pb);
> > > + physical_handle_flows_for_lport(pb, removed, &p_ctx,
> > &pfo->flow_table);
> > > + }
> >
> > nit: I think it is better to keep the interface
> > physical_handle_port_binding_changes() and hide this loop details in the
> > physical.c module.
>
> Ack.
>
> Thanks
> Numan
>
> >
> > Thanks,
> > Han
> >
> > >
> > > engine_set_node_state(node, EN_UPDATED);
> > > return true;
> > > @@ -2613,13 +2633,55 @@ pflow_output_ovs_iface_handler(struct
engine_node
> > *node OVS_UNUSED,
> > > struct ed_type_runtime_data *rt_data =
> > > engine_get_input_data("runtime_data", node);
> > >
> > > + struct physical_ctx p_ctx;
> > > + init_physical_ctx(node, rt_data, &p_ctx);
> > > +
> > > + engine_set_node_state(node, EN_UPDATED);
> > > + return physical_handle_ovs_iface_changes(&p_ctx);
> > > +}
> > > +
> > > +static bool
> > > +pflow_output_runtime_data_handler(struct engine_node *node, void
*data)
> > > +{
> > > + struct ed_type_runtime_data *rt_data =
> > > + engine_get_input_data("runtime_data", node);
> > > +
> > > + /* There is no tracked data. Fall back to full recompute of
> > > + * pflow_output. */
> > > + if (!rt_data->tracked) {
> > > + return false;
> > > + }
> > > +
> > > + struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> > > + if (hmap_is_empty(tracked_dp_bindings)) {
> > > + return true;
> > > + }
> > > +
> > > struct ed_type_pflow_output *pfo = data;
> > >
> > > struct physical_ctx p_ctx;
> > > init_physical_ctx(node, rt_data, &p_ctx);
> > >
> > > + struct tracked_datapath *tdp;
> > > + HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
> > > + if (tdp->tracked_type != TRACKED_RESOURCE_UPDATED) {
> > > + /* Fall back to full recompute when a local datapath
> > > + * is added or deleted. */
> > > + return false;
> > > + }
> > > +
> > > + struct shash_node *shash_node;
> > > + SHASH_FOR_EACH (shash_node, &tdp->lports) {
> > > + struct tracked_lport *lport = shash_node->data;
> > > + bool removed =
> > > + lport->tracked_type == TRACKED_RESOURCE_REMOVED ?
true:
> > false;
> > > + physical_handle_flows_for_lport(lport->pb, removed,
&p_ctx,
> > > + &pfo->flow_table);
> > > + }
> > > + }
> > > +
> > > engine_set_node_state(node, EN_UPDATED);
> > > - return physical_handle_ovs_iface_changes(&p_ctx,
&pfo->flow_table);
> > > + return true;
> > > }
> > >
> > > static void *
> > > @@ -2914,7 +2976,7 @@ main(int argc, char *argv[])
> > > pflow_output_sb_multicast_group_handler);
> > >
> > > engine_add_input(&en_pflow_output, &en_runtime_data,
> > > - NULL);
> > > + pflow_output_runtime_data_handler);
> > > engine_add_input(&en_pflow_output, &en_sb_encap, NULL);
> > > engine_add_input(&en_pflow_output, &en_mff_ovn_geneve, NULL);
> > > engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL);
> > > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
> > > index 578588305..78a53312f 100644
> > > --- a/controller/ovn-controller.h
> > > +++ b/controller/ovn-controller.h
> > > @@ -45,14 +45,6 @@ const struct ovsrec_bridge *get_bridge(const struct
> > ovsrec_bridge_table *,
> > >
> > > struct sbrec_encap *preferred_encap(const struct sbrec_chassis *);
> > >
> > > -/* Must be a bit-field ordered from most-preferred (higher number) to
> > > - * least-preferred (lower number). */
> > > -enum chassis_tunnel_type {
> > > - GENEVE = 1 << 2,
> > > - STT = 1 << 1,
> > > - VXLAN = 1 << 0
> > > -};
> > > -
> > > uint32_t get_tunnel_type(const char *name);
> > >
> > > struct pb_ld_binding {
> > > diff --git a/controller/physical.c b/controller/physical.c
> > > index b244ff1c2..87080d001 100644
> > > --- a/controller/physical.c
> > > +++ b/controller/physical.c
> > > @@ -86,46 +86,6 @@ physical_register_ovs_idl(struct ovsdb_idl
*ovs_idl)
> > > ovsdb_idl_track_add_column(ovs_idl,
> > &ovsrec_interface_col_external_ids);
> > > }
> > >
> > > -static struct simap localvif_to_ofport =
> > > - SIMAP_INITIALIZER(&localvif_to_ofport);
> > > -static struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
> > > -
> > > -/* Maps from a chassis to the OpenFlow port number of the tunnel that
> > can be
> > > - * used to reach that chassis. */
> > > -struct chassis_tunnel {
> > > - struct hmap_node hmap_node;
> > > - char *chassis_id;
> > > - ofp_port_t ofport;
> > > - enum chassis_tunnel_type type;
> > > -};
> > > -
> > > -/*
> > > - * This function looks up the list of tunnel ports (provided by
> > > - * ovn-chassis-id ports) and returns the tunnel for the given
chassid-id
> > and
> > > - * encap-ip. The ovn-chassis-id is formed using the chassis-id and
> > encap-ip.
> > > - * The list is hashed using the chassis-id. If the encap-ip is not
> > specified,
> > > - * it means we'll just return a tunnel for that chassis-id, i.e. we
just
> > check
> > > - * for chassis-id and if there is a match, we'll return the tunnel.
> > > - * If encap-ip is also provided we use both chassis-id and encap-ip
to do
> > > - * a more specific lookup.
> > > - */
> > > -static struct chassis_tunnel *
> > > -chassis_tunnel_find(const char *chassis_id, char *encap_ip)
> > > -{
> > > - /*
> > > - * If the specific encap_ip is given, look for the chassisid_ip
> > entry,
> > > - * else return the 1st found entry for the chassis.
> > > - */
> > > - struct chassis_tunnel *tun = NULL;
> > > - HMAP_FOR_EACH_WITH_HASH (tun, hmap_node, hash_string(chassis_id,
0),
> > > - &tunnels) {
> > > - if (encaps_tunnel_id_match(tun->chassis_id, chassis_id,
> > encap_ip)) {
> > > - return tun;
> > > - }
> > > - }
> > > - return NULL;
> > > -}
> > > -
> > > static void
> > > put_load(uint64_t value, enum mf_field_id dst, int ofs, int n_bits,
> > > struct ofpbuf *ofpacts)
> > > @@ -166,17 +126,18 @@ put_resubmit(uint8_t table_id, struct ofpbuf
> > *ofpacts)
> > > * from the associated encap.
> > > */
> > > static struct chassis_tunnel *
> > > -get_port_binding_tun(const struct sbrec_port_binding *binding)
> > > +get_port_binding_tun(const struct sbrec_port_binding *binding,
> > > + const struct hmap *chassis_tunnels)
> > > {
> > > struct sbrec_encap *encap = binding->encap;
> > > struct sbrec_chassis *chassis = binding->chassis;
> > > struct chassis_tunnel *tun = NULL;
> > >
> > > if (encap) {
> > > - tun = chassis_tunnel_find(chassis->name, encap->ip);
> > > + tun = chassis_tunnel_find(chassis_tunnels, chassis->name,
> > encap->ip);
> > > }
> > > if (!tun) {
> > > - tun = chassis_tunnel_find(chassis->name, NULL);
> > > + tun = chassis_tunnel_find(chassis_tunnels, chassis->name,
NULL);
> > > }
> > > return tun;
> > > }
> > > @@ -325,12 +286,13 @@ put_remote_port_redirect_overlay(const struct
> > > uint32_t port_key,
> > > struct match *match,
> > > struct ofpbuf *ofpacts_p,
> > > + const struct hmap *chassis_tunnels,
> > > struct ovn_desired_flow_table
> > *flow_table)
> > > {
> > > if (!is_ha_remote) {
> > > /* Setup encapsulation */
> > > const struct chassis_tunnel *rem_tun =
> > > - get_port_binding_tun(binding);
> > > + get_port_binding_tun(binding, chassis_tunnels);
> > > if (!rem_tun) {
> > > return;
> > > }
> > > @@ -348,10 +310,10 @@ put_remote_port_redirect_overlay(const struct
> > > continue;
> > > }
> > > if (!tun) {
> > > - tun = chassis_tunnel_find(ch->name, NULL);
> > > + tun = chassis_tunnel_find(chassis_tunnels, ch->name,
> > NULL);
> > > } else {
> > > struct chassis_tunnel *chassis_tunnel =
> > > - chassis_tunnel_find(ch->name,
> > NULL);
> > > + chassis_tunnel_find(chassis_tunnels, ch->name,
NULL);
> > > if (chassis_tunnel &&
> > > tun->type != chassis_tunnel->type) {
> > > static struct vlog_rate_limit rl =
> > > @@ -385,7 +347,7 @@ put_remote_port_redirect_overlay(const struct
> > > if (!ch) {
> > > continue;
> > > }
> > > - tun = chassis_tunnel_find(ch->name, NULL);
> > > + tun = chassis_tunnel_find(chassis_tunnels, ch->name,
NULL);
> > > if (!tun) {
> > > continue;
> > > }
> > > @@ -925,6 +887,9 @@ consider_port_binding(struct ovsdb_idl_index
> > *sbrec_port_binding_by_name,
> > > const struct simap *ct_zones,
> > > const struct sset *active_tunnels,
> > > const struct hmap *local_datapaths,
> > > + const struct shash *local_bindings,
> > > + const struct simap *patch_ofports,
> > > + const struct hmap *chassis_tunnels,
> > > const struct sbrec_port_binding *binding,
> > > const struct sbrec_chassis *chassis,
> > > struct ovn_desired_flow_table *flow_table,
> > > @@ -1081,17 +1046,25 @@ consider_port_binding(struct ovsdb_idl_index
> > *sbrec_port_binding_by_name,
> > > if (!binding->tag) {
> > > goto out;
> > > }
> > > - ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
> > > - binding->parent_port));
> > > + ofport = local_binding_get_lport_ofport(local_bindings,
> > > +
binding->parent_port);
> > > if (ofport) {
> > > tag = *binding->tag;
> > > nested_container = true;
> > > parent_port = lport_lookup_by_name(
> > > sbrec_port_binding_by_name, binding->parent_port);
> > > }
> > > - } else {
> > > - ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
> > > + } else if (!strcmp(binding->type, "localnet")
> > > + || !strcmp(binding->type, "l2gateway")) {
> > > +
> > > + ofport = u16_to_ofp(simap_get(patch_ofports,
> > > binding->logical_port));
> > > + if (ofport && binding->tag) {
> > > + tag = *binding->tag;
> > > + }
> > > + } else {
> > > + ofport = local_binding_get_lport_ofport(local_bindings,
> > > +
binding->logical_port);
> > > const char *requested_chassis = smap_get(&binding->options,
> > >
"requested-chassis");
> > > if (ofport && requested_chassis && requested_chassis[0] &&
> > > @@ -1102,12 +1075,6 @@ consider_port_binding(struct ovsdb_idl_index
> > *sbrec_port_binding_by_name,
> > > */
> > > ofport = 0;
> > > }
> > > -
> > > - if ((!strcmp(binding->type, "localnet")
> > > - || !strcmp(binding->type, "l2gateway"))
> > > - && ofport && binding->tag) {
> > > - tag = *binding->tag;
> > > - }
> > > }
> > >
> > > bool is_ha_remote = false;
> > > @@ -1118,7 +1085,7 @@ consider_port_binding(struct ovsdb_idl_index
> > *sbrec_port_binding_by_name,
> > > /* It is remote port, may be reached by tunnel or localnet
port
> > */
> > > is_remote = true;
> > > if (localnet_port) {
> > > - ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
> > > + ofport = u16_to_ofp(simap_get(patch_ofports,
> > >
localnet_port->logical_port));
> > > if (!ofport) {
> > > goto out;
> > > @@ -1129,7 +1096,8 @@ consider_port_binding(struct ovsdb_idl_index
> > *sbrec_port_binding_by_name,
> > > if (!binding->chassis) {
> > > goto out;
> > > }
> > > - tun = chassis_tunnel_find(binding->chassis->name,
NULL);
> > > + tun = chassis_tunnel_find(chassis_tunnels,
> > > + binding->chassis->name,
NULL);
> > > if (!tun) {
> > > goto out;
> > > }
> > > @@ -1383,7 +1351,7 @@ consider_port_binding(struct ovsdb_idl_index
> > *sbrec_port_binding_by_name,
> > > put_remote_port_redirect_overlay(binding, is_ha_remote,
> > > ha_ch_ordered,
> > mff_ovn_geneve,
> > > tun, port_key, &match,
> > ofpacts_p,
> > > - flow_table);
> > > + chassis_tunnels,
> > flow_table);
> > > }
> > > }
> > > out:
> > > @@ -1396,8 +1364,11 @@ static void
> > > consider_mc_group(enum mf_field_id mff_ovn_geneve,
> > > const struct simap *ct_zones,
> > > const struct hmap *local_datapaths,
> > > + struct shash *local_bindings,
> > > + struct simap *patch_ofports,
> > > const struct sbrec_chassis *chassis,
> > > const struct sbrec_multicast_group *mc,
> > > + const struct hmap *chassis_tunnels,
> > > struct ovn_desired_flow_table *flow_table)
> > > {
> > > uint32_t dp_key = mc->datapath->tunnel_key;
> > > @@ -1444,19 +1415,21 @@ consider_mc_group(enum mf_field_id
mff_ovn_geneve,
> > > put_load(zone_id, MFF_LOG_CT_ZONE, 0, 32, &ofpacts);
> > > }
> > >
> > > + const char *lport_name = (port->parent_port &&
> > *port->parent_port) ?
> > > + port->parent_port :
port->logical_port;
> > > +
> > > if (!strcmp(port->type, "patch")) {
> > > put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32,
> > > &remote_ofpacts);
> > > put_resubmit(OFTABLE_CHECK_LOOPBACK, &remote_ofpacts);
> > > - } else if (simap_contains(&localvif_to_ofport,
> > > - (port->parent_port && *port->parent_port)
> > > - ? port->parent_port : port->logical_port)
> > > + } else if (local_binding_get_primary_pb(local_bindings,
> > lport_name)
> > > + || simap_contains(patch_ofports,
port->logical_port)
> > > || (!strcmp(port->type, "l3gateway")
> > > && port->chassis == chassis)) {
> > > put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32,
&ofpacts);
> > > put_resubmit(OFTABLE_CHECK_LOOPBACK, &ofpacts);
> > > - } else if (port->chassis &&
!get_localnet_port(local_datapaths,
> > > - mc->datapath->tunnel_key)) {
> > > + } else if (port->chassis && !get_localnet_port(
> > > + local_datapaths, mc->datapath->tunnel_key)) {
> > > /* Add remote chassis only when localnet port not exist,
> > > * otherwise multicast will reach remote ports through
> > localnet
> > > * port. */
> > > @@ -1497,7 +1470,7 @@ consider_mc_group(enum mf_field_id
mff_ovn_geneve,
> > > const struct chassis_tunnel *prev = NULL;
> > > SSET_FOR_EACH (chassis_name, &remote_chassis) {
> > > const struct chassis_tunnel *tun
> > > - = chassis_tunnel_find(chassis_name, NULL);
> > > + = chassis_tunnel_find(chassis_tunnels, chassis_name,
> > NULL);
> > > if (!tun) {
> > > continue;
> > > }
> > > @@ -1524,41 +1497,50 @@ consider_mc_group(enum mf_field_id
mff_ovn_geneve,
> > > sset_destroy(&remote_chassis);
> > > }
> > >
> > > -/* Replaces 'old' by 'new' (destroying 'new'). Returns true if 'old'
> > and 'new'
> > > - * contained different data, false if they were the same. */
> > > -static bool
> > > -update_ofports(struct simap *old, struct simap *new)
> > > -{
> > > - bool changed = !simap_equal(old, new);
> > > - simap_swap(old, new);
> > > - simap_destroy(new);
> > > - return changed;
> > > -}
> > > -
> > > void
> > > -physical_handle_port_binding_changes(struct physical_ctx *p_ctx,
> > > - struct ovn_desired_flow_table
> > *flow_table)
> > > +physical_handle_flows_for_lport(const struct sbrec_port_binding *pb,
> > > + bool removed, struct physical_ctx
*p_ctx,
> > > + struct ovn_desired_flow_table
> > *flow_table)
> > > {
> > > - const struct sbrec_port_binding *binding;
> > > - struct ofpbuf ofpacts;
> > > - ofpbuf_init(&ofpacts, 0);
> > > - SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding,
> > > -
> > p_ctx->port_binding_table) {
> > > - if (sbrec_port_binding_is_deleted(binding)) {
> > > - ofctrl_remove_flows(flow_table, &binding->header_.uuid);
> > > - } else {
> > > - if (!sbrec_port_binding_is_new(binding)) {
> > > - ofctrl_remove_flows(flow_table,
&binding->header_.uuid);
> > > - }
> > > + ofctrl_remove_flows(flow_table, &pb->header_.uuid);
> > > +
> > > + if (!strcmp(pb->type, "external")) {
> > > + /* External lports have a dependency on the localnet port.
> > > + * We need to remove the flows of the localnet port as well
> > > + * and re-consider adding the flows for it.
> > > + */
> > > + struct local_datapath *ldp =
> > > + get_local_datapath(p_ctx->local_datapaths,
> > > + pb->datapath->tunnel_key);
> > > + if (ldp && ldp->localnet_port) {
> > > + struct ofpbuf ofpacts;
> > > + ofctrl_remove_flows(flow_table,
> > &ldp->localnet_port->header_.uuid);
> > > + ofpbuf_init(&ofpacts, 0);
> > > consider_port_binding(p_ctx->sbrec_port_binding_by_name,
> > > p_ctx->mff_ovn_geneve,
p_ctx->ct_zones,
> > > p_ctx->active_tunnels,
> > > p_ctx->local_datapaths,
> > > - binding, p_ctx->chassis,
> > > + p_ctx->local_bindings,
> > > + &p_ctx->nonvif_data->patch_ofports,
> > > +
&p_ctx->nonvif_data->chassis_tunnels,
> > > + ldp->localnet_port, p_ctx->chassis,
> > > flow_table, &ofpacts);
> > > + ofpbuf_uninit(&ofpacts);
> > > }
> > > }
> > > - ofpbuf_uninit(&ofpacts);
> > > +
> > > + if (!removed) {
> > > + struct ofpbuf ofpacts;
> > > + ofpbuf_init(&ofpacts, 0);
> > > + consider_port_binding(p_ctx->sbrec_port_binding_by_name,
> > > + p_ctx->mff_ovn_geneve, p_ctx->ct_zones,
> > > + p_ctx->active_tunnels,
> > p_ctx->local_datapaths,
> > > + p_ctx->local_bindings,
> > > + &p_ctx->nonvif_data->patch_ofports,
> > > + &p_ctx->nonvif_data->chassis_tunnels,
pb,
> > > + p_ctx->chassis, flow_table, &ofpacts);
> > > + ofpbuf_uninit(&ofpacts);
> > > + }
> > > }
> > >
> > > void
> > > @@ -1574,8 +1556,11 @@ physical_handle_mc_group_changes(struct
> > physical_ctx *p_ctx,
> > > ofctrl_remove_flows(flow_table, &mc->header_.uuid);
> > > }
> > > consider_mc_group(p_ctx->mff_ovn_geneve, p_ctx->ct_zones,
> > > - p_ctx->local_datapaths,
> > > - p_ctx->chassis, mc, flow_table);
> > > + p_ctx->local_datapaths,
> > p_ctx->local_bindings,
> > > + &p_ctx->nonvif_data->patch_ofports,
> > > + p_ctx->chassis, mc,
> > > + &p_ctx->nonvif_data->chassis_tunnels,
> > > + flow_table);
> > > }
> > > }
> > > }
> > > @@ -1591,135 +1576,6 @@ physical_run(struct physical_ctx *p_ctx,
> > > uuid_generate(hc_uuid);
> > > }
> > >
> > > - /* This bool tracks physical mapping changes. */
> > > - bool physical_map_changed = false;
> > > -
> > > - struct simap new_localvif_to_ofport =
> > > - SIMAP_INITIALIZER(&new_localvif_to_ofport);
> > > - struct simap new_tunnel_to_ofport =
> > > - SIMAP_INITIALIZER(&new_tunnel_to_ofport);
> > > - for (int i = 0; i < p_ctx->br_int->n_ports; i++) {
> > > - const struct ovsrec_port *port_rec = p_ctx->br_int->ports[i];
> > > - if (!strcmp(port_rec->name, p_ctx->br_int->name)) {
> > > - continue;
> > > - }
> > > -
> > > - const char *tunnel_id = smap_get(&port_rec->external_ids,
> > > - "ovn-chassis-id");
> > > - if (tunnel_id && encaps_tunnel_id_match(tunnel_id,
> > > - p_ctx->chassis->name,
> > > - NULL)) {
> > > - continue;
> > > - }
> > > -
> > > - const char *localnet = smap_get(&port_rec->external_ids,
> > > - "ovn-localnet-port");
> > > - const char *l2gateway = smap_get(&port_rec->external_ids,
> > > - "ovn-l2gateway-port");
> > > -
> > > - for (int j = 0; j < port_rec->n_interfaces; j++) {
> > > - const struct ovsrec_interface *iface_rec =
> > port_rec->interfaces[j];
> > > -
> > > - /* Get OpenFlow port number. */
> > > - if (!iface_rec->n_ofport) {
> > > - continue;
> > > - }
> > > - int64_t ofport = iface_rec->ofport[0];
> > > - if (ofport < 1 || ofport > ofp_to_u16(OFPP_MAX)) {
> > > - continue;
> > > - }
> > > -
> > > - /* Record as patch to local net, logical patch port,
> > chassis, or
> > > - * local logical port. */
> > > - bool is_patch = !strcmp(iface_rec->type, "patch");
> > > - if (is_patch && localnet) {
> > > - /* localnet patch ports can be handled just like
VIFs. */
> > > - simap_put(&new_localvif_to_ofport, localnet, ofport);
> > > - break;
> > > - } else if (is_patch && l2gateway) {
> > > - /* L2 gateway patch ports can be handled just like
VIFs.
> > */
> > > - simap_put(&new_localvif_to_ofport, l2gateway,
ofport);
> > > - break;
> > > - } else if (tunnel_id) {
> > > - enum chassis_tunnel_type tunnel_type;
> > > - if (!strcmp(iface_rec->type, "geneve")) {
> > > - tunnel_type = GENEVE;
> > > - if (!p_ctx->mff_ovn_geneve) {
> > > - continue;
> > > - }
> > > - } else if (!strcmp(iface_rec->type, "stt")) {
> > > - tunnel_type = STT;
> > > - } else if (!strcmp(iface_rec->type, "vxlan")) {
> > > - tunnel_type = VXLAN;
> > > - } else {
> > > - continue;
> > > - }
> > > -
> > > - simap_put(&new_tunnel_to_ofport, tunnel_id, ofport);
> > > - /*
> > > - * We split the tunnel_id to get the chassis-id
> > > - * and hash the tunnel list on the chassis-id. The
> > > - * reason to use the chassis-id alone is because
> > > - * there might be cases (multicast, gateway chassis)
> > > - * where we need to tunnel to the chassis, but won't
> > > - * have the encap-ip specifically.
> > > - */
> > > - char *hash_id = NULL;
> > > - char *ip = NULL;
> > > -
> > > - if (!encaps_tunnel_id_parse(tunnel_id, &hash_id,
&ip)) {
> > > - continue;
> > > - }
> > > - struct chassis_tunnel *tun =
> > chassis_tunnel_find(hash_id, ip);
> > > - if (tun) {
> > > - /* If the tunnel's ofport has changed, update. */
> > > - if (tun->ofport != u16_to_ofp(ofport) ||
> > > - tun->type != tunnel_type) {
> > > - tun->ofport = u16_to_ofp(ofport);
> > > - tun->type = tunnel_type;
> > > - physical_map_changed = true;
> > > - }
> > > - } else {
> > > - tun = xmalloc(sizeof *tun);
> > > - hmap_insert(&tunnels, &tun->hmap_node,
> > > - hash_string(hash_id, 0));
> > > - tun->chassis_id = xstrdup(tunnel_id);
> > > - tun->ofport = u16_to_ofp(ofport);
> > > - tun->type = tunnel_type;
> > > - physical_map_changed = true;
> > > - }
> > > - free(hash_id);
> > > - free(ip);
> > > - break;
> > > - } else {
> > > - const char *iface_id =
smap_get(&iface_rec->external_ids,
> > > - "iface-id");
> > > - if (iface_id) {
> > > - simap_put(&new_localvif_to_ofport, iface_id,
ofport);
> > > - }
> > > - }
> > > - }
> > > - }
> > > -
> > > - /* Remove tunnels that are no longer here. */
> > > - struct chassis_tunnel *tun, *tun_next;
> > > - HMAP_FOR_EACH_SAFE (tun, tun_next, hmap_node, &tunnels) {
> > > - if (!simap_find(&new_tunnel_to_ofport, tun->chassis_id)) {
> > > - hmap_remove(&tunnels, &tun->hmap_node);
> > > - physical_map_changed = true;
> > > - free(tun->chassis_id);
> > > - free(tun);
> > > - }
> > > - }
> > > -
> > > - /* Capture changed or removed openflow ports. */
> > > - physical_map_changed |= update_ofports(&localvif_to_ofport,
> > > - &new_localvif_to_ofport);
> > > - if (physical_map_changed) {
> > > - /* Reprocess logical flow table immediately. */
> > > - poll_immediate_wake();
> > > - }
> > > -
> > > struct ofpbuf ofpacts;
> > > ofpbuf_init(&ofpacts, 0);
> > >
> > > @@ -1733,16 +1589,20 @@ physical_run(struct physical_ctx *p_ctx,
> > > consider_port_binding(p_ctx->sbrec_port_binding_by_name,
> > > p_ctx->mff_ovn_geneve, p_ctx->ct_zones,
> > > p_ctx->active_tunnels,
> > p_ctx->local_datapaths,
> > > - binding, p_ctx->chassis,
> > > - flow_table, &ofpacts);
> > > + p_ctx->local_bindings,
> > > + &p_ctx->nonvif_data->patch_ofports,
> > > + &p_ctx->nonvif_data->chassis_tunnels,
> > binding,
> > > + p_ctx->chassis, flow_table, &ofpacts);
> > > }
> > >
> > > /* Handle output to multicast groups, in tables 32 and 33. */
> > > const struct sbrec_multicast_group *mc;
> > > SBREC_MULTICAST_GROUP_TABLE_FOR_EACH (mc, p_ctx->mc_group_table)
{
> > > consider_mc_group(p_ctx->mff_ovn_geneve, p_ctx->ct_zones,
> > > - p_ctx->local_datapaths, p_ctx->chassis,
> > > - mc, flow_table);
> > > + p_ctx->local_datapaths,
p_ctx->local_bindings,
> > > + &p_ctx->nonvif_data->patch_ofports,
> > p_ctx->chassis,
> > > + mc, &p_ctx->nonvif_data->chassis_tunnels,
> > > + flow_table);
> > > }
> > >
> > > /* Table 0, priority 100.
> > > @@ -1757,7 +1617,8 @@ physical_run(struct physical_ctx *p_ctx,
> > > * We set MFF_LOG_DATAPATH, MFF_LOG_INPORT, and MFF_LOG_OUTPORT
from
> > the
> > > * tunnel key data where possible, then resubmit to table 33 to
> > handle
> > > * packets to the local hypervisor. */
> > > - HMAP_FOR_EACH (tun, hmap_node, &tunnels) {
> > > + struct chassis_tunnel *tun;
> > > + HMAP_FOR_EACH (tun, hmap_node,
&p_ctx->nonvif_data->chassis_tunnels)
> > {
> > > struct match match = MATCH_CATCHALL_INITIALIZER;
> > > match_set_in_port(&match, tun->ofport);
> > >
> > > @@ -1788,7 +1649,7 @@ physical_run(struct physical_ctx *p_ctx,
> > > }
> > >
> > > /* Handle ramp switch encapsulations. */
> > > - HMAP_FOR_EACH (tun, hmap_node, &tunnels) {
> > > + HMAP_FOR_EACH (tun, hmap_node,
&p_ctx->nonvif_data->chassis_tunnels)
> > {
> > > if (tun->type != VXLAN) {
> > > continue;
> > > }
> > > @@ -1925,13 +1786,10 @@ physical_run(struct physical_ctx *p_ctx,
> > > &ofpacts, hc_uuid);
> > >
> > > ofpbuf_uninit(&ofpacts);
> > > -
> > > - simap_destroy(&new_tunnel_to_ofport);
> > > }
> > >
> > > bool
> > > -physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx,
> > > - struct ovn_desired_flow_table
> > *flow_table)
> > > +physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx)
> > > {
> > > const struct ovsrec_interface *iface_rec;
> > > OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
> > p_ctx->iface_table) {
> > > @@ -1943,64 +1801,6 @@ physical_handle_ovs_iface_changes(struct
> > physical_ctx *p_ctx,
> > > }
> > > }
> > >
> > > - struct ofpbuf ofpacts;
> > > - ofpbuf_init(&ofpacts, 0);
> > > -
> > > - OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
> > p_ctx->iface_table) {
> > > - const char *iface_id = smap_get(&iface_rec->external_ids,
> > "iface-id");
> > > - if (!iface_id) {
> > > - continue;
> > > - }
> > > -
> > > - const struct sbrec_port_binding *lb_pb =
> > > - local_binding_get_primary_pb(p_ctx->local_bindings,
> > iface_id);
> > > - if (!lb_pb) {
> > > - /* For regular VIFs (e.g. lsp) the upcoming port-binding
> > update
> > > - * will remove lfows related to the unclaimed ovs port.
> > > - * Localport is a special case and it needs to be managed
> > here
> > > - * since the port is not binded and otherwise the related
> > lfows
> > > - * will not be cleared removing the ovs port.
> > > - */
> > > - lb_pb =
> > lport_lookup_by_name(p_ctx->sbrec_port_binding_by_name,
> > > - iface_id);
> > > - if (!lb_pb || strcmp(lb_pb->type, "localport")) {
> > > - continue;
> > > - }
> > > - }
> > > -
> > > - int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport :
0;
> > > - if (ovsrec_interface_is_deleted(iface_rec)) {
> > > - ofctrl_remove_flows(flow_table, &lb_pb->header_.uuid);
> > > - simap_find_and_delete(&localvif_to_ofport, iface_id);
> > > - } else {
> > > - if (!ovsrec_interface_is_new(iface_rec)) {
> > > - ofctrl_remove_flows(flow_table,
&lb_pb->header_.uuid);
> > > - }
> > > -
> > > - simap_put(&localvif_to_ofport, iface_id, ofport);
> > > - consider_port_binding(p_ctx->sbrec_port_binding_by_name,
> > > - p_ctx->mff_ovn_geneve,
p_ctx->ct_zones,
> > > - p_ctx->active_tunnels,
> > > - p_ctx->local_datapaths,
> > > - lb_pb, p_ctx->chassis,
> > > - flow_table, &ofpacts);
> > > - }
> > > - }
> > > -
> > > - ofpbuf_uninit(&ofpacts);
> > > - return true;
> > > -}
> > > -
> > > -bool
> > > -get_tunnel_ofport(const char *chassis_name, char *encap_ip,
ofp_port_t
> > *ofport)
> > > -{
> > > - struct chassis_tunnel *tun = NULL;
> > > - tun = chassis_tunnel_find(chassis_name, encap_ip);
> > > - if (!tun) {
> > > - return false;
> > > - }
> > > -
> > > - *ofport = tun->ofport;
> > > return true;
> > > }
> > >
> > > diff --git a/controller/physical.h b/controller/physical.h
> > > index feab41df4..3fdc20005 100644
> > > --- a/controller/physical.h
> > > +++ b/controller/physical.h
> > > @@ -34,6 +34,7 @@ struct simap;
> > > struct sbrec_multicast_group_table;
> > > struct sbrec_port_binding_table;
> > > struct sset;
> > > +struct local_nonvif_data;
> > >
> > > /* OVN Geneve option information.
> > > *
> > > @@ -56,18 +57,18 @@ struct physical_ctx {
> > > const struct simap *ct_zones;
> > > enum mf_field_id mff_ovn_geneve;
> > > struct shash *local_bindings;
> > > + struct local_nonvif_data *nonvif_data;
> > > };
> > >
> > > void physical_register_ovs_idl(struct ovsdb_idl *);
> > > void physical_run(struct physical_ctx *,
> > > struct ovn_desired_flow_table *);
> > > void physical_clear_unassoc_flows_with_db(struct
ovn_desired_flow_table
> > *);
> > > -void physical_handle_port_binding_changes(struct physical_ctx *,
> > > - struct
ovn_desired_flow_table
> > *);
> > > void physical_handle_mc_group_changes(struct physical_ctx *,
> > > struct ovn_desired_flow_table
*);
> > > -bool physical_handle_ovs_iface_changes(struct physical_ctx *,
> > > - struct ovn_desired_flow_table
*);
> > > -bool get_tunnel_ofport(const char *chassis_name, char *encap_ip,
> > > - ofp_port_t *ofport);
> > > +bool physical_handle_ovs_iface_changes(struct physical_ctx *);
> > > +void physical_handle_flows_for_lport(const struct sbrec_port_binding
*,
> > > + bool removed,
> > > + struct physical_ctx *,
> > > + struct ovn_desired_flow_table
*);
> > > #endif /* controller/physical.h */
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
More information about the dev
mailing list