[ovs-dev] northd: Lookup route policy before ip routing

If there is no route in table ip_routing, the route policy
item in table policy is useless.
    Because route policy has a higher priority than ip routing,
so moddify the table id of IP_ROUTING and POLICY is a little
better.By this way, there is no need reroute any more, this
should be more effienct.

Signed-off-by: wangchuanlei <wangchuanlei@inspur.com>
---
 northd/northd.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Hi wangchuanlei,

Thanks for the patch!

On 7/10/23 10:46, wangchuanlei wrote:
>     If there is no route in table ip_routing, the route policy
> item in table policy is useless.

I'm sorry but I disagree with this.  We can always add a default route
that matches all traffic that's not matched by any other route.

Afterwards, in the policy stage we can re-route/drop accordingly.

>     Because route policy has a higher priority than ip routing,
> so moddify the table id of IP_ROUTING and POLICY is a little
> better.By this way, there is no need reroute any more, this
> should be more effienct.

I can see how this can be slightly more efficient in slow path but I'm
quite confident the difference won't be significant.  Do you have
performance measurements to indicate the opposite?

The real problem with just swapping the two pipelines is that we change
behavior completely.  CMS out there (ovn-kube, neutron, etc.) rely on
the fact that policies are applied after routing.  We can't just decide
to change this and break compatibility.

I'm quite sure the ovn-kubernetes tests also fail because of this change
in behavior:

https://github.com/ovsrobot/ovn/actions/runs/5506310703/jobs/10035411109

It also makes OVN PBR tests fail:

https://github.com/ovsrobot/ovn/actions/runs/5506310686

> 
> Signed-off-by: wangchuanlei <wangchuanlei@inspur.com>

Regards,
Dumitru

> ---
>  northd/northd.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index a45c8b53a..35187abf8 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -174,10 +174,10 @@ enum ovn_stage {
>      PIPELINE_STAGE(ROUTER, IN,  ND_RA_OPTIONS,   10, "lr_in_nd_ra_options") \
>      PIPELINE_STAGE(ROUTER, IN,  ND_RA_RESPONSE,  11, "lr_in_nd_ra_response") \
>      PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_PRE,  12, "lr_in_ip_routing_pre")  \
> -    PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING,      13, "lr_in_ip_routing")      \
> -    PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_ECMP, 14, "lr_in_ip_routing_ecmp") \
> -    PIPELINE_STAGE(ROUTER, IN,  POLICY,          15, "lr_in_policy")          \
> -    PIPELINE_STAGE(ROUTER, IN,  POLICY_ECMP,     16, "lr_in_policy_ecmp")     \
> +    PIPELINE_STAGE(ROUTER, IN,  POLICY,          13, "lr_in_policy")          \
> +    PIPELINE_STAGE(ROUTER, IN,  POLICY_ECMP,     14, "lr_in_policy_ecmp")     \
> +    PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING,      15, "lr_in_ip_routing")      \
> +    PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_ECMP, 16, "lr_in_ip_routing_ecmp") \
>      PIPELINE_STAGE(ROUTER, IN,  ARP_RESOLVE,     17, "lr_in_arp_resolve")     \
>      PIPELINE_STAGE(ROUTER, IN,  CHK_PKT_LEN,     18, "lr_in_chk_pkt_len")     \
>      PIPELINE_STAGE(ROUTER, IN,  LARGER_PKTS,     19, "lr_in_larger_pkts")     \
> @@ -278,6 +278,7 @@ enum ovn_stage {
>  #define REG_SRC_IPV4 "reg1"
>  #define REG_SRC_IPV6 "xxreg1"
>  #define REG_ROUTE_TABLE_ID "reg7"
> +#define REG_ROUTE_POLICY "reg2"
>  
>  /* Register used to store backend ipv6 address
>   * for load balancer affinity. */
> @@ -10240,6 +10241,7 @@ build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od,
>          bool is_ipv4 = strchr(nexthop, '.') ? true : false;
>          ds_put_format(&actions, "%s = %s; "
>                        "%s = %s; "
> +                      "%s = 1; "
>                        "eth.src = %s; "
>                        "outport = %s; "
>                        "flags.loopback = 1; "
> @@ -10249,6 +10251,7 @@ build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od,
>                        nexthop,
>                        is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
>                        lrp_addr_s,
> +                      REG_ROUTE_POLICY,
>                        out_port->lrp_networks.ea_s,
>                        out_port->json_key);
>  
> @@ -10340,6 +10343,7 @@ build_ecmp_routing_policy_flows(struct hmap *lflows, struct ovn_datapath *od,
>                        out_port->json_key);
>  
>          ds_clear(&match);
> +        ds_put_cstr(&actions, REG_ROUTE_POLICY" = 1; ");
>          ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu16" && "
>                        REG_ECMP_MEMBER_ID" == %"PRIuSIZE,
>                        ecmp_group_id, i + 1);
> @@ -12911,6 +12915,8 @@ build_mcast_lookup_flows_for_lrouter(
>       */
>      ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10550,
>                    "nd_rs || nd_ra", debug_drop_action());
> +    ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10000,
> +                  REG_ROUTE_POLICY" == 1", "reg8[0..15] = 0; next;");
>      if (!od->mcast_info.rtr.relay) {
>          return;
>      }