[ovs-dev] [PATCH] Add ODP level handling of OVS_KEY_ATTR_IPV4_TUNNEL.

Jesse Gross jesse at nicira.com
Mon Dec 17 19:32:55 UTC 2012


On Sat, Dec 15, 2012 at 12:26 AM, Jarno Rajahalme
<jarno.rajahalme at nsn.com> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index faa6a00..be8dfc0 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -439,22 +439,33 @@ static int execute_set_action(struct sk_buff *skb,
>                 skb_set_mark(skb, nla_get_u32(nested_attr));
>                 break;
>
> -       case OVS_KEY_ATTR_TUN_ID:
> +       case OVS_KEY_ATTR_TUN_ID: {
>                 /* If we're only using the TUN_ID action, store the value in a
> -                * temporary instance of struct ovs_key_ipv4_tunnel on the stack.
> +                * temporary instance of struct ovs_key_ipv4_tunnel on the
> +                * stack.
>                  * If both IPV4_TUNNEL and TUN_ID are being used together we
>                  * can't write into the IPV4_TUNNEL action, so make a copy and
> -                * write into that version.
> +                * write into that version, but only if the TUN_ID is actually
> +                * different.
> +                * A possible later IPV4_TUNNEL will override anything done
> +                * here.
>                  */
> +
> +               __be64 tun_id = nla_get_be64(nested_attr);
> +
>                 if (!OVS_CB(skb)->tun_key)
>                         memset(tun_key, 0, sizeof(*tun_key));
> -               else if (OVS_CB(skb)->tun_key != tun_key)
> -                       memcpy(tun_key, OVS_CB(skb)->tun_key, sizeof(*tun_key));
> +               else {
> +                       if (OVS_CB(skb)->tun_key->tun_id == tun_id)
> +                               break;
> +                       if (OVS_CB(skb)->tun_key != tun_key)
> +                               memcpy(tun_key, OVS_CB(skb)->tun_key,
> +                                      sizeof *tun_key);

This is compatibility code that will be removed as soon as the
userspace portions are ready, so I'm not sure that there's much point
in optimizing it.  This sequence should also never be generate by
userspace, so we're actually making the common case marginally slower.

> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index 968631d..18779e6 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -372,6 +372,7 @@ struct ovs_key_nd {
>  #define OVS_TNL_F_DONT_FRAGMENT (1 << 0)
>  #define OVS_TNL_F_CSUM (1 << 1)
>  #define OVS_TNL_F_KEY (1 << 2)
> +#define OVS_TNL_F_MASK ((1 << 3) - 1) /* All known bits defined above */

This mask needs to be based on userspace's definition of the flags.
If you compile userspace against a kernel with newer headers then that
doesn't mean that userspace can necessarily understand those flags.

> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index de97fd2..445cd46 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -1361,7 +1378,21 @@ odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow,
>          nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, flow->skb_priority);
>      }
>
> -    if (flow->tunnel.tun_id != htonll(0)) {
> +    if (flow->tunnel.ip_dst) {
> +        struct ovs_key_ipv4_tunnel *ipv4_tun_key;
> +
> +        ipv4_tun_key = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_IPV4_TUNNEL,
> +                                            sizeof *ipv4_tun_key);
> +        /* layouts differ, flags has different size */
> +        ipv4_tun_key->tun_id = flow->tunnel.tun_id;
> +        ipv4_tun_key->tun_flags = flow_to_odp_flags(flow->tunnel.flags);
> +        ipv4_tun_key->ipv4_src = flow->tunnel.ip_src;
> +        ipv4_tun_key->ipv4_dst = flow->tunnel.ip_dst;
> +        ipv4_tun_key->ipv4_tos = flow->tunnel.ip_tos;
> +        ipv4_tun_key->ipv4_ttl = flow->tunnel.ip_ttl;
> +        memset(ipv4_tun_key->pad, 0, sizeof ipv4_tun_key->pad);
> +    }
> +    else if (flow->tunnel.tun_id != htonll(0)) {

Can you put the else if on the previous line to match existing style?

> @@ -1871,6 +1904,25 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
>          expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_TUN_ID;
>      }
>
> +    if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IPV4_TUNNEL)) {
> +        const struct ovs_key_ipv4_tunnel *ipv4_tun_key;
> +
> +        ipv4_tun_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV4_TUNNEL]);
> +
> +        /* check for unknown flags */
> +        if (ipv4_tun_key->tun_flags & ~OVS_TNL_F_MASK) {
> +            fitness = ODP_FIT_TOO_MUCH; /* perfect -> too much */
> +        }

I think we could simplify this a little bit by just not adding
OVS_KEY_ATTR_IPV4_TUNNEL to expected_attrs in this case.  Then
existing logic would handle things and log it, etc.

>  static void
> -commit_set_tun_id_action(const struct flow *flow, struct flow *base,
> +commit_set_tunnel_action(const struct flow *flow, struct flow *base,
>                           struct ofpbuf *odp_actions)
>  {
> -    if (base->tunnel.tun_id == flow->tunnel.tun_id) {
> -        return;
> -    }
> -    base->tunnel.tun_id = flow->tunnel.tun_id;
> +    /*
> +     * A valid IPV4_TUNNEL must have non-zero ip_dst.
> +     */
> +    if (flow->tunnel.ip_dst) {
> +        struct ovs_key_ipv4_tunnel ipv4_tun_key;
> +
> +        if (!memcmp(&base->tunnel, &flow->tunnel, sizeof base->tunnel)) {
> +            return;
> +        }
> +        memcpy(&base->tunnel, &flow->tunnel, sizeof base->tunnel);

Can't we put this check and copy at the beginning and then avoid doing
in each block separately?



More information about the dev mailing list