On Wed, Jun 17, 2020 at 3:20 PM Roi Dayan <r...@mellanox.com> wrote:
>
>
>
> On 2020-06-17 8:48 AM, Tonghao Zhang wrote:
> > On Tue, Jun 16, 2020 at 9:05 PM Roi Dayan <r...@mellanox.com> wrote:
> >>
> >> The cited commit intended to add tc support for masking tunnel src/dst
> >> ips and ports. It's not possible to do tunnel ports masking with
> >> openflow rules and the default mask for tunnel ports set to 0 in
> >> tnl_wc_init(), unlike tunnel ports default mask which is full mask.
> >> So instead of never passing tunnel ports to tc, revert the changes
> >> to tunnel ports to always pass the tunnel port.
> >> In sw classification is done by the kernel, but for hw we must match
> >> the tunnel dst port.
> > Hi Roi
> > I use the "ovs-appctl dpctl/add-flow" to install rules with tunnel
> > src/dst port masked to tc dp.
> > And I didn't find that feature affects openflow layer. "make check"
> > all test suite were passed.
> > If hw we must match tunnel dst port, can we change the tnl_wc_init. I
> > test it ok.
>
> dpctl used to debug and not strict like openflows. you can do a lot of stuff
> in it to break ovs normal operation, e.g. not aging rules, not adding new
> rules.
> But with openflow rules, you cannot actually control masking of tunnel ports.
> "make check" is more of unit testing. its spawning new ovs process for a
> single test/check so I don't think it's valid check for this case.
>
> also, I'm not sure about changing tnl_wc_init(), because of the specific
> comment
> about not to change it. also src port should stay 0.
> otherwise you will get a lot of rules because the src port is random port
> opened
> for a connection.
>
> from the original commit "8b7ea2d Extend OVS IPFIX exporter to export tunnel
> headers"
> I see there is another comment in other place in the code expecting default 0
> for ports.
> * The protocol identifier of DPIF_IPFIX_TUNNEL_IPSEC_GRE is IPPROTO_GRE,
> * and both tp_src and tp_dst are zero.
Thanks Roi, I got it.
> >
> > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> > index 03f0ab76562a..5145a6d54e80 100644
> > --- a/ofproto/tunnel.c
> > +++ b/ofproto/tunnel.c
> > @@ -381,8 +381,8 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards
> > *wc)
> > wc->masks.tunnel.ip_ttl = 0;
> > /* The tp_src and tp_dst members in flow_tnl are set to be always
> > * wildcarded, not to unwildcard them here. */
> > - wc->masks.tunnel.tp_src = 0;
> > - wc->masks.tunnel.tp_dst = 0;
> > + wc->masks.tunnel.tp_src = OVS_BE16_MAX;
> > + wc->masks.tunnel.tp_dst = OVS_BE16_MAX;
> >
> > if (is_ip_any(flow)
> > && IP_ECN_is_ce(flow->tunnel.ip_tos)) {
> >
> >> Fixes: 5f568d049130 ("netdev-offload-tc: Allow to match the IP and port
> >> mask of tunnel")
> >> Signed-off-by: Roi Dayan <r...@mellanox.com>
> >> Reviewed-by: Eli Britstein <el...@mellanox.com>
> >> ---
> >> NEWS | 2 --
> >> include/openvswitch/match.h | 3 ---
> >> lib/match.c | 13 -------------
> >> lib/netdev-offload-tc.c | 13 ++-----------
> >> lib/tc.c | 28 ++--------------------------
> >> tests/tunnel.at | 4 ++--
> >> 6 files changed, 6 insertions(+), 57 deletions(-)
> >>
> >> diff --git a/NEWS b/NEWS
> >> index 88b273a0af89..22cacda20ac7 100644
> >> --- a/NEWS
> >> +++ b/NEWS
> >> @@ -19,8 +19,6 @@ Post-v2.13.0
> >> - Tunnels: TC Flower offload
> >> * Tunnel Local endpoint address masked match are supported.
> >> * Tunnel Romte endpoint address masked match are supported.
> >> - * Tunnel Local endpoint ports masked match are supported.
> >> - * Tunnel Romte endpoint ports masked match are supported.
> >>
> >>
> >> v2.13.0 - 14 Feb 2020
> >> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> >> index 9e480318e2b3..2e8812048e86 100644
> >> --- a/include/openvswitch/match.h
> >> +++ b/include/openvswitch/match.h
> >> @@ -105,9 +105,6 @@ void match_set_tun_flags(struct match *match, uint16_t
> >> flags);
> >> void match_set_tun_flags_masked(struct match *match, uint16_t flags,
> >> uint16_t mask);
> >> void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
> >> void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port,
> >> ovs_be16 mask);
> >> -void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
> >> -void match_set_tun_tp_src_masked(struct match *match,
> >> - ovs_be16 port, ovs_be16 mask);
> >> void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id,
> >> ovs_be16 mask);
> >> void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
> >> void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags,
> >> uint8_t mask);
> >> diff --git a/lib/match.c b/lib/match.c
> >> index a77554851146..ba716579d8ec 100644
> >> --- a/lib/match.c
> >> +++ b/lib/match.c
> >> @@ -294,19 +294,6 @@ match_set_tun_tp_dst_masked(struct match *match,
> >> ovs_be16 port, ovs_be16 mask)
> >> }
> >>
> >> void
> >> -match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
> >> -{
> >> - match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
> >> -}
> >> -
> >> -void
> >> -match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16
> >> mask)
> >> -{
> >> - match->wc.masks.tunnel.tp_src = mask;
> >> - match->flow.tunnel.tp_src = port & mask;
> >> -}
> >> -
> >> -void
> >> match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id,
> >> ovs_be16 mask)
> >> {
> >> match->wc.masks.tunnel.gbp_id = mask;
> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> >> index aa6d22e74f29..258d31f54b08 100644
> >> --- a/lib/netdev-offload-tc.c
> >> +++ b/lib/netdev-offload-tc.c
> >> @@ -712,15 +712,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> >> match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
> >> flower->mask.tunnel.ttl);
> >> }
> >> - if (flower->mask.tunnel.tp_dst) {
> >> - match_set_tun_tp_dst_masked(match,
> >> - flower->key.tunnel.tp_dst,
> >> - flower->mask.tunnel.tp_dst);
> >> - }
> >> - if (flower->mask.tunnel.tp_src) {
> >> - match_set_tun_tp_src_masked(match,
> >> - flower->key.tunnel.tp_src,
> >> - flower->mask.tunnel.tp_src);
> >> + if (flower->key.tunnel.tp_dst) {
> >> + match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
> >> }
> >> if (flower->key.tunnel.metadata.present.len) {
> >> flower_tun_opt_to_match(match, flower);
> >> @@ -1470,8 +1463,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct
> >> match *match,
> >> flower.mask.tunnel.ipv6.ipv6_dst = tnl_mask->ipv6_dst;
> >> flower.mask.tunnel.tos = tnl_mask->ip_tos;
> >> flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
> >> - flower.mask.tunnel.tp_src = tnl_mask->tp_src;
> >> - flower.mask.tunnel.tp_dst = tnl_mask->tp_dst;
> >> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ?
> >> tnl_mask->tun_id : 0;
> >> flower_match_to_tun_opt(&flower, tnl, tnl_mask);
> >> flower.tunnel = true;
> >> diff --git a/lib/tc.c b/lib/tc.c
> >> index 29b4328d8bfc..c2ab77553306 100644
> >> --- a/lib/tc.c
> >> +++ b/lib/tc.c
> >> @@ -395,12 +395,6 @@ static const struct nl_policy tca_flower_policy[] = {
> >> .optional = true, },
> >> [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_U16,
> >> .optional = true, },
> >> - [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT] = { .type = NL_A_U16,
> >> - .optional = true, },
> >> - [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK] = { .type = NL_A_U16,
> >> - .optional = true, },
> >> - [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK] = { .type = NL_A_U16,
> >> - .optional = true, },
> >> [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
> >> [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true,
> >> },
> >> [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
> >> @@ -746,15 +740,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct
> >> tc_flower *flower)
> >> flower->key.tunnel.ipv6.ipv6_dst =
> >> nl_attr_get_in6_addr(attrs[TCA_FLOWER_KEY_ENC_IPV6_DST]);
> >> }
> >> - if (attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]) {
> >> - flower->mask.tunnel.tp_src =
> >> - nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]);
> >> - flower->key.tunnel.tp_src =
> >> - nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT]);
> >> - }
> >> - if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]) {
> >> - flower->mask.tunnel.tp_dst =
> >> - nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]);
> >> + if (attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]) {
> >> flower->key.tunnel.tp_dst =
> >> nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
> >> }
> >> @@ -2713,10 +2699,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request,
> >> struct tc_flower *flower)
> >> struct in6_addr *ipv6_dst_mask = &flower->mask.tunnel.ipv6.ipv6_dst;
> >> struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src;
> >> struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst;
> >> - ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst;
> >> - ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src;
> >> ovs_be16 tp_dst = flower->key.tunnel.tp_dst;
> >> - ovs_be16 tp_src = flower->key.tunnel.tp_src;
> >> ovs_be32 id = be64_to_be32(flower->key.tunnel.id);
> >> uint8_t tos = flower->key.tunnel.tos;
> >> uint8_t ttl = flower->key.tunnel.ttl;
> >> @@ -2748,16 +2731,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request,
> >> struct tc_flower *flower)
> >> nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL, ttl);
> >> nl_msg_put_u8(request, TCA_FLOWER_KEY_ENC_IP_TTL_MASK, ttl_mask);
> >> }
> >> - if (tp_dst_mask) {
> >> - nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
> >> - tp_dst_mask);
> >> + if (tp_dst) {
> >> nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
> >> }
> >> - if (tp_src_mask) {
> >> - nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,
> >> - tp_src_mask);
> >> - nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_SRC_PORT, tp_src);
> >> - }
> >> if (id_mask) {
> >> nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
> >> }
> >> diff --git a/tests/tunnel.at b/tests/tunnel.at
> >> index a74a67aa8123..e08fd1e048f8 100644
> >> --- a/tests/tunnel.at
> >> +++ b/tests/tunnel.at
> >> @@ -123,10 +123,10 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0],
> >> [dnl
> >> p2 2/2: (dummy)
> >> ])
> >>
> >> -AT_CHECK([ovs-appctl dpctl/add-flow
> >> "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1/0xf,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()"
> >> "2"])
> >> +AT_CHECK([ovs-appctl dpctl/add-flow
> >> "tunnel(dst=1.1.1.1,src=3.3.3.200/255.255.255.0,tp_dst=123,tp_src=1,ttl=64),recirc_id(0),in_port(1),eth(),eth_type(0x0800),ipv4()"
> >> "2"])
> >>
> >> AT_CHECK([ovs-appctl dpctl/dump-flows | tail -1], [0], [dnl
> >> -tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1/0xf,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800),
> >> packets:0, bytes:0, used:never, actions:2
> >> +tunnel(src=3.3.3.200/255.255.255.0,dst=1.1.1.1,ttl=64,tp_src=1,tp_dst=123),recirc_id(0),in_port(1),eth_type(0x0800),
> >> packets:0, bytes:0, used:never, actions:2
> >> ])
> >>
> >> OVS_VSWITCHD_STOP
> >> --
> >> 2.8.4
> >>
> >
> >
--
Best regards, Tonghao
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
