On Tue, Mar 11, 2014 at 04:56:21PM -0700, Andy Zhou wrote:
> Infrastructure to enable megaflow support for bond ports using
> recirculation. This patch adds the following features:
> * Generate RECIRC action when bond can benefit from recirculation.
> * Populate post recirculation rules in table 254.
> * Uses post recirculation rules for bond rebalancing
> * Logic to detect whether data path supports recirculation.
>
> Bond port using recirculation is currently turned off by always
> detect the data path as not supporting recirculation.
>
> Signed-off-by: Andy Zhou <az...@nicira.com>
I have a lot of comments below, but I guess that none of them are too
bad. I'm very positive about this patch.
Important Stuff
===============
I think that this makes every translation that outputs to NORMAL check
and update all 256 of the bond's rules in the ofproto. That's
expensive, was that really your intent?
In rule_dpif_lookup(), it's unsafe to use a pointer to 'recirc_flow'
outside the block that declares it; it needs to be declared at
function level. But I'm not sure I agree with what's going on there
at a higher level, or maybe I just don't understand it. It looks to
me like this always replaces the metadata by the recirc_id as part of
the lookup. Why not just initialize the metadata from the recirc_id
as part of receiving the packet, instead of doing it as part of the
lookup? Not sure of the rationale, in any case.
It makes me nervous to see bond.c retaining a pointer to the rules
that it creates, especially when the table is no longer being marked
read-only (so that "ovs-ofctl del-flows br0 table=254" could blow
these flows away). Can we take a reference to the rules, or mark the
table read-only again (and add some kind of override for use by
internal flow_mods), or otherwise make this safer?
Regarding this:
+ } else {
+ pr_op->pr_rule = (struct rule *)(rule_dpif); /* Can we do
better ? XXXX */
+ }
one way to make it safer would be to have
ofproto_dpif_add_internal_flow() produce a "struct rule" instead of a
rule_dpif.
Does update_recirc_rules() properly update rules following a
rebalancing? It looks to me like add_pr_rule() never updates the
output port if it finds a matching rule, so I'm not sure whether the
new port assignments would properly take effect.
It looks like update_recirc_rules() adds rules with priority
RECIRC_RULE_PRIORITY but deletes them with priority 0. I don't think
that works.
I'd like to delete this line from open_dpif_backer(), as long as the
routine for detecting kernel support is OK:
backer->enable_recirc = false; /* XXX Do not enable recirc yet */
Minor Stuff
===========
I see dpif-netdev makes recirculation optional via ->enable_recirc,
but that variable is initially true and I see no way to change it to
false. Is that a feature for later?
It seems pretty awkward to add a new 'enabled_recirc' parameter to
odp_flow_key_from_flow() and odp_flow_key_from_mask(). Are these also
only for testing, or do they have larger utility?
I just noticed that struct ovs_action_recirc has two __be32 members
that don't seem to naturally be in network byte order. Why are they
in network byte order?
It's unnecessary to set resubmit->ofpact.compat in
add_internal_flows(), so I wouldn't bother. (See the large comment on
struct ofpact describing 'compat', in particular, "If the action
didn't originate from OpenFlow, then setting 'compat' to zero should
be fine: code to translate the ofpact to OpenFlow must tolerate this
case.)
OFTABLE_READONLY is now unused. I don't know whether it is worth
keeping it. Maybe it is useful to some hardware implementation,
though.
The comment on rule_dpif_lookup() should describe what the new
'table_id' parameter is for.
Style
=====
Guru just checked in a commit that changes DELETE to DEL in some other
file because MSVC has something named DELETE, so you might want to use
DEL in bond.c to save fixing it later.
Missing {}, and unneeded (), here in update_recirc_rules():
+ if ((bond->hash == NULL) || (!bond->recirc_id))
+ return;
In bond_reconfigure(), we don't usually put spaces around ->:
+ } else if (bond -> recirc_id) {
Here, please put the function name at the beginning of a line:
+static void bond_entry_acount(struct bond_entry *entry, uint64_t rule_tx_bytes)
Spacing looks odd here:
+ for (i =0; i< BOND_MASK + 1; i++) {
The parentheses are oddly doubled here in bond_may_recirc():
+ if ((bond->balance == BM_TCP)) {
This appears to delete the blank line between
bond_slave_from_bal_node() and log_bals(), but it shouldn't.
Extra space before comma in bond_print_details():
+ may_recirc = bond_may_recirc(bond, &recirc_id , NULL);
The large comment in bond.h should use the same format as other large
comments in OVS, with * lined up at the start of a line.
In compose_output_action__(), the cast here shouldn't be necessary:
+ act_recirc = (struct ovs_action_recirc *)
+ nl_msg_put_unspec_uninit(&ctx->xout->odp_actions,
+ OVS_ACTION_ATTR_RECIRC, sizeof *act_recirc);
Also, struct ovs_action_attr_recirc has holes in it (3 bytes following
hash_alg), so maybe we should use nl_msg_put_unspec_zero() to zero
them for reproducibility.
Extra space in xlate_ofpact_resubmit() here:
+ if (ctx->table_id >= ctx->xbridge->n_visible_tables ) {
Extra () here in open_dpif_backer():
+ recirc_id_size = (backer->enable_recirc) ? RECIRC_ID_SIZE : 0;
In rule_dpif_lookup(), s/inernal/internal/:
+ /* Reflect recirc_id into metadata to match with inernal
It looks like rule_get_flags() and ofproto_rule_is_hidden() are being
moved around without actually being changed. If so, it would be better to
leave them as-is.
In update_recirc_rules(), we don't usually include a new-line at the
end of a message:
+ VLOG_ERR("Bond: Failed to add post recirculation flow %s \n",
+ match_to_string(&pr_op->match, 0));
Oh, also that call to match_to_string() will leak memory. I see both
of these in the ADD and the DELETE cases.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
