[ovs-dev] controller: Use precomputed is_switch instead of querying external IDs.

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.

Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 controller/lflow.c   | 25 ++++++++++++++-----------
 controller/pinctrl.c |  2 +-
 2 files changed, 15 insertions(+), 12 deletions(-)

The idea looks good to me, but I have an additional question. Is it 
still useful to have the datapath_is_switch() function exposed via 
ovn-util.h? Aside from when the local_datapath has its is_switch field 
assigned, is there an occasion where this function would be useful? 
Could we make it a local (aka static) function in 
controller/local_data.c instead?

On 1/19/22 09:11, Dumitru Ceara 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.
> 
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
>   controller/lflow.c   | 25 ++++++++++++++-----------
>   controller/pinctrl.c |  2 +-
>   2 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 933e2f3cc..2474342d4 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -582,7 +582,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,
> @@ -592,7 +592,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,
> @@ -612,7 +612,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,
> @@ -634,13 +634,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)) {
> @@ -691,7 +691,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,
> @@ -704,7 +704,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) {
> @@ -751,7 +752,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;
> @@ -864,7 +867,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) {
> @@ -928,7 +931,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/pinctrl.c b/controller/pinctrl.c
> index d2bb7f441..ee34ac6f4 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -4283,7 +4283,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;
>           }
>   
>

On 2/4/22 16:10, Mark Michelson wrote:
> The idea looks good to me, but I have an additional question. Is it
> still useful to have the datapath_is_switch() function exposed via
> ovn-util.h? Aside from when the local_datapath has its is_switch field
> assigned, is there an occasion where this function would be useful?
> Could we make it a local (aka static) function in
> controller/local_data.c instead?

Good point, I'll prepare a v2 with something like that.

Thanks for the review!