Message-ID: <CAEP_g=9hkP-7fuFK3zSJcR=2BTK0feq7qUa8LHs3dbGQBy+suw@mail.gmail.com>
Date:	Sun, 3 Jun 2012 18:15:04 +0900
From:	Jesse Gross <jesse@...ira.com>
To:	Simon Horman <horms@...ge.net.au>
Cc:	dev@...nvswitch.org, netdev@...r.kernel.org
Subject: Re: [ovs-dev] [PATCH 01/21] datapath: tunnelling: Replace tun_id with tun_key

On Thu, May 24, 2012 at 6:08 PM, Simon Horman <horms@...ge.net.au> wrote:
> this is a first pass at providing a tun_key which can be used
> as the basis for flow-based tunnelling. The tun_key includes and
> replaces the tun_id in both struct ovs_skb_cb and struct sw_tun_key.
>
> In ovs_skb_cb tun_key is a pointer as it is envisaged that it will grow
> when support for IPv6 to an extent that inlining the structure will result
> in ovs_skb_cb being larger than the 48 bytes available in skb->cb.
>
> As OVS does not support IPv6 as the outer transport protocol for tunnels
> the IPv6 portions of this change, which appeared in the previous revision,
> have been dropped in order to limit the scope and size of this patch.
>
> This patch does not make any effort to retain the existing tun_id behaviour
> nor does it fully implement flow-based tunnels. As such it it is incomplete
> and can't be used in its current form (other than to break OVS tunnelling).
>
> ** Please do not apply **
>
> Cc: Kyle Mestery <kmestery@...co.com>
> Signed-off-by: Simon Horman <horms@...ge.net.au>

Thanks and sorry again about being so slow to look at this.

Overall, this looks pretty good to me.  The main difficulty that I had
was in figuring out what should go with the old behavior and what
should go with the new since it's at an intermediate point between the
two but I understand that it's difficult to break it up in a way that
both encapsulates a particular set of functionality and isn't too
large.  Otherwise, I noticed a few specific things that I noted below.

