From d78be13bc9767d38affb8816a5f4cd80a0bca5d8 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Wed, 18 May 2011 16:40:21 -0700 Subject: [PATCH] ofproto-dpif: Fix bond accounting. Calls to bond_account() and bond_choose_output_slave() had different ideas for the vlan of a flow that did not have a tagged VLAN. The call to bond_choose_output_slave() passed OFP_VLAN_NONE in this case, the call to bond_account() passed 0. This meant that packets not on a VLAN weren't accounted properly, which typically caused bond/show to show "0 kB load" on active hashes. Obviously that broke rebalancing too. I've verified that this fixes accounting. I haven't directly verified that it fixes rebalancing, so it's possible that there is another issue too. Reported-by: Michael Mao --- ofproto/ofproto-dpif.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 087a8986..d3cb92b9 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2129,6 +2129,12 @@ facet_install(struct ofproto_dpif *p, struct facet *facet, bool zero_stats) } } +static int +vlan_tci_to_openflow_vlan(ovs_be16 vlan_tci) +{ + return vlan_tci != htons(0) ? vlan_tci_to_vid(vlan_tci) : OFP_VLAN_NONE; +} + static void facet_account(struct ofproto_dpif *ofproto, struct facet *facet, uint64_t extra_bytes) @@ -2138,6 +2144,7 @@ facet_account(struct ofproto_dpif *ofproto, const struct nlattr *a; tag_type dummy = 0; unsigned int left; + ovs_be16 vlan_tci; int vlan; total_bytes = facet->byte_count + extra_bytes; @@ -2165,14 +2172,32 @@ facet_account(struct ofproto_dpif *ofproto, if (!ofproto->has_bonded_bundles) { return; } + + /* This loop feeds byte counters to bond_account() for rebalancing to use + * as a basis. We also need to track the actual VLAN on which the packet + * is going to be sent to ensure that it matches the one passed to + * bond_choose_output_slave(). (Otherwise, we will account to the wrong + * hash bucket.) */ + vlan_tci = facet->flow.vlan_tci; NL_ATTR_FOR_EACH_UNSAFE (a, left, facet->actions, facet->actions_len) { - if (nl_attr_type(a) == ODP_ACTION_ATTR_OUTPUT) { - struct ofport_dpif *port; + struct ofport_dpif *port; + switch (nl_attr_type(a)) { + case ODP_ACTION_ATTR_OUTPUT: port = get_odp_port(ofproto, nl_attr_get_u32(a)); if (port && port->bundle && port->bundle->bond) { - bond_account(port->bundle->bond, &facet->flow, vlan, n_bytes); + bond_account(port->bundle->bond, &facet->flow, + vlan_tci_to_openflow_vlan(vlan_tci), n_bytes); } + break; + + case ODP_ACTION_ATTR_STRIP_VLAN: + vlan_tci = htons(0); + break; + + case ODP_ACTION_ATTR_SET_DL_TCI: + vlan_tci = nl_attr_get_be16(a); + break; } } } -- 2.30.2