Thanks a lot for the detailed review. I will clean up this patch series
based on the feedback.
On Tue, Mar 18, 2014 at 4:25 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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?
>
Yes, I tried to keep things simple for now. I will find a better way to do
this.
>
> 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.
>
I tried to keep flow as it was received from the datapath, so it won't be
confusing when we dump it out.
>
> 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?
>
> How about make it controller read-only? ovs-ofctl is a debugging tool.
> 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.
>
Good idea.
>
> 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 seems there is still a bug there. Will debug it.
>
> 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.
>
Will fix it.
>
> 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 */
>
> Will remove it.
>
> 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?
>
I plan to add a unixctl command to change it so I can write test cases
against it.
I have been manually setting it during development.
>
> 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?
>
Will fix it. They should be in host 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.)
>
Did not know about this. Thanks for point it out. Will fix.
>
> OFTABLE_READONLY is now unused. I don't know whether it is worth
> keeping it. Maybe it is useful to some hardware implementation,
> though.
>
Keep it and interpret it as controller read only?
>
> The comment on rule_dpif_lookup() should describe what the new
> 'table_id' parameter is for.
>
Will do.
>
>
> Style
> =====
>
> Will fix them.
> 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
