[ovs-dev] [PATCH] datapath: 64-bit GRE support

[Jesse Gross]

On Tue, Sep 11, 2012 at 4:21 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
> From: Pravin Shelar <pshelar at nicira.com>
>
> Extend GRE to have a 64-bit key. Use GRE sequence number to store
> upper 32-bits of the key.
>
> Bug #13186
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>

I received a sparse warning when applying this:

  CHECK   /home/jgross/openvswitch/datapath/linux/vport-gre.c
/home/jgross/openvswitch/datapath/linux/vport-gre.c:164:58: warning:
restricted __be32 degrades to integer

Also, to add to Ben's comments about documentation in vswitch.xml, we
should also document the impacts of this on sequence numbers in
general.

> diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
> index ab89c5b..a0ce7fd 100644
> --- a/datapath/vport-gre.c
> +++ b/datapath/vport-gre.c
> @@ -86,11 +97,21 @@ static void gre_build_header(const struct vport *vport,
>                 options++;
>         }
>
> -       if (mutable->out_key || mutable->flags & TNL_F_OUT_KEY_ACTION)
> +       if (mutable->flags & TNL_F_OUT_KEY_ACTION) {
> +               greh->flags |= GRE_KEY;
> +               if (mutable->key.tunnel_type & TNL_T_PROTO_GRE64) {
> +                       greh->flags |= GRE_SEQ;
> +               }
> +       }
> +       if (mutable->out_key) {
>                 greh->flags |= GRE_KEY;
> -
> -       if (mutable->out_key)
>                 *options = be64_get_low32(mutable->out_key);
> +               if (mutable->key.tunnel_type & TNL_T_PROTO_GRE64) {
> +                       options++;
> +                       *options = be64_get_high32(mutable->out_key);
> +                       greh->flags |= GRE_SEQ;
> +               }
> +       }

We should probably make these conditions for key present else-ifs both
here and in gre_update_header().  They should be mutually exclusive
and if they're not then we'll overwrite part of the packet.

> @@ -158,15 +188,19 @@ static int parse_header(struct iphdr *iph, __be16 *flags, __be64 *key)
>         }
>
>         if (greh->flags & GRE_KEY) {
> +               __be32 seq = 0;
>                 hdr_len += GRE_HEADER_SECTION;
> -
> -               *key = be32_extend_to_be64(*options);
> -               options++;
> +               if (unlikely(greh->flags & GRE_SEQ))
> +                       seq = *(options + 1);
> +               *key = key_to_tunnel_id(*options, seq);
>         } else
>                 *key = 0;
>
> -       if (unlikely(greh->flags & GRE_SEQ))
> +       if (unlikely(greh->flags & GRE_SEQ)) {

I would drop the unlikely() now in both places.

I would also move the setting of tunnel_type to the GRE_KEY block
seeing as we only parse the key there.