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

Ben Pfaff blp at nicira.com
Wed Dec 1 23:59:04 UTC 2010


On Wed, Dec 01, 2010 at 03:11:54PM -0800, Jesse Gross wrote:
> 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.

They will become variable-length in the netlink series, in which actions
are completely redone as sequences of Netlink attributes.  So i can
break out just that change from the netlink series and shove it in
before this one.  This requires some work and some review, but it's work
and review that would have to happen fairly soon anyhow.

Or, like you say, we can make the actions fixed-length but longer, for
now.  That will waste 4 or 8 bytes for every datapath action other than
the set_tunnel_id one.  The number of actions can sometimes be very
large, although you are probably right that it is usually small, so this
might be OK.

Which do you prefer?

> > 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.

That's what I thought, too.

> > +/* 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.

Thanks, fixed.

> >  /* 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.

Yes.




More information about the dev mailing list