[ovs-dev] [PATCH 12/12] Expand tunnel IDs from 32 to 64 bits.
Ben Pfaff
blp at nicira.com
Thu Dec 9 21:27:40 UTC 2010
On Wed, Dec 08, 2010 at 09:15:59PM -0800, Jesse Gross wrote:
> On Tue, Dec 7, 2010 at 11:00 AM, Ben Pfaff <blp at nicira.com> wrote:
> > static inline struct tnl_vport *tnl_vport_priv(const struct vport *vport)
> > diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
> > index f6996ed..b7e256c 100644
> > --- a/datapath/vport-gre.c
> > +++ b/datapath/vport-gre.c
> > @@ -71,7 +71,7 @@ static void gre_build_header(const struct vport *vport,
> > greh->flags |= GRE_KEY;
> >
> > if (mutable->port_config.out_key)
> > - *options = mutable->port_config.out_key;
> > + *options = htonl(be64_to_cpu(mutable->port_config.out_key));
> > }
>
> We should be able to do better than a double byte swap to handle
> truncation. It's not so big a deal here, when we are just building up
> the cache but in gre_update_header() and parse header() it matters.
> We could just replace this with a simple shift on little endian
> platforms and a no-op on big endian.
Good point. I ran both possibilities through GCC and x >> 32 saves 3 to
4 instructions over htonl(be64_to_cpu(x)).
I don't see an existing helper function for that, drat.
> > static struct sk_buff *gre_update_header(const struct vport *vport,
> > @@ -84,7 +84,7 @@ static struct sk_buff *gre_update_header(const struct vport *vport,
> >
> > /* Work backwards over the options so the checksum is last. */
> > if (mutable->port_config.flags & TNL_F_OUT_KEY_ACTION) {
> > - *options = OVS_CB(skb)->tun_id;
> > + *options = htonl(OVS_CB(skb)->tun_id);
> > options--;
>
> This byte swap isn't right: tun_id isn't in host byte order.
Thanks, I changed this to be64_get_low32(OVS_CB(skb)->tun_id)) (which is
the function I invented to do the big-endian 64->32).
> > diff --git a/include/openvswitch/tunnel.h b/include/openvswitch/tunnel.h
> > index e7d3fce..c338cd0 100644
> > --- a/include/openvswitch/tunnel.h
> > +++ b/include/openvswitch/tunnel.h
> > @@ -53,13 +53,13 @@
> >
> > /* This goes in the "config" member of struct odp_port for tunnel vports. */
> > struct tnl_port_config {
> > - __u32 flags;
> > - __be32 saddr;
> > - __be32 daddr;
> > - __be32 in_key;
> > - __be32 out_key;
> > - __u8 tos;
> > - __u8 ttl;
> > + __aligned_u64 in_key;
> > + __aligned_u64 out_key;
>
> __aligned_be64?
Thanks, I forgot that existed.
> > diff --git a/lib/nx-match.c b/lib/nx-match.c
> > index ea31c79..b63ab86 100644
> > --- a/lib/nx-match.c
> > +++ b/lib/nx-match.c
> > @@ -318,7 +318,7 @@ parse_nxm_entry(struct cls_rule *rule, const struct nxm_field *f,
> >
> > /* Tunnel ID. */
> > case NFI_NXM_NX_TUN_ID:
> > - flow->tun_id = htonl(ntohll(get_unaligned_be64(value)));
> > + flow->tun_id = ntohll(get_unaligned_be64(value));
> > return 0;
> >
> > /* Registers. */
> > @@ -646,7 +646,7 @@ nx_put_match(struct ofpbuf *b, const struct cls_rule *cr)
> >
> > /* Tunnel ID. */
> > if (!(wc & FWW_TUN_ID)) {
> > - nxm_put_64(b, NXM_NX_TUN_ID, htonll(ntohl(flow->tun_id)));
> > + nxm_put_64(b, NXM_NX_TUN_ID, htonll(flow->tun_id));
> > }
>
> Why do we need to do any byte swapping here? Shouldn't everything be
> the same size and in network byte order?
We don't, I guess those are leftovers from the first version. Fixed.
Thanks for the reviews, I'll send out a revised series soon.
More information about the dev
mailing list