[ovs-dev] [tun_id64 4/4] Expand tunnel IDs from 32 to 64 bits.

Jesse Gross jesse at nicira.com
Wed Dec 1 23:11:54 UTC 2010


On Tue, Nov 30, 2010 at 5:16 PM, Ben Pfaff <blp at nicira.com> wrote:
> Because datapath (ODP) actions are a fixed 64 bits in length, with 16 bits
> dedicated to the action type, it requires two actions to set the full 64
> bits of the tunnel ID: ODPAT_SET_TUNNEL followed by ODPAT_SET_TUNNEL_HI.
>
> It is awkward to extract or set the low or high 32 bits of an integer that
> is in network byte order, so this commit also changes the various tunnel
> ID fields to be in host byte order.

I really don't like using two actions to set the tunnel ID and then
also the byte swapping that it entails.  It feels a lot like a
workaround to a compatibility problem that I'm not sure we have.

Expanding the size of the actions seems inevitable for IPv6 (or
potentially making them variable size).  Assuming that in most cases
the number of actions is fairly small, there is probably very little
overhead to increasing the size of the structure slightly.

> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
> index 1a59d3b..19126fe 100644
> --- a/include/openflow/nicira-ext.h
> +++ b/include/openflow/nicira-ext.h
> @@ -230,7 +230,8 @@ enum nx_action_subtype {
>     NXAST_POP_QUEUE,            /* struct nx_action_pop_queue */
>     NXAST_REG_MOVE,             /* struct nx_action_reg_move */
>     NXAST_REG_LOAD,             /* struct nx_action_reg_load */
> -    NXAST_NOTE                  /* struct nx_action_note */
> +    NXAST_NOTE,                 /* struct nx_action_note */
> +    NXAST_SET_TUNNEL64,         /* struct nx_action_set_tunnel64s */
>  };

It's unfortunate that we have to introduce another extension.  I
suppose we could use the register mechanism but that would probably be
clunky as well.

> +/* Action structure for NXAST_SET_TUNNEL64.
> + *
> + * Sets the encapsulating tunnel ID to a 64-bit value. */
> +struct nx_action_set_tunnel64 {
> +    ovs_be16 type;                  /* OFPAT_VENDOR. */
> +    ovs_be16 len;                   /* Length is 16. */
> +    ovs_be32 vendor;                /* NX_VENDOR_ID. */
> +    ovs_be16 subtype;               /* NXAST_SET_TUNNEL. */

Comment should refer to NXAST_SET_TUNNEL64.

>  /* Action structure for NXAST_DROP_SPOOFED_ARP.
>  *
>  * Stops processing further actions, if the packet being processed is an
> diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
> index 4885906..bee1bc9 100644
> --- a/include/openvswitch/datapath-protocol.h
> +++ b/include/openvswitch/datapath-protocol.h
> @@ -139,10 +139,9 @@ struct odp_stats {
>
>  /**
>  * struct odp_msg - format of messages read from datapath fd.
> - * @type: One of the %_ODPL_* constants.
>  * @length: Total length of message, including this header.
> + * @type: One of the %_ODPL_* constants.
>  * @port: Port that received the packet embedded in this message.
> - * @reserved: Not currently used.  Should be set to 0.
>  * @arg: Argument value whose meaning depends on @type.
>  *
>  * For @type == %_ODPL_MISS_NR, the header is followed by packet data.  The
> @@ -159,11 +158,10 @@ struct odp_stats {
>  * data.
>  */
>  struct odp_msg {
> -    uint32_t type;
>     uint32_t length;
> +    uint16_t type;
>     uint16_t port;
> -    uint16_t reserved;
> -    uint32_t arg;
> +    uint64_t arg;
>  };

It's too bad that we can't use __aligned_u64 here.  This file really
needs to become Linux specific.




More information about the dev mailing list