Looks like this will be pretty useful.
In facet_check_consistency()
Is facet_revalidate the appropriate coverage counter to increment?
When rule != facet->rule we don't initialize the dynamic string.
I think the code only warns when installable facets aren't installed,
not when un-installable facets are installed.
It may be worth printing the other possible values of
subfacet->key_fitness in case it's not OK but not TOO_LITTLE.
In ofproto_dpif_self_check():
I've found it useful to do appctl commands for all instances when one
isn't specified in the lacp, bond, and cfm module. It may be worth
doing it here as well, though I don't feel strongly about it.
Ethan
On Tue, Dec 27, 2011 at 13:28, Ben Pfaff <b...@nicira.com> wrote:
> One of the major tasks of ofproto-dpif is to translate OpenFlow
> actions into "ODP" datapath actions. These translations are essentially
> a cache that requires revalidation when certain state changes occur. For
> best performance it's important to revalidate flows only when necessary,
> so from time to time Open vSwitch has gotten this wrong, which meant that
> stale flows could persist in the kernel and cause surprising behavior.
>
> This commit implements a simple "self check": every trip through the
> Open vSwitch main loop randomly chooses one flow entry and checks that
> its actions have been correctly translated. If not, Open vSwitch logs
> the details of the problem. This should help find problems more
> quickly in the future.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 163
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 163 insertions(+), 0 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index f131ebd..924cd69 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -313,6 +313,7 @@ static struct facet *facet_find(struct ofproto_dpif *,
> const struct flow *);
> static struct facet *facet_lookup_valid(struct ofproto_dpif *,
> const struct flow *);
> static bool facet_revalidate(struct facet *);
> +static bool facet_check_consistency(struct facet *);
> static bool execute_controller_action(struct ofproto_dpif *,
> const struct flow *,
> const struct nlattr *odp_actions,
> @@ -373,6 +374,8 @@ static struct subfacet *subfacet_find(struct ofproto_dpif
> *,
> const struct nlattr *key, size_t
> key_len);
> static void subfacet_destroy(struct subfacet *);
> static void subfacet_destroy__(struct subfacet *);
> +static void subfacet_get_key(struct subfacet *, struct odputil_keybuf *,
> + struct ofpbuf *key);
> static void subfacet_reset_dp_stats(struct subfacet *,
> struct dpif_flow_stats *);
> static void subfacet_update_time(struct subfacet *, long long int used);
> @@ -814,6 +817,19 @@ run(struct ofproto *ofproto_)
> }
> }
>
> + /* Check the consistency of a random facet, to aid debugging. */
> + if (!hmap_is_empty(&ofproto->facets) && !ofproto->need_revalidate) {
> + struct facet *facet;
> +
> + facet = CONTAINER_OF(hmap_random_node(&ofproto->facets),
> + struct facet, hmap_node);
> + if (!tag_set_intersects(&ofproto->revalidate_set, facet->tags)) {
> + if (!facet_check_consistency(facet)) {
> + ofproto->need_revalidate = true;
> + }
> + }
> + }
> +
> return 0;
> }
>
> @@ -3340,6 +3356,116 @@ facet_lookup_valid(struct ofproto_dpif *ofproto,
> const struct flow *flow)
> return facet;
> }
>
> +static bool
> +facet_check_consistency(struct facet *facet)
> +{
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 15);
> +
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> +
> + struct rule_dpif *rule;
> + struct subfacet *subfacet;
> + bool ok;
> +
> + COVERAGE_INC(facet_revalidate);
> +
> + /* Check the rule for consistency. */
> + rule = rule_dpif_lookup(ofproto, &facet->flow, 0);
> + if (!rule) {
> + if (!VLOG_DROP_WARN(&rl)) {
> + char *s = flow_to_string(&facet->flow);
> + VLOG_WARN("%s: facet should not exist", s);
> + free(s);
> + }
> + return false;
> + } else if (rule != facet->rule) {
> + struct ds s;
> +
> + flow_format(&s, &facet->flow);
> + ds_put_format(&s, ": facet associated with wrong rule (was "
> + "table=%"PRIu8",", facet->rule->up.table_id);
> + cls_rule_format(&facet->rule->up.cr, &s);
> + ds_put_format(&s, ") (should have been table=%"PRIu8",",
> + rule->up.table_id);
> + cls_rule_format(&rule->up.cr, &s);
> + ds_put_char(&s, ')');
> +
> + VLOG_WARN("%s", ds_cstr(&s));
> + ds_destroy(&s);
> +
> + ok = false;
> + } else {
> + ok = true;
> + }
> +
> + /* Check the datapath actions for consistency. */
> + LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
> + struct action_xlate_ctx ctx;
> + struct ofpbuf *odp_actions;
> + bool actions_changed;
> + bool should_install;
> +
> + action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> + subfacet->initial_tci, NULL);
> + odp_actions = xlate_actions(&ctx, rule->up.actions,
> + rule->up.n_actions);
> +
> + should_install = (ctx.may_set_up_flow
> + && subfacet->key_fitness != ODP_FIT_TOO_LITTLE);
> + if (!should_install && !subfacet->installed) {
> + /* The actions for uninstallable flows may vary from one packet
> to
> + * the next, so don't compare the actions. */
> + goto next;
> + }
> +
> + actions_changed = (subfacet->actions_len != odp_actions->size
> + || memcmp(subfacet->actions, odp_actions->data,
> + subfacet->actions_len));
> + if (should_install != subfacet->installed || actions_changed) {
> + struct odputil_keybuf keybuf;
> + struct ofpbuf key;
> + struct ds s;
> +
> + ok = false;
> +
> + ds_init(&s);
> + subfacet_get_key(subfacet, &keybuf, &key);
> + odp_flow_key_format(key.data, key.size, &s);
> +
> + ds_put_cstr(&s, ": inconsistency in subfacet");
> + if (should_install != subfacet->installed) {
> + ds_put_format(&s, " (should%s have been installed)",
> + should_install ? "" : " not");
> + ds_put_format(&s, " (may_set_up_flow=%s, fitness=%s)",
> + ctx.may_set_up_flow ? "true" : "false",
> + subfacet->key_fitness == ODP_FIT_TOO_LITTLE
> + ? "too_little" : "OK");
> + }
> + if (actions_changed) {
> + ds_put_cstr(&s, " (actions were: ");
> + format_odp_actions(&s, subfacet->actions,
> + subfacet->actions_len);
> + ds_put_cstr(&s, ") (correct actions: ");
> + format_odp_actions(&s, odp_actions->data,
> + odp_actions->size);
> + ds_put_char(&s, ')');
> + } else {
> + ds_put_cstr(&s, " (actions: ");
> + format_odp_actions(&s, subfacet->actions,
> + subfacet->actions_len);
> + ds_put_char(&s, ')');
> + }
> + VLOG_WARN("%s", ds_cstr(&s));
> + ds_destroy(&s);
> + }
> +
> + next:
> + ofpbuf_delete(odp_actions);
> + }
> +
> + return ok;
> +}
> +
> /* Re-searches the classifier for 'facet':
> *
> * - If the rule found is different from 'facet''s current rule, moves
> @@ -5812,6 +5938,41 @@ ofproto_dpif_unclog(struct unixctl_conn *conn
> OVS_UNUSED, int argc OVS_UNUSED,
> }
>
> static void
> +ofproto_dpif_self_check(struct unixctl_conn *conn,
> + int argc OVS_UNUSED, const char *argv[],
> + void *aux OVS_UNUSED)
> +{
> + const char *dpname = argv[1];
> + struct ofproto_dpif *ofproto;
> + struct facet *facet;
> + char *reply;
> + int errors;
> +
> + ofproto = ofproto_dpif_lookup(dpname);
> + if (!ofproto) {
> + unixctl_command_reply(conn, 501, "Unknown ofproto (use ofproto/list "
> + "for help)");
> + return;
> + }
> +
> + errors = 0;
> + HMAP_FOR_EACH (facet, hmap_node, &ofproto->facets) {
> + if (!facet_check_consistency(facet)) {
> + errors++;
> + }
> + }
> + if (errors) {
> + ofproto->need_revalidate = true;
> + }
> +
> + reply = (errors
> + ? xasprintf("%s: self-check failed (%d errors)", dpname, errors)
> + : xasprintf("%s: self-check passed", dpname));
> + unixctl_command_reply(conn, 200, reply);
> + free(reply);
> +}
> +
> +static void
> ofproto_dpif_unixctl_init(void)
> {
> static bool registered;
> @@ -5832,6 +5993,8 @@ ofproto_dpif_unixctl_init(void)
> ofproto_dpif_clog, NULL);
> unixctl_command_register("ofproto/unclog", "", 0, 0,
> ofproto_dpif_unclog, NULL);
> + unixctl_command_register("ofproto/self-check", "bridge", 1, 1,
> + ofproto_dpif_self_check, NULL);
> }
>
> /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
