[ovs-dev] [PATCH 2/2] ofproto-dpif: Fix bond accounting.

[Justin Pettit]

Looks good to me.

--Justin


On May 18, 2011, at 4:40 PM, Ben Pfaff wrote:

> 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 <mmao at nicira.com>
> ---
> ofproto/ofproto-dpif.c |   31 ++++++++++++++++++++++++++++---
> 1 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4dec05d..714cdf6 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2141,6 +2141,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)
> @@ -2150,6 +2156,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;
> @@ -2177,14 +2184,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;
>         }
>     }
> }
> -- 
> 1.7.4.4
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev