[ovs-dev] [PATCH] odp-util: Update ODPUTIL_FLOW_KEY_BYTES for current kernel flow format.

Ben Pfaff blp at nicira.com
Tue May 15 16:58:19 UTC 2012


On Tue, May 15, 2012 at 09:40:46AM -0700, Justin Pettit wrote:
> That's quite a packet.  The change looks reasonable to me--and 200
> bytes seems adequate to cover that packet.  However, it might be worth
> having Jesse double-check that that's the correct encapsulation in the
> example

I am always in favor of more checking, especially by careful developers
like Jesse, but perhaps I gave an incorrect impression that this value
depends on kernel code?  It doesn't.  ODPUTIL_FLOW_KEY_BYTES is the
maximum number of bytes that the userspace function
odp_flow_key_from_flow() can write to a buffer.

Actually that's inadequately documented.  I'm appending a revised patch
that updates some comments too.

> Do you think it's worth having the total be a #define, and then do a
> build-time assertion that the total value is less than
> ODPUTIL_FLOW_KEY_BYTES?

Can you explain how that would be different?

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Tue, 15 May 2012 09:56:21 -0700
Subject: [PATCH] odp-util: Update ODPUTIL_FLOW_KEY_BYTES for current kernel flow format.

Before we submitted the kernel module upstream, we updated the flow format
by adding two fields to the description of packets with VLAN headers, but
we forgot to update ODPUTIL_FLOW_KEY_BYTES to reflect these changes.  The
result was that a maximum-length flow did not fit in the given space.

This fixes a crash processing IPv6 neighbor discovery packets with VLAN
headers received in a tunnel configured with key=flow or in_key=flow.

Also, update some comments to better describe the implications of
ODPUTIL_FLOW_KEY_BYTES (suggested by Justin).

Reported-by: Dan Wendlandt <dan at nicira.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 lib/odp-util.c |    7 +++++--
 lib/odp-util.h |   25 +++++++++++++++++++------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index aae5cc7..7e5c12f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1155,7 +1155,10 @@ ovs_to_odp_frag(uint8_t ovs_frag)
             : OVS_FRAG_TYPE_NONE);
 }
 
-/* Appends a representation of 'flow' as OVS_KEY_ATTR_* attributes to 'buf'. */
+/* Appends a representation of 'flow' as OVS_KEY_ATTR_* attributes to 'buf'.
+ *
+ * 'buf' must have at least ODPUTIL_FLOW_KEY_BYTES bytes of space, or be
+ * capable of being expanded to allow for that much space. */
 void
 odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow)
 {
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 7c9b588..45d7d3f 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -65,8 +65,16 @@ void format_odp_actions(struct ds *, const struct nlattr *odp_actions,
 int odp_actions_from_string(const char *, const struct shash *port_names,
                             struct ofpbuf *odp_actions);
 
-/* Upper bound on the length of a nlattr-formatted flow key.  The longest
- * nlattr-formatted flow key would be:
+/* The maximum number of bytes that odp_flow_key_from_flow() appends to a
+ * buffer.  This is the upper bound on the length of a nlattr-formatted flow
+ * key that ovs-vswitchd fully understands.
+ *
+ * OVS doesn't insist that ovs-vswitchd and the datapath have exactly the same
+ * idea of a flow, so therefore this value isn't necessarily an upper bound on
+ * the length of a flow key that the datapath can pass to ovs-vswitchd.
+ *
+ * The longest nlattr-formatted flow key appended by odp_flow_key_from_flow()
+ * would be:
  *
  *                         struct  pad  nl hdr  total
  *                         ------  ---  ------  -----
@@ -74,15 +82,20 @@ int odp_actions_from_string(const char *, const struct shash *port_names,
  *  OVS_KEY_ATTR_TUN_ID        8    --     4     12
  *  OVS_KEY_ATTR_IN_PORT       4    --     4      8
  *  OVS_KEY_ATTR_ETHERNET     12    --     4     16
+ *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8  (outer VLAN ethertype)
  *  OVS_KEY_ATTR_8021Q         4    --     4      8
- *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8
+ *  OVS_KEY_ATTR_ENCAP         0    --     4      4  (VLAN encapsulation)
+ *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8  (inner VLAN ethertype)
  *  OVS_KEY_ATTR_IPV6         40    --     4     44
  *  OVS_KEY_ATTR_ICMPV6        2     2     4      8
  *  OVS_KEY_ATTR_ND           28    --     4     32
  *  -------------------------------------------------
- *  total                                       144
+ *  total                                       156
+ *
+ * We include some slack space in case the calculation isn't quite right or we
+ * add another field and forget to adjust this value.
  */
-#define ODPUTIL_FLOW_KEY_BYTES 144
+#define ODPUTIL_FLOW_KEY_BYTES 200
 
 /* A buffer with sufficient size and alignment to hold an nlattr-formatted flow
  * key.  An array of "struct nlattr" might not, in theory, be sufficiently
-- 
1.7.2.5




More information about the dev mailing list