> diff --git a/datapath/flow.c b/datapath/flow.c
> index d07337c..49c0dd8 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -1162,14 +1166,15 @@ int ovs_flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
>  * get the metadata, that is, the parts of the flow key that cannot be
>  * extracted from the packet itself.
>  */
> -int ovs_flow_metadata_from_nlattrs(u32 *priority, u16 *in_port, __be64 *tun_id,
> +int ovs_flow_metadata_from_nlattrs(u32 *priority, u16 *in_port,
> +                                  struct ovs_key_ipv4_tunnel *tun_key,
>                                   const struct nlattr *attr)
>  {
>        const struct nlattr *nla;
>        int rem;
>
>        *in_port = DP_MAX_PORTS;
> -       *tun_id = 0;
> +       tun_key->tun_id = 0;

I think we probably want to memset the entire tun_key to zero to avoid
having potentially uninitialized data in the flow.

> @@ -1204,15 +1210,21 @@ int ovs_flow_metadata_from_nlattrs(u32 *priority, u16 *in_port, __be64 *tun_id,
>  int ovs_flow_to_nlattrs(const struct sw_flow_key *swkey, struct sk_buff *skb)
>  {
>        struct ovs_key_ethernet *eth_key;
> +       struct ovs_key_ipv4_tunnel *tun_key;
>        struct nlattr *nla, *encap;
>
>        if (swkey->phy.priority &&
>            nla_put_u32(skb, OVS_KEY_ATTR_PRIORITY, swkey->phy.priority))
>                goto nla_put_failure;
>
> -       if (swkey->phy.tun_id != cpu_to_be64(0) &&
> -           nla_put_be64(skb, OVS_KEY_ATTR_TUN_ID, swkey->phy.tun_id))
> -               goto nla_put_failure;
> +       if (swkey->phy.tun_key.ipv4_dst) {

It's probably OK to use DIP equal to zero as a not present marker but
we need to enforce that it's always true - for example we shouldn't
allow somebody to setup a flow that way or receive packets with a zero
address.  Alternately, we may be able to find a spare bit to indicate
this, like is done with vlans.

In any case, I think we need to do some additional validation when
setting up flows to check reserved space, for example, as otherwise
that will never match.

> diff --git a/datapath/flow.h b/datapath/flow.h
> index 5be481e..bab5363 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -42,7 +42,7 @@ struct sw_flow_actions {
>
>  struct sw_flow_key {
>        struct {
> -               __be64  tun_id;         /* Encapsulating tunnel ID. */
> +               struct ovs_key_ipv4_tunnel tun_key;  /* Encapsulating tunnel key. */

This is an optimization but as we get closer I'd like to put the
tun_key at the end of struct sw_flow_key so that packets that didn't
come from a tunnel don't have to pay the cost during the lookup (this
is especially true as we add support for IPv6 tunnels).

In a similar vein, struct ovs_key_ipv4_tunnel contains some fields
that I think can never apply for lookup such as the flags so it would
be nice if we could remove that for lookup.

> @@ -150,6 +150,7 @@ u64 ovs_flow_used_time(unsigned long flow_jiffies);
>  *                         ------  ---  ------  -----
>  *  OVS_KEY_ATTR_PRIORITY      4    --     4      8
>  *  OVS_KEY_ATTR_TUN_ID        8    --     4     12
> + *  OVS_KEY_ATTR_IPV4_TUNNEL  18     2     4     24

If my math is correct, I think the size of the base struct
ova_key_ipv4_tunnel is 24 bytes.

> +static inline void tun_key_swap_addr(struct ovs_key_ipv4_tunnel *tun_key)
> +{
> +       __be32 ndst = tun_key->ipv4_src;
> +       tun_key->ipv4_src = tun_key->ipv4_dst;
> +       tun_key->ipv4_dst = ndst;
> +}

I'm not quite sure when we would need to swap the addresses in a
tunnel and I didn't see any uses of this function.

> +static inline void tun_key_init(struct ovs_key_ipv4_tunnel *tun_key,
> +                               const struct iphdr *iph, __be64 tun_id)
> +{
> +       tun_key->tun_id = tun_id;
> +       tun_key->ipv4_src = iph->saddr;
> +       tun_key->ipv4_dst = iph->daddr;
> +       tun_key->ipv4_tos = iph->tos;
> +       tun_key->ipv4_ttl = iph->ttl;
> +}

Aren't there some fields that we need to zero out to avoid problems in
the lookup?

> diff --git a/datapath/tunnel.c b/datapath/tunnel.c
> index d651c11..010e513 100644
> --- a/datapath/tunnel.c
> +++ b/datapath/tunnel.c
> @@ -367,9 +367,9 @@ struct vport *ovs_tnl_find_port(struct net *net, __be32 saddr, __be32 daddr,
>        return NULL;
>  }
>
> -static void ecn_decapsulate(struct sk_buff *skb, u8 tos)
> +static void ecn_decapsulate(struct sk_buff *skb)
>  {
> -       if (unlikely(INET_ECN_is_ce(tos))) {
> +       if (unlikely(INET_ECN_is_ce(OVS_CB(skb)->tun_key->ipv4_tos))) {

This might come in a later patch, although I didn't see it in a quick
scan, but it should be possible to implement all the ECN encapsulation
and decapsulation in userspace, just like we can do with the rest of
the ToS and TTL.

>  bool ovs_tnl_frag_needed(struct vport *vport,
>                         const struct tnl_mutable_config *mutable,
> -                        struct sk_buff *skb, unsigned int mtu, __be64 flow_key)
> +                        struct sk_buff *skb, unsigned int mtu,
> +                        struct ovs_key_ipv4_tunnel *tun_key)
>  {
>        unsigned int eth_hdr_len = ETH_HLEN;
>        unsigned int total_length = 0, header_length = 0, payload_length;
>        struct ethhdr *eh, *old_eh = eth_hdr(skb);
>        struct sk_buff *nskb;
> +       struct ovs_key_ipv4_tunnel ntun_key;
>
>        /* Sanity check */
>        if (skb->protocol == htons(ETH_P_IP)) {
> @@ -705,8 +707,10 @@ bool ovs_tnl_frag_needed(struct vport *vport,
>         * any way of synthesizing packets.
>         */
>        if ((mutable->flags & (TNL_F_IN_KEY_MATCH | TNL_F_OUT_KEY_ACTION)) ==
> -           (TNL_F_IN_KEY_MATCH | TNL_F_OUT_KEY_ACTION))
> -               OVS_CB(nskb)->tun_id = flow_key;
> +           (TNL_F_IN_KEY_MATCH | TNL_F_OUT_KEY_ACTION)) {
> +               ntun_key = *tun_key;
> +               OVS_CB(nskb)->tun_key = &ntun_key;
> +       }

I guess this is probably where you were going to use the function to
reverse IP addresses.  The logic doesn't really work but it's moot
since this is going away anyways.

> @@ -799,10 +803,8 @@ static void create_tunnel_header(const struct vport *vport,
>        iph->ihl        = sizeof(struct iphdr) >> 2;
>        iph->frag_off   = htons(IP_DF);
>        iph->protocol   = tnl_vport->tnl_ops->ipproto;
> -       iph->tos        = mutable->tos;
>        iph->daddr      = rt->rt_dst;
>        iph->saddr      = rt->rt_src;
> -       iph->ttl        = mutable->ttl;
>        if (!iph->ttl)
>                iph->ttl = ip4_dst_hoplimit(&rt_dst(rt));

I'm not sure that these changes quite belong in this patch (not that
it shouldn't be done but it seems like the supporting code isn't there
yet).

> diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
> index ab89c5b..fd2b038 100644
> --- a/datapath/vport-gre.c
> +++ b/datapath/vport-gre.c
> @@ -101,10 +101,6 @@ static struct sk_buff *gre_update_header(const struct vport *vport,
>        __be32 *options = (__be32 *)(skb_network_header(skb) + mutable->tunnel_hlen
>                                               - GRE_HEADER_SECTION);
>
> -       /* Work backwards over the options so the checksum is last. */
> -       if (mutable->flags & TNL_F_OUT_KEY_ACTION)
> -               *options = be64_get_low32(OVS_CB(skb)->tun_id);

Why does this go away?

> diff --git a/datapath/vport.c b/datapath/vport.c
> index 172261a..0c77a1b 100644
> --- a/datapath/vport.c
> +++ b/datapath/vport.c
> @@ -462,7 +462,7 @@ void ovs_vport_receive(struct vport *vport, struct sk_buff *skb)
>                OVS_CB(skb)->flow = NULL;
>
>        if (!(vport->ops->flags & VPORT_F_TUN_ID))
> -               OVS_CB(skb)->tun_id = 0;
> +               OVS_CB(skb)->tun_key = NULL;

We probably should rename this flag now.

> diff --git a/lib/odp-util.h b/lib/odp-util.h
> index d53f083..4e5a8a1 100644
> --- a/lib/odp-util.h
> +++ b/lib/odp-util.h
> @@ -72,6 +72,7 @@ int odp_actions_from_string(const char *, const struct simap *port_names,
>  *                         ------  ---  ------  -----
>  *  OVS_KEY_ATTR_PRIORITY      4    --     4      8
>  *  OVS_KEY_ATTR_TUN_ID        8    --     4     12
> + *  OVS_KEY_ATTR_IPV4_TUNNEL  18     2     4     24

Same thing about the size here as well.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists