On Wed, Feb 23, 2022 at 8:22 PM Han Zhou <[email protected]> wrote:
>
> On Wed, Feb 23, 2022 at 3:24 PM Numan Siddique <[email protected]> wrote:
> >
> > On Tue, Feb 8, 2022 at 5:28 AM Dumitru Ceara <[email protected]> wrote:
> > >
> > > We already store whether a datapath is a switch or not. No need to do
> > > an smap lookup through its external IDs every time we need this
> > > information.
> > >
> > > Also, move the functions that inspect Datapath_Binding.external_ids
> > > out of ovn-util.c. It makes more sense to just define them where
> > > they're used.
> > >
> > > Signed-off-by: Dumitru Ceara <[email protected]>
> >
> > Thanks for the patch. I applied this patch to the main branch.
> >
> > Numan
> >
> Looks like when applying this patch a previous commit 385aedba7 was
> overwritten.
>
> I guess it's a problem of resolving conflict. Below is the actual diff seen:
>
> @@ -791,9 +792,11 @@ consider_logical_flow__(const struct
> sbrec_logical_flow *lflow,
> struct lflow_ctx_in *l_ctx_in,
> struct lflow_ctx_out *l_ctx_out)
> {
> - if (!get_local_datapath(l_ctx_in->local_datapaths, dp->tunnel_key)) {
> - VLOG_DBG("Skip lflow "UUID_FMT" for non-local datapath %"PRId64,
> - UUID_ARGS(&lflow->header_.uuid), dp->tunnel_key);
> + struct local_datapath *ldp =
> get_local_datapath(l_ctx_in->local_datapaths,
> + dp->tunnel_key);
> + if (!ldp) {
> + VLOG_DBG("lflow "UUID_FMT" is not for local datapath, skip",
> + UUID_ARGS(&lflow->header_.uuid));
> return;
> }
>
> I sent a patch to reapply the fix (but not shown up in patchwork yet when I
> send this email).
Sorry about that. Instead of asking Dumitru to rebase the patch, I
resolved the conflict myself.
And in doing so, I made this mistake. Thanks for sending the patch.
I've acked it.
Numan
>
> Thanks,
> Han
>
>
> > > ---
> > > v2:
> > > - Per Mark's comment: don't expose datapath_is_switch().
> > > - Also moved other similar functions.
> > > ---
> > > controller/lflow.c | 25 ++++++++++++++-----------
> > > controller/local_data.c | 15 +++++++++++++++
> > > controller/ovn-controller.c | 10 ++++++++--
> > > controller/pinctrl.c | 2 +-
> > > lib/ovn-util.c | 18 ------------------
> > > lib/ovn-util.h | 3 ---
> > > 6 files changed, 38 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > index b976c7d562..055c53a751 100644
> > > --- a/controller/lflow.c
> > > +++ b/controller/lflow.c
> > > @@ -622,7 +622,7 @@ lflow_parse_ctrl_meter(const struct
> sbrec_logical_flow *lflow,
> > >
> > > static void
> > > add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> > > - const struct sbrec_datapath_binding *dp,
> > > + const struct local_datapath *ldp,
> > > struct hmap *matches, uint8_t ptable,
> > > uint8_t output_ptable, struct ofpbuf
> *ovnacts,
> > > bool ingress, struct lflow_ctx_in *l_ctx_in,
> > > @@ -632,7 +632,7 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> > > .sbrec_multicast_group_by_name_datapath
> > > = l_ctx_in->sbrec_multicast_group_by_name_datapath,
> > > .sbrec_port_binding_by_name =
> l_ctx_in->sbrec_port_binding_by_name,
> > > - .dp = dp,
> > > + .dp = ldp->datapath,
> > > .lflow = lflow,
> > > .lfrr = l_ctx_out->lfrr,
> > > .chassis_tunnels = l_ctx_in->chassis_tunnels,
> > > @@ -652,7 +652,7 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> > > .lookup_port = lookup_port_cb,
> > > .tunnel_ofport = tunnel_ofport_cb,
> > > .aux = &aux,
> > > - .is_switch = datapath_is_switch(dp),
> > > + .is_switch = ldp->is_switch,
> > > .group_table = l_ctx_out->group_table,
> > > .meter_table = l_ctx_out->meter_table,
> > > .lflow_uuid = lflow->header_.uuid,
> > > @@ -674,13 +674,13 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> > >
> > > struct expr_match *m;
> > > HMAP_FOR_EACH (m, hmap_node, matches) {
> > > - match_set_metadata(&m->match, htonll(dp->tunnel_key));
> > > - if (datapath_is_switch(dp)) {
> > > + match_set_metadata(&m->match,
> htonll(ldp->datapath->tunnel_key));
> > > + if (ldp->is_switch) {
> > > unsigned int reg_index
> > > = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) -
> MFF_REG0;
> > > int64_t port_id = m->match.flow.regs[reg_index];
> > > if (port_id) {
> > > - int64_t dp_id = dp->tunnel_key;
> > > + int64_t dp_id = ldp->datapath->tunnel_key;
> > > char buf[16];
> > > get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
> > > if (!sset_contains(l_ctx_in->related_lport_ids, buf)) {
> > > @@ -731,7 +731,7 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> > > */
> > > static struct expr *
> > > convert_match_to_expr(const struct sbrec_logical_flow *lflow,
> > > - const struct sbrec_datapath_binding *dp,
> > > + const struct local_datapath *ldp,
> > > struct expr **prereqs,
> > > const struct shash *addr_sets,
> > > const struct shash *port_groups,
> > > @@ -744,7 +744,8 @@ convert_match_to_expr(const struct
> sbrec_logical_flow *lflow,
> > >
> > > struct expr *e = expr_parse_string(lflow->match, &symtab,
> addr_sets,
> > > port_groups, &addr_sets_ref,
> > > - &port_groups_ref,
> dp->tunnel_key,
> > > + &port_groups_ref,
> > > + ldp->datapath->tunnel_key,
> > > &error);
> > > const char *addr_set_name;
> > > SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> > > @@ -791,7 +792,9 @@ consider_logical_flow__(const struct
> sbrec_logical_flow *lflow,
> > > struct lflow_ctx_in *l_ctx_in,
> > > struct lflow_ctx_out *l_ctx_out)
> > > {
> > > - if (!get_local_datapath(l_ctx_in->local_datapaths,
> dp->tunnel_key)) {
> > > + struct local_datapath *ldp =
> get_local_datapath(l_ctx_in->local_datapaths,
> > > + dp->tunnel_key);
> > > + if (!ldp) {
> > > VLOG_DBG("lflow "UUID_FMT" is not for local datapath, skip",
> > > UUID_ARGS(&lflow->header_.uuid));
> > > return;
> > > @@ -904,7 +907,7 @@ consider_logical_flow__(const struct
> sbrec_logical_flow *lflow,
> > > /* Get match expr, either from cache or from lflow match. */
> > > switch (lcv_type) {
> > > case LCACHE_T_NONE:
> > > - expr = convert_match_to_expr(lflow, dp, &prereqs,
> l_ctx_in->addr_sets,
> > > + expr = convert_match_to_expr(lflow, ldp, &prereqs,
> l_ctx_in->addr_sets,
> > > l_ctx_in->port_groups,
> l_ctx_out->lfrr,
> > > &pg_addr_set_ref);
> > > if (!expr) {
> > > @@ -968,7 +971,7 @@ consider_logical_flow__(const struct
> sbrec_logical_flow *lflow,
> > > break;
> > > }
> > >
> > > - add_matches_to_flow_table(lflow, dp, matches, ptable,
> output_ptable,
> > > + add_matches_to_flow_table(lflow, ldp, matches, ptable,
> output_ptable,
> > > &ovnacts, ingress, l_ctx_in, l_ctx_out);
> > >
> > > /* Update cache if needed. */
> > > diff --git a/controller/local_data.c b/controller/local_data.c
> > > index 1857bfd1ba..98445902b7 100644
> > > --- a/controller/local_data.c
> > > +++ b/controller/local_data.c
> > > @@ -50,6 +50,9 @@ static struct tracked_datapath
> *tracked_datapath_create(
> > > enum en_tracked_resource_type tracked_type,
> > > struct hmap *tracked_datapaths);
> > >
> > > +static bool datapath_is_switch(const struct sbrec_datapath_binding *);
> > > +static bool datapath_is_transit_switch(const struct
> sbrec_datapath_binding *);
> > > +
> > > static uint64_t local_datapath_usage;
> > >
> > > struct local_datapath *
> > > @@ -605,3 +608,15 @@ local_datapath_peer_port_add(struct local_datapath
> *ld,
> > > ld->peer_ports[ld->n_peer_ports - 1].local = local;
> > > ld->peer_ports[ld->n_peer_ports - 1].remote = remote;
> > > }
> > > +
> > > +static bool
> > > +datapath_is_switch(const struct sbrec_datapath_binding *ldp)
> > > +{
> > > + return smap_get(&ldp->external_ids, "logical-switch") != NULL;
> > > +}
> > > +
> > > +static bool
> > > +datapath_is_transit_switch(const struct sbrec_datapath_binding *ldp)
> > > +{
> > > + return smap_get(&ldp->external_ids, "interconn-ts") != NULL;
> > > +}
> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > index 8631bccbc1..1a80ea87e2 100644
> > > --- a/controller/ovn-controller.c
> > > +++ b/controller/ovn-controller.c
> > > @@ -634,6 +634,12 @@ alloc_id_to_ct_zone(const char *zone_name, struct
> simap *ct_zones,
> > > return true;
> > > }
> > >
> > > +static int
> > > +get_snat_ct_zone(const struct sbrec_datapath_binding *dp)
> > > +{
> > > + return smap_get_int(&dp->external_ids, "snat-ct-zone", -1);
> > > +}
> > > +
> > > static void
> > > update_ct_zones(const struct shash *binding_lports,
> > > const struct hmap *local_datapaths,
> > > @@ -665,7 +671,7 @@ update_ct_zones(const struct shash *binding_lports,
> > > shash_add(&all_lds, dnat, ld);
> > > shash_add(&all_lds, snat, ld);
> > >
> > > - int req_snat_zone = datapath_snat_ct_zone(ld->datapath);
> > > + int req_snat_zone = get_snat_ct_zone(ld->datapath);
> > > if (req_snat_zone >= 0) {
> > > simap_put(&req_snat_zones, snat, req_snat_zone);
> > > }
> > > @@ -1851,7 +1857,7 @@ ct_zones_datapath_binding_handler(struct
> engine_node *node, void *data)
> > > return false;
> > > }
> > >
> > > - int req_snat_zone = datapath_snat_ct_zone(dp);
> > > + int req_snat_zone = get_snat_ct_zone(dp);
> > > if (req_snat_zone == -1) {
> > > /* datapath snat ct zone is not set. This condition will
> also hit
> > > * when CMS clears the snat-ct-zone for the logical router.
> > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > > index 293aecea27..08eb90e6ec 100644
> > > --- a/controller/pinctrl.c
> > > +++ b/controller/pinctrl.c
> > > @@ -4290,7 +4290,7 @@ run_buffered_binding(struct ovsdb_idl_index
> *sbrec_mac_binding_by_lport_ip,
> > > * a router datapath. Hence we can skip logical switch
> > > * datapaths.
> > > * */
> > > - if (datapath_is_switch(ld->datapath)) {
> > > + if (ld->is_switch) {
> > > continue;
> > > }
> > >
> > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > > index e6d0f73565..a22ae84fee 100644
> > > --- a/lib/ovn-util.c
> > > +++ b/lib/ovn-util.c
> > > @@ -559,24 +559,6 @@ ovn_logical_flow_hash_datapath(const struct uuid
> *logical_datapath,
> > > return hash_add(hash, uuid_hash(logical_datapath));
> > > }
> > >
> > > -bool
> > > -datapath_is_switch(const struct sbrec_datapath_binding *ldp)
> > > -{
> > > - return smap_get(&ldp->external_ids, "logical-switch") != NULL;
> > > -}
> > > -
> > > -bool
> > > -datapath_is_transit_switch(const struct sbrec_datapath_binding *ldp)
> > > -{
> > > - return smap_get(&ldp->external_ids, "interconn-ts") != NULL;
> > > -}
> > > -
> > > -int
> > > -datapath_snat_ct_zone(const struct sbrec_datapath_binding *dp)
> > > -{
> > > - return smap_get_int(&dp->external_ids, "snat-ct-zone", -1);
> > > -}
> > > -
> > >
> > > struct tnlid_node {
> > > struct hmap_node hmap_node;
> > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > > index 743d28745f..1fe91ba994 100644
> > > --- a/lib/ovn-util.h
> > > +++ b/lib/ovn-util.h
> > > @@ -117,9 +117,6 @@ uint32_t ovn_logical_flow_hash(uint8_t table_id,
> enum ovn_pipeline pipeline,
> > > const char *match, const char *actions);
> > > uint32_t ovn_logical_flow_hash_datapath(const struct uuid
> *logical_datapath,
> > > uint32_t hash);
> > > -bool datapath_is_switch(const struct sbrec_datapath_binding *);
> > > -bool datapath_is_transit_switch(const struct sbrec_datapath_binding
> *ldp);
> > > -int datapath_snat_ct_zone(const struct sbrec_datapath_binding *ldp);
> > > void ovn_conn_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > > const char *argv[] OVS_UNUSED, void *idl_);
> > >
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > dev mailing list
> > > [email protected]
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
