Also, IMO this series is good to go to the branch-2.7 as well. Jarno
> On Jan 25, 2017, at 2:54 PM, Jarno Rajahalme <[email protected]> wrote: > > With a few remarks below, > > Acked-by: Jarno Rajahalme <[email protected] <mailto:[email protected]>> > >> On Jan 24, 2017, at 9:57 PM, Pravin B Shelar <[email protected]> wrote: >> >> OVS router is basically partial copy of linux kernel FIB. >> kernel routing table uses skb-mark along with usual routing >> parameters. Following patch brings in support for skb-mark >> to ovs-router so that we can lookup route for given skb-mark. >> >> Signed-off-by: Pravin B Shelar <[email protected]> >> --- >> v1-v2: >> Removed ovs/route2/add command >> reverted change to plen variable type. >> --- >> lib/netdev-vport.c | 4 +- >> lib/ovs-router.c | 125 >> ++++++++++++++++++++++++++++++------------- >> lib/ovs-router.h | 6 ++- >> lib/route-table.c | 2 +- >> ofproto/ofproto-dpif-sflow.c | 5 +- >> ofproto/ofproto-dpif-xlate.c | 2 +- >> tests/ovs-router.at | 42 +++++++++++++++ >> tests/tunnel-push-pop.at | 3 ++ >> 8 files changed, 147 insertions(+), 42 deletions(-) >> >> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c >> index 88b0bcf..2d0aa43 100644 >> --- a/lib/netdev-vport.c >> +++ b/lib/netdev-vport.c >> @@ -260,10 +260,12 @@ tunnel_check_status_change__(struct netdev_vport >> *netdev) >> bool status = false; >> struct in6_addr *route; >> struct in6_addr gw; >> + uint32_t mark; >> >> iface[0] = '\0'; >> route = &netdev->tnl_cfg.ipv6_dst; >> - if (ovs_router_lookup(route, iface, NULL, &gw)) { >> + mark = netdev->tnl_cfg.egress_pkt_mark; >> + if (ovs_router_lookup(mark, route, iface, NULL, &gw)) { >> struct netdev *egress_netdev; >> >> if (!netdev_open(iface, NULL, &egress_netdev)) { >> diff --git a/lib/ovs-router.c b/lib/ovs-router.c >> index 935b60a..d30eb3c 100644 >> --- a/lib/ovs-router.c >> +++ b/lib/ovs-router.c >> @@ -45,6 +45,11 @@ >> #include "unaligned.h" >> #include "unixctl.h" >> #include "util.h" >> +#include "openvswitch/vlog.h" >> + >> +VLOG_DEFINE_THIS_MODULE(ovs_router); >> + >> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> >> static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER; >> static struct classifier cls; >> @@ -57,6 +62,7 @@ struct ovs_router_entry { >> struct in6_addr src_addr; >> uint8_t plen; >> uint8_t priority; >> + uint32_t mark; >> }; >> >> static struct ovs_router_entry * >> @@ -88,11 +94,12 @@ ovs_router_lookup_fallback(const struct in6_addr >> *ip6_dst, char output_bridge[], >> } >> >> bool >> -ovs_router_lookup(const struct in6_addr *ip6_dst, char output_bridge[], >> +ovs_router_lookup(uint32_t mark, const struct in6_addr *ip6_dst, >> + char output_bridge[], >> struct in6_addr *src, struct in6_addr *gw) >> { >> const struct cls_rule *cr; >> - struct flow flow = {.ipv6_dst = *ip6_dst}; >> + struct flow flow = {.ipv6_dst = *ip6_dst, .pkt_mark = mark}; >> >> cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL); >> if (cr) { >> @@ -115,7 +122,8 @@ rt_entry_free(struct ovs_router_entry *p) >> free(p); >> } >> >> -static void rt_init_match(struct match *match, const struct in6_addr >> *ip6_dst, >> +static void rt_init_match(struct match *match, uint32_t mark, >> + const struct in6_addr *ip6_dst, >> uint8_t plen) >> { >> struct in6_addr dst; >> @@ -127,6 +135,8 @@ static void rt_init_match(struct match *match, const >> struct in6_addr *ip6_dst, >> memset(match, 0, sizeof *match); >> match->flow.ipv6_dst = dst; >> match->wc.masks.ipv6_dst = mask; >> + match->wc.masks.pkt_mark = UINT32_MAX; >> + match->flow.pkt_mark = mark; >> } >> >> static int >> @@ -178,7 +188,8 @@ out: >> } >> >> static int >> -ovs_router_insert__(uint8_t priority, const struct in6_addr *ip6_dst, >> +ovs_router_insert__(uint32_t mark, uint8_t priority, >> + const struct in6_addr *ip6_dst, >> uint8_t plen, const char output_bridge[], >> const struct in6_addr *gw) >> { >> @@ -187,13 +198,14 @@ ovs_router_insert__(uint8_t priority, const struct >> in6_addr *ip6_dst, >> struct match match; >> int err; >> >> - rt_init_match(&match, ip6_dst, plen); >> + rt_init_match(&match, mark, ip6_dst, plen); >> >> p = xzalloc(sizeof *p); >> ovs_strlcpy(p->output_bridge, output_bridge, sizeof p->output_bridge); >> if (ipv6_addr_is_set(gw)) { >> p->gw = *gw; >> } >> + p->mark = mark; >> p->nw_addr = match.flow.ipv6_dst; >> p->plen = plen; >> p->priority = priority; >> @@ -202,7 +214,12 @@ ovs_router_insert__(uint8_t priority, const struct >> in6_addr *ip6_dst, >> err = get_src_addr(gw, output_bridge, &p->src_addr); >> } >> if (err) { >> + struct ds ds = DS_EMPTY_INITIALIZER; >> + >> + ipv6_format_mapped(ip6_dst, &ds); >> + VLOG_DBG_RL(&rl, "src addr not available for route %s", >> ds_cstr(&ds)); >> free(p); >> + ds_destroy(&ds); >> return err; >> } >> /* Longest prefix matches first. */ >> @@ -222,13 +239,12 @@ ovs_router_insert__(uint8_t priority, const struct >> in6_addr *ip6_dst, >> } >> >> void >> -ovs_router_insert(const struct in6_addr *ip_dst, uint8_t plen, >> +ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t >> plen, >> const char output_bridge[], const struct in6_addr *gw) >> { >> - ovs_router_insert__(plen, ip_dst, plen, output_bridge, gw); >> + ovs_router_insert__(mark, plen, ip_dst, plen, output_bridge, gw); >> } >> >> - >> static bool >> __rt_entry_delete(const struct cls_rule *cr) >> { >> @@ -245,14 +261,15 @@ __rt_entry_delete(const struct cls_rule *cr) >> } >> >> static bool >> -rt_entry_delete(uint8_t priority, const struct in6_addr *ip6_dst, uint8_t >> plen) >> +rt_entry_delete(uint32_t mark, uint8_t priority, >> + const struct in6_addr *ip6_dst, uint8_t plen) >> { >> const struct cls_rule *cr; >> struct cls_rule rule; >> struct match match; >> bool res = false; >> >> - rt_init_match(&match, ip6_dst, plen); >> + rt_init_match(&match, mark, ip6_dst, plen); >> >> cls_rule_init(&rule, &match, priority); >> >> @@ -292,32 +309,48 @@ static void >> ovs_router_add(struct unixctl_conn *conn, int argc, >> const char *argv[], void *aux OVS_UNUSED) >> { >> - ovs_be32 ip; >> - unsigned int plen; >> + struct in6_addr gw6 = in6addr_any; >> struct in6_addr ip6; >> - struct in6_addr gw6; >> + uint32_t mark = 0; >> + unsigned int plen; >> + ovs_be32 ip; > > Most of the above changes are not necessary. > >> int err; >> >> if (scan_ipv4_route(argv[1], &ip, &plen)) { >> ovs_be32 gw = 0; >> - if (argc > 3 && !ip_parse(argv[3], &gw)) { >> - unixctl_command_reply_error(conn, "Invalid gateway"); >> - return; >> + >> + if (argc > 3) { >> + if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) && >> + !ip_parse(argv[3], &gw)) { >> + unixctl_command_reply_error(conn, "Invalid pkt_mark or >> gateway"); >> + return; >> + } >> } >> in6_addr_set_mapped_ipv4(&ip6, ip); >> - in6_addr_set_mapped_ipv4(&gw6, gw); >> + if (gw) { >> + in6_addr_set_mapped_ipv4(&gw6, gw); >> + } >> plen += 96; >> } else if (scan_ipv6_route(argv[1], &ip6, &plen)) { >> - gw6 = in6addr_any; >> - if (argc > 3 && !ipv6_parse(argv[3], &gw6)) { >> - unixctl_command_reply_error(conn, "Invalid IPv6 gateway"); >> - return; >> + if (argc > 3) { >> + if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) && >> + !ipv6_parse(argv[3], &gw6)) { >> + unixctl_command_reply_error(conn, "Invalid pkt_mark or IPv6 >> gateway"); >> + return; >> + } >> } >> } else { >> unixctl_command_reply_error(conn, "Invalid parameters"); >> return; >> } >> - err = ovs_router_insert__(plen + 32, &ip6, plen, argv[2], &gw6); >> + if (argc > 4) { >> + if (!ovs_scan(argv[4], "pkt_mark=%"SCNi32, &mark)) { >> + unixctl_command_reply_error(conn, "Invalid pkt_mark"); >> + return; >> + } >> + } >> + >> + err = ovs_router_insert__(mark, plen + 32, &ip6, plen, argv[2], &gw6); >> if (err) { >> unixctl_command_reply_error(conn, "Error while inserting route."); >> } else { >> @@ -329,9 +362,10 @@ static void >> ovs_router_del(struct unixctl_conn *conn, int argc OVS_UNUSED, >> const char *argv[], void *aux OVS_UNUSED) >> { >> - ovs_be32 ip; >> - unsigned int plen; >> struct in6_addr ip6; >> + uint32_t mark = 0; >> + unsigned int plen; >> + ovs_be32 ip; >> > > Most of the changes above are not necessary. > >> if (scan_ipv4_route(argv[1], &ip, &plen)) { >> in6_addr_set_mapped_ipv4(&ip6, ip); >> @@ -340,7 +374,14 @@ ovs_router_del(struct unixctl_conn *conn, int argc >> OVS_UNUSED, >> unixctl_command_reply_error(conn, "Invalid parameters"); >> return; >> } >> - if (rt_entry_delete(plen + 32, &ip6, plen)) { >> + if (argc > 2) { >> + if (!ovs_scan(argv[2], "pkt_mark=%"SCNi32, &mark)) { >> + unixctl_command_reply_error(conn, "Invalid pkt_mark"); >> + return; >> + } >> + } >> + >> + if (rt_entry_delete(mark, plen + 32, &ip6, plen)) { >> unixctl_command_reply(conn, "OK"); >> seq_change(tnl_conf_seq); >> } else { >> @@ -368,7 +409,12 @@ ovs_router_show(struct unixctl_conn *conn, int argc >> OVS_UNUSED, >> if (IN6_IS_ADDR_V4MAPPED(&rt->nw_addr)) { >> plen -= 96; >> } >> - ds_put_format(&ds, "/%"PRIu16" dev %s", plen, rt->output_bridge); >> + ds_put_format(&ds, "/%"PRIu16, plen); >> + if (rt->mark) { >> + ds_put_format(&ds, " MARK %"PRIu32, rt->mark); >> + } >> + >> + ds_put_format(&ds, " dev %s", rt->output_bridge); >> if (ipv6_addr_is_set(&rt->gw)) { >> ds_put_format(&ds, " GW "); >> ipv6_format_mapped(&rt->gw, &ds); >> @@ -382,14 +428,15 @@ ovs_router_show(struct unixctl_conn *conn, int argc >> OVS_UNUSED, >> } >> >> static void >> -ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED, >> +ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc, >> const char *argv[], void *aux OVS_UNUSED) >> { >> - ovs_be32 ip; >> + struct in6_addr gw, src; >> + char iface[IFNAMSIZ]; >> struct in6_addr ip6; >> unsigned int plen; >> - char iface[IFNAMSIZ]; >> - struct in6_addr gw, src; >> + uint32_t mark = 0; >> + ovs_be32 ip; >> > > Most of the changes above are not necessary. > >> if (scan_ipv4_route(argv[1], &ip, &plen) && plen == 32) { >> in6_addr_set_mapped_ipv4(&ip6, ip); >> @@ -397,9 +444,15 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int >> argc OVS_UNUSED, >> unixctl_command_reply_error(conn, "Invalid parameters"); >> return; >> } >> - >> - if (ovs_router_lookup(&ip6, iface, &src, &gw)) { >> + if (argc > 2) { >> + if (!ovs_scan(argv[2], "pkt_mark=%"SCNi32, &mark)) { >> + unixctl_command_reply_error(conn, "Invalid pkt_mark"); >> + return; >> + } >> + } >> + if (ovs_router_lookup(mark, &ip6, iface, &src, &gw)) { >> struct ds ds = DS_EMPTY_INITIALIZER; >> + >> ds_put_format(&ds, "src "); >> ipv6_format_mapped(&src, &ds); >> ds_put_format(&ds, "\ngateway "); >> @@ -434,11 +487,11 @@ void >> ovs_router_init(void) >> { >> classifier_init(&cls, NULL); >> - unixctl_command_register("ovs/route/add", "ip_addr/prefix_len >> out_br_name gw", 2, 3, >> + unixctl_command_register("ovs/route/add", "ip_addr/prefix_len >> out_br_name [gw | pkt_mark=mark]", 2, 4, > > The above reads like you could optionally have ‘gw’ or ‘pkt_mark’. Should > that be “[gw] [pkt_mark=mark]” instead? > >> ovs_router_add, NULL); >> unixctl_command_register("ovs/route/show", "", 0, 0, ovs_router_show, >> NULL); >> - unixctl_command_register("ovs/route/del", "ip_addr/prefix_len", 1, 1, >> ovs_router_del, >> - NULL); >> - unixctl_command_register("ovs/route/lookup", "ip_addr", 1, 1, >> + unixctl_command_register("ovs/route/del", "ip_addr/prefix_len >> [pkt_mark=mark]", 1, 2, >> + ovs_router_del, NULL); >> + unixctl_command_register("ovs/route/lookup", "ip_addr [pkt_mark=mark]", >> 1, 2, >> ovs_router_lookup_cmd, NULL); >> } >> diff --git a/lib/ovs-router.h b/lib/ovs-router.h >> index 9cb7509..29c7c5f 100644 >> --- a/lib/ovs-router.h >> +++ b/lib/ovs-router.h >> @@ -25,10 +25,12 @@ >> extern "C" { >> #endif >> >> -bool ovs_router_lookup(const struct in6_addr *ip_dst, char out_dev[], >> +bool ovs_router_lookup(uint32_t mark, const struct in6_addr *ip_dst, >> + char out_dev[], >> struct in6_addr *src, struct in6_addr *gw); >> void ovs_router_init(void); >> -void ovs_router_insert(const struct in6_addr *ip_dst, uint8_t plen, >> +void ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, >> + uint8_t plen, >> const char output_bridge[], const struct in6_addr *gw); >> void ovs_router_flush(void); >> #ifdef __cplusplus >> diff --git a/lib/route-table.c b/lib/route-table.c >> index 61c8cd8..ae8af43 100644 >> --- a/lib/route-table.c >> +++ b/lib/route-table.c >> @@ -298,7 +298,7 @@ route_table_handle_msg(const struct route_table_msg >> *change) >> if (change->relevant && change->nlmsg_type == RTM_NEWROUTE) { >> const struct route_data *rd = &change->rd; >> >> - ovs_router_insert(&rd->rta_dst, rd->rtm_dst_len, >> + ovs_router_insert(rd->mark, &rd->rta_dst, rd->rtm_dst_len, >> rd->ifname, &rd->rta_gw); >> } >> } >> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c >> index e4ae760..520b8dd 100644 >> --- a/ofproto/ofproto-dpif-sflow.c >> +++ b/ofproto/ofproto-dpif-sflow.c >> @@ -449,7 +449,10 @@ sflow_choose_agent_address(const char *agent_device, >> struct in6_addr addr6, src, gw; >> >> in6_addr_set_mapped_ipv4(&addr6, sa.sin.sin_addr.s_addr); >> - if (ovs_router_lookup(&addr6, name, &src, &gw)) { >> + /* sFlow only supports target in default routing table with >> + * packet mark zero. >> + */ >> + if (ovs_router_lookup(0, &addr6, name, &src, &gw)) { >> >> in4.s_addr = in6_addr_get_mapped_ipv4(&src); >> goto success; >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 0513394..f12acf3 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -2878,7 +2878,7 @@ tnl_route_lookup_flow(const struct flow *oflow, >> struct in6_addr dst; >> >> dst = flow_tnl_dst(&oflow->tunnel); >> - if (!ovs_router_lookup(&dst, out_dev, src, &gw)) { >> + if (!ovs_router_lookup(oflow->pkt_mark, &dst, out_dev, src, &gw)) { >> return -ENOENT; >> } >> >> diff --git a/tests/ovs-router.at b/tests/ovs-router.at >> index 93a730a..e1fa640 100644 >> --- a/tests/ovs-router.at >> +++ b/tests/ovs-router.at >> @@ -20,10 +20,32 @@ AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 >> 192.0.2.1/24], [0], [OK >> ]) >> AT_CHECK([ovs-appctl ovs/route/add 198.51.100.0/24 br0 192.0.2.254], [0], [OK >> ]) >> +AT_CHECK([ovs-appctl ovs/route/add 192.0.2.1/24 br0 pkt_mark=123], [0], [OK >> +]) >> + >> +AT_CHECK([ovs-appctl ovs/route/add 198.51.100.200/24 br0 192.0.2.250 >> pkt_mark=1234], [0], [OK >> +]) >> + >> +AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [User: >> 192.0.2.0/24 MARK 123 dev br0 SRC 192.0.2.1 >> +User: 198.51.100.0/24 MARK 1234 dev br0 GW 192.0.2.250 SRC 192.0.2.1 >> +User: 198.51.100.0/24 dev br0 GW 192.0.2.254 SRC 192.0.2.1 >> +]) >> + >> AT_CHECK([ovs-appctl ovs/route/lookup 198.51.100.1], [0], [src 192.0.2.1 >> gateway 192.0.2.254 >> dev br0 >> ]) >> + >> +AT_CHECK([ovs-appctl ovs/route/lookup 198.51.100.1 pkt_mark=1234], [0], >> [src 192.0.2.1 >> +gateway 192.0.2.250 >> +dev br0 >> +]) >> +AT_CHECK([ovs-appctl ovs/route/del 198.51.100.0/24 pkt_mark=1234], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [User: >> 192.0.2.0/24 MARK 123 dev br0 SRC 192.0.2.1 >> +User: 198.51.100.0/24 dev br0 GW 192.0.2.254 SRC 192.0.2.1 >> +]) >> + >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> @@ -34,9 +56,29 @@ AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 >> 2001:db8:cafe::1/64], [0], [OK >> ]) >> AT_CHECK([ovs-appctl ovs/route/add 2001:db8:babe::/64 br0 2001:db8:cafe::2], >> [0], [OK >> ]) >> +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:babe::/64 br0 2001:db8:cafe::3 >> pkt_mark=321], [0], [OK >> +]) >> + >> +AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [dnl >> +User: 2001:db8:babe::/64 MARK 321 dev br0 GW 2001:db8:cafe::3 SRC >> 2001:db8:cafe::1 >> +User: 2001:db8:babe::/64 dev br0 GW 2001:db8:cafe::2 SRC 2001:db8:cafe::1 >> +]) >> + >> AT_CHECK([ovs-appctl ovs/route/lookup 2001:db8:babe::1eaf], [0], [src >> 2001:db8:cafe::1 >> gateway 2001:db8:cafe::2 >> dev br0 >> ]) >> + >> +AT_CHECK([ovs-appctl ovs/route/lookup 2001:db8:babe::1eaf pkt_mark=321], >> [0], [src 2001:db8:cafe::1 >> +gateway 2001:db8:cafe::3 >> +dev br0 >> +]) >> + >> +AT_CHECK([ovs-appctl ovs/route/del 2001:db8:babe::/64 pkt_mark=321], [0], >> [OK >> +]) >> +AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [dnl >> +User: 2001:db8:babe::/64 dev br0 GW 2001:db8:cafe::2 SRC 2001:db8:cafe::1 >> +]) >> + >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at >> index 4aaa669..4eeac41 100644 >> --- a/tests/tunnel-push-pop.at >> +++ b/tests/tunnel-push-pop.at >> @@ -40,6 +40,9 @@ AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 >> 2001:cafe::88/24], [0], [OK >> AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 br0], [0], [OK >> ]) >> >> +AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 br0 pkt_mark=1234], [0], [OK >> +]) >> + >> AT_CHECK([ovs-ofctl add-flow br0 action=normal]) >> >> dnl Check ARP request >> -- >> 2.9.3 >> >> _______________________________________________ >> 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
