On Fri, Jul 17, 2015 at 08:51:47AM -0700, Ben Pfaff wrote:
> On Thu, Jul 16, 2015 at 05:47:00PM -0700, Jarno Rajahalme wrote:
> > Use two maps in miniflow to allow for expansion of struct flow past
> > 512 bytes. We now have one map for tunnel related fields, and another
> > for the rest of the packet metadata and actual packet header fields.
> > This split has the benefit that for non-tunneled packets the overhead
> > should be minimal.
> >
> > Some miniflow utilities now exist in two variants, new ones operating
> > over all the data, and the old ones operating only on a single 64-bit
> > map at a time. The old ones require doubling of code but should
> > execute faster, so those are used in the datapath and classifier's
> > lookup path.
> >
> > Signed-off-by: Jarno Rajahalme <[email protected]>
>
> This version passes tests and does not cause any sparse warnings. Thank
> you!
>
> I am a little surprised to see two named bitmaps instead of an array of
> two elements. Names are nice for some things, but other times it is
> convenient to be able to use loops to iterate, and of course arrays
> generalize better.
>
> This change to dpif-netdev.c looks like an independent bug fix to me:
>
> @@ -1892,10 +1913,11 @@ dpif_netdev_mask_from_nlattrs(const struct nlattr
> *key, uint32_t key_len,
> memset(mask, 0x0, sizeof *mask);
>
> for (id = 0; id < MFF_N_IDS; ++id) {
> /* Skip registers and metadata. */
> if (!(id >= MFF_REG0 && id < MFF_REG0 + FLOW_N_REGS)
> + && !(id >= MFF_XREG0 && id < MFF_XREG0 + FLOW_N_XREGS)
> && id != MFF_METADATA) {
> const struct mf_field *mf = mf_from_id(id);
> if (mf_are_prereqs_ok(mf, flow)) {
> mf_mask_field(mf, mask);
> }
>
> Acked-by: Ben Pfaff <[email protected]>
Oh, here are some comment suggestions:
diff --git a/lib/classifier-private.h b/lib/classifier-private.h
index c4c6ce9..3a150ab 100644
--- a/lib/classifier-private.h
+++ b/lib/classifier-private.h
@@ -226,7 +226,11 @@ struct trie_node {
* These are only used by the classifier, so place them here to allow
* for better optimization. */
-/* TODO: Ensure that 'start' and 'end' are compile-time constants. */
+/* Initializes 'map->tnl_map' and 'map->pkt_map' with a subset of 'miniflow'
+ * that includes only the portions with u64-offset 'i' such that start <= i <
+ * end. Does not copy any data from 'miniflow' to 'map'.
+ *
+ * TODO: Ensure that 'start' and 'end' are compile-time constants. */
static inline unsigned int /* offset */
miniflow_get_map_in_range(const struct miniflow *miniflow,
uint8_t start, uint8_t end, struct miniflow *map)
diff --git a/lib/flow.h b/lib/flow.h
index 85a9792..96aa4aa 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -393,8 +393,8 @@ BUILD_ASSERT_DECL(FLOW_U64S - FLOW_TNL_U64S <= 64);
* 0-bit indicates that the corresponding uint64_t is zero, each 1-bit that it
* *may* be nonzero (see below how this applies to minimasks).
*
- * The values indicated by 'tnl_map' and 'pkt_map' always follow the 'map' in
- * memory. The user of the miniflow is responsible for always having enough
+ * The values indicated by 'tnl_map' and 'pkt_map' always follow the miniflow
+ * in memory. The user of the miniflow is responsible for always having enough
* storage after the struct miniflow corresponding to the number of 1-bits in
* maps.
*
@@ -409,7 +409,9 @@ BUILD_ASSERT_DECL(FLOW_U64S - FLOW_TNL_U64S <= 64);
struct miniflow {
uint64_t tnl_map;
uint64_t pkt_map;
- /* uint64_t values[]; Storage follows 'map' in memory. */
+ /* Followed by:
+ * uint64_t values[n];
+ * where 'n' is miniflow_n_values(miniflow). */
};
BUILD_ASSERT_DECL(sizeof(struct miniflow) == 2 * sizeof(uint64_t));
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
