On Nov 19, 2013, at 2:49 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Nov 19, 2013 at 11:20:37AM -0800, Jarno Rajahalme wrote:
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> miniflow_get_map_in_range() is longer than I would ordinarily write as
> inline in a header file. Do you have a special reason to do that?
>
Just that contains code that was previously open coded in
minimatch_hash_range(), which is in match.c rather than flow.c.
However, it is used at flow set-up time rather than at lookup, so
inlining it doesn’t really matter. I moved mini flow_get_map_in_range()
to flow.c.
> I spent some time studying find_match_wc(). I think that the time for
> 'rl != rule' is equivalent to 'rule == NULL', because once we've
> narrowed the possibilities down to a single rule, that rule can't
> change as we look further (if it did, that would indicate a bug, I
> believe). Once I figured that out, it seemed that the logic could be
> simplified slightly, at least from my point of view.
>
> It also seemed like the relationship between find_match_wc() and
> find_match() could be simplified a bit.
>
> So here's what I suggest folding in, I'm curious to see what you
> think.
You’re right on all counts, and your changes are elegant. Thanks for a
thorough review :-)
I’m folding this in and pushing the result with the following incremental on
top of yours,
Jarno
diff --git a/lib/classifier.c b/lib/classifier.c
index f94cb96..33ade96 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -820,7 +820,7 @@ update_subtables_after_removal(struct classifier *cls,
}
}
-static struct cls_rule *
+static inline struct cls_rule *
find_match(const struct cls_subtable *subtable, const struct flow *flow,
uint32_t hash)
{
diff --git a/lib/flow.c b/lib/flow.c
index 979eab5..c6683a5 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -686,6 +686,27 @@ flow_wildcards_fold_minimask(struct flow_wildcards *wc,
flow_union_with_miniflow(&wc->masks, &mask->masks);
}
+inline uint64_t
+miniflow_get_map_in_range(const struct miniflow *miniflow,
+ uint8_t start, uint8_t end, const uint32_t **data)
+{
+ uint64_t map = miniflow->map;
+ uint32_t *p = miniflow->values;
+
+ if (start > 0) {
+ uint64_t msk = (UINT64_C(1) << start) - 1; /* 'start' LSBs set */
+ p += count_1bits(map & msk); /* Skip to start. */
+ map &= ~msk;
+ }
+ if (end < FLOW_U32S) {
+ uint64_t msk = (UINT64_C(1) << end) - 1; /* 'end' LSBs set */
+ map &= msk;
+ }
+
+ *data = p;
+ return map;
+}
+
/* Fold minimask 'mask''s wildcard mask into 'wc's wildcard mask
* in range [start, end). */
void
diff --git a/lib/flow.h b/lib/flow.h
index 3040f7b..5e78073 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -30,7 +30,6 @@
struct dpif_flow_stats;
struct ds;
struct flow_wildcards;
-struct miniflow;
struct minimask;
struct ofpbuf;
@@ -388,6 +387,8 @@ bool miniflow_equal_flow_in_minimask(const struct miniflow
*a,
uint32_t miniflow_hash(const struct miniflow *, uint32_t basis);
uint32_t miniflow_hash_in_minimask(const struct miniflow *,
const struct minimask *, uint32_t basis);
+uint64_t miniflow_get_map_in_range(const struct miniflow *, uint8_t start,
+ uint8_t end, const uint32_t **data);
/* Compressed flow wildcards. */
@@ -418,27 +419,6 @@ uint32_t minimask_hash(const struct minimask *, uint32_t
basis);
bool minimask_has_extra(const struct minimask *, const struct minimask *);
bool minimask_is_catchall(const struct minimask *);
-static inline uint64_t
-miniflow_get_map_in_range(const struct miniflow *miniflow,
- uint8_t start, uint8_t end, const uint32_t **data)
-{
- uint64_t map = miniflow->map;
- uint32_t *p = miniflow->values;
-
- if (start > 0) {
- uint64_t msk = (UINT64_C(1) << start) - 1; /* 'start' LSBs set */
- p += count_1bits(map & msk); /* Skip to start. */
- map &= ~msk;
- }
- if (end < FLOW_U32S) {
- uint64_t msk = (UINT64_C(1) << end) - 1; /* 'end' LSBs set */
- map &= msk;
- }
-
- *data = p;
- return map;
-}
-
/* Returns the value of the OpenFlow 1.1+ "metadata" field in 'flow'. */
static inline ovs_be64
miniflow_get_metadata(const struct miniflow *flow)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
