On Mon, Mar 30, 2015 at 03:46:29PM -0700, Alex Wang wrote:
> This commit implements a prototype of Header Space Analysis of the
> OVS OpenFlow table.
>
> What It Does
> ------------
>
> 1. Dump all OpenFlow rules from a specified bridge.
> 2. Generate all-unmasked header space with specified input port number
> and start looking up from default starting table.
> 3. Traverse through all possible processing pipelines (i.e. OpenFlow
> rules cascaded with resubmit action) and conduct leak check or loop
> check.
> 3.1 A leak check prints all possible output ports with the actual input
> header space format.
> 3.2 A loop check checks if an OpenFlow rule is matched multiple time in
> the processing pipeline. It will tell if the loop is finite or
> infinite and print the loop path.
>
> How To Use
> ----------
>
> ovs-appctl hsa/detect-leak <bridge name> <OpenFlow port number>
> ovs-appctl hsa/detect-loop <bridge name> <OpenFlow port number>
>
> Limitation
> ----------
>
> - Only support a very limited set of OFPACT_* actions. And extending
> to accept more actions can be very complicated.
> - Single thread implementation, can take 20+ mins to run on a production
> setup with ~100 OpenFlow rules.
>
> Signed-off-by: Alex Wang <al...@nicira.com>
This is nice! The data structures are well documented, too--thanks for
that!
I get the following sparse warning:
../ofproto/ofproto-dpif-hsa.c:722:56: warning: incorrect type in argument 2
(different base types)
../ofproto/ofproto-dpif-hsa.c:722:56: expected restricted ofp_port_t
[usertype] port
../ofproto/ofproto-dpif-hsa.c:722:56: got unsigned long long [unsigned]
[usertype] port
The copyright notices should probably list 2015 and not the other years.
Because each element in move_map[] acts like an array of two elements,
would it be worthwhile actually making it a 2-d array, like "uint8_t
move_map[MOVE_MAP][2]"?
I am not sure how to interpret this:
* Assume all fields are in big endian.
hs_clone() copies some large members (match_hs, match_src, move_map)
that were just zeroed in the call to hs_create(). It might be worth
avoiding the double initialization.
I think that the use of list_insert() in the LIST_FOR_EACH loop in
hs_clone() will make the clone's list of constraints be in the order
opposite of the original header_space. That might be undesirable; you
could use list_push_back() to avoid it.
You could use xmemdup() in place of xmalloc() + memcpy(), in hs_clone().
In hsa_rule_compare(), the use of subtraction for comparison is risky if
the priorities could be large enough to overflow. Currently we don't
use priorities big enough for that to happen, but it's not nice to risk
it.
Here is one way to do it safely:
return r2->prio > r1->prio ? -1 : r2->prio < r1->prio;
It seems inefficient in hs_constraint_is_exact_match() to initialize two
flow_wildcards structures. I think you can use is_all_ones() and
is_all_zeros() instead.
I don't think the list_is_empty() check in hsa_finish() or
hsa_debug_dump_flows() is needed.
I'm not fully comfortable with assert-failing (killing ovs-vswitchd) if
the flow table has unsupported features in it. Also, even when HSA
doesn't kill the process due to assert-failing, it could still delay the
process by an arbitrary amount of time. Do you have an idea for a way
to keep curious admins from accidentally taking down their switch by
running an HSA command ("hey, what does this ovs-appctl command do?")?
I don't know why this is a CONST_CAST, in
hsa_move_map_set_field_by_subfield():
((uint8_t *) &value)[0] = CONST_CAST(uint8_t, src->field->id);
For initializing and examining a uint16_t that's divided into two
byte-size fields, as in hsa_move_map_set_field_by_subfield() or
hsa_move_map_apply_matched_field(), it's more usual to just use bitwise
operators.
There's a lot of code here and I only really scratched the surface for
understanding it.
Is it possible to write some tests?
Here are some suggestions as an incremental diff:
diff --git a/ofproto/ofproto-dpif-hsa.c b/ofproto/ofproto-dpif-hsa.c
index 7065c9b..38e6614 100644
--- a/ofproto/ofproto-dpif-hsa.c
+++ b/ofproto/ofproto-dpif-hsa.c
@@ -114,7 +114,7 @@ struct header_space {
* array[0], the latter value is at array[1].
*
* The value is 'BIT_UNSET' if the field has never been set, or
- * 'BIT_SET' if the field has been set by aciton.
+ * 'BIT_SET' if the field has been set by action.
* */
uint16_t move_map[MOVE_MAP_LEN];
@@ -217,7 +217,7 @@ hs_create(void)
hs->output = OFPP_NONE;
list_init(&hs->constraints);
- memset(hs->move_map, 0xff, MOVE_MAP_LEN * sizeof(uint16_t));
+ memset(hs->move_map, 0xff, MOVE_MAP_LEN * sizeof *hs->move_map);
hmap_init(&hs->matched_map);
return hs;
@@ -320,7 +320,7 @@ hsa_rule_swap(size_t a, size_t b, void *aux)
list_swap(list_at_position(rules, a), list_at_position(rules, b));
}
-/* This compares is implemented for sorting in descending order. */
+/* This comparison is implemented for sorting in descending order. */
static int
hsa_rule_compare(size_t a, size_t b, void *aux)
{
@@ -649,9 +649,9 @@ hsa_rule_apply_output_action__(struct header_space *hs,
ofp_port_t port,
case OFPP_LOCAL:
case OFPP_NORMAL:
/* Should not see such actions installed from controller. */
- ovs_assert(false);
+ OVS_NOT_REACHED();
case OFPP_CONTROLLER:
- /* Do not thing. */
+ /* Do nothing. */
break;
default:
if (port != hs->match_src.flow.in_port.ofp_port) {
@@ -1165,7 +1165,7 @@ hsa_mf_are_prereqs_ok(const struct mf_field *mf,
case MFP_ND:
case MFP_ND_SOLICIT:
case MFP_ND_ADVERT:
- ovs_assert(false);
+ OVS_NOT_REACHED();
}
OVS_NOT_REACHED();
}
@@ -1531,7 +1531,7 @@ hsa_unixctl_leak_detect(struct unixctl_conn *conn, int
argc OVS_UNUSED,
{
struct ds out = DS_EMPTY_INITIALIZER;
- op_type=HSA_LEAK_DETECT;
+ op_type = HSA_LEAK_DETECT;
hsa_do_analysis(&out, argv[1], argv[2]);
unixctl_command_reply(conn, ds_cstr(&out));
ds_destroy(&out);
@@ -1543,7 +1543,7 @@ hsa_unixctl_loop_detect(struct unixctl_conn *conn, int
argc OVS_UNUSED,
{
struct ds out = DS_EMPTY_INITIALIZER;
- op_type=HSA_LOOP_DETECT;
+ op_type = HSA_LOOP_DETECT;
hsa_do_analysis(&out, argv[1], argv[2]);
unixctl_command_reply(conn, ds_cstr(&out));
ds_destroy(&out);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
