[ovs-dev] [PATCH] lib/flow.h: Improve struct miniflow comment and definition.

Ben Pfaff blp at nicira.com
Wed Aug 27 00:13:12 UTC 2014


On Wed, Aug 27, 2014 at 12:11:34AM +0000, Saurabh Shah wrote:
> > -----Original Message-----
> > From: Ben Pfaff [mailto:blp at nicira.com]
> > Sent: Tuesday, August 26, 2014 4:56 PM
> > To: Saurabh Shah
> > Cc: Jarno Rajahalme; dev at openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH] lib/flow.h: Improve struct miniflow comment
> > and definition.
> > 
> > On Tue, Aug 26, 2014 at 11:32:55PM +0000, Saurabh Shah wrote:
> > >
> > >
> > > >Miniflows can nowadays be dynamically allocated to different inline
> > > >sizes, as done by lib/classifier.c, but this had not been documented
> > > >at the struct miniflow definition.
> > > >
> > > >Also, MINI_N_INLINE had a different value for 32-bit and 64-bit
> > > >builds due to a historical reason.  Now we use 8 for both.
> > > >
> > > >Finally, use change the storage type of 'values_inline' to uint8_t,
> > > >as uint64_t looks kind of wide for a boolean, even though we intend
> > > >the bit be carved out from the uint64_t where 'map' resides.
> > > >
> > > >Suggested-by: Ben Pfaff <blp at nicira.com>
> > > >Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> > > >---
> > > > lib/flow.c |    3 ++-
> > > > lib/flow.h |   16 +++++++++++++---
> > > > 2 files changed, 15 insertions(+), 4 deletions(-)
> > > >
> > > >diff --git a/lib/flow.c b/lib/flow.c
> > > >index bef2f27..d322ac2 100644
> > > >--- a/lib/flow.c
> > > >+++ b/lib/flow.c
> > > >@@ -1715,7 +1715,8 @@ miniflow_clone_inline(struct miniflow *dst,
> > > >const struct miniflow *src,
> > > > /* Initializes 'dst' with the data in 'src', destroying 'src'.
> > > >  * The caller must eventually free 'dst' with miniflow_destroy().
> > > >  * 'dst' must be regularly sized miniflow, but 'src' can have
> > > >- * larger than default inline values. */
> > > >+ * storage for more than the default MINI_N_INLINE inline
> > > >+ * values. */
> > > > void
> > > > miniflow_move(struct miniflow *dst, struct miniflow *src)  { diff
> > > >--git a/lib/flow.h b/lib/flow.h index 3b8d24d..2891d60 100644
> > > >--- a/lib/flow.h
> > > >+++ b/lib/flow.h
> > > >@@ -346,7 +346,10 @@ bool flow_equal_except(const struct flow *a,
> > > >const struct flow *b,  ?
> > > > /* Compressed flow. */
> > > >
> > > >-#define MINI_N_INLINE (sizeof(void *) == 4 ? 7 : 8)
> > > >+/* Number of 32-bit words present in struct miniflow. */ #define
> > > >+MINI_N_INLINE 8
> > > >+
> > > >+/* Maximum number of 32-bit words supported. */
> > > > BUILD_ASSERT_DECL(FLOW_U32S <= 63);
> > > >
> > > > /* A sparse representation of a "struct flow".
> > > >@@ -367,6 +370,11 @@ BUILD_ASSERT_DECL(FLOW_U32S <= 63);
> > > >  * the first element of the values array, the next 1-bit is in the
> > > >next array
> > > >  * element, and so on.
> > > >  *
> > > >+ * MINI_N_INLINE is the default number of inline words.  When a
> > > >+ miniflow
> > > >is
> > > >+ * dynamically allocated the actual amount of inline storage may be
> > > >different.
> > > >+ * In that case 'inline_values' contains storage at least for the
> > > >+ number
> > > >+ * of words indicated by 'map' (one uint32_t for each 1-bit in the map).
> > > >+ *
> > > >  * Elements in values array are allowed to be zero.  This is useful
> > > >for "struct
> > > >  * minimatch", for which ensuring that the miniflow and minimask
> > > >members have
> > > >  * same 'map' allows optimization.  This allowance applies only to a
> > > >miniflow @@ -375,12 +383,14 @@ BUILD_ASSERT_DECL(FLOW_U32S <=
> > 63);
> > > >  */
> > > > struct miniflow {
> > > >     uint64_t map:63;
> > > >-    uint64_t values_inline:1;
> > > >+    uint8_t values_inline:1;
> > > >     union {
> > > >         uint32_t *offline_values;
> > > >-        uint32_t inline_values[MINI_N_INLINE];
> > > >+        uint32_t inline_values[MINI_N_INLINE]; /* Minimum inline
> > > >+ size. */
> > > >     };
> > > > };
> > > >+BUILD_ASSERT_DECL(sizeof(struct miniflow)
> > > >+                  == sizeof(uint64_t) + MINI_N_INLINE *
> > > >+sizeof(uint32_t))
> > >
> > > This fails on windows:
> > >
> > > c:\mingw\msys\1.0\home\administrator\work\ro\ovs\lib\hash.h(148) :
> > > warning
> > > C4028
> > > : formal parameter 2 different from declaration
> > > c:\mingw\msys\1.0\home\administrator\work\ro\ovs\lib\flow.h(393) :
> > > error
> > > C2034:
> > > 'build_assert_failed' : type of bit field too small for number of bits
> > 
> > Does it fail because of the uint64_t -> uint8_t change or because of the
> > change to MINI_N_INLINE?  (If it's the former let's just drop that
> > change.)
> 
> I reverted uint8_t -> uint64_t and was able to move past compile flow.h.

OK, so much for that idea.  Jarno, let's stick with uint64_t then?



More information about the dev mailing list