[ovs-dev] Fwd: [PATCH] bond: Unify hash functions in hash action and entry lookup.
Darrell Ball
dball at vmware.com
Sat Jul 22 21:02:08 UTC 2017
-----Original Message-----
From: <ovs-dev-bounces at openvswitch.org> on behalf of Andy Zhou <azhou at ovn.org>
Date: Friday, July 21, 2017 at 2:17 PM
To: "<dev at openvswitch.org>" <dev at openvswitch.org>
Subject: [ovs-dev] Fwd: [PATCH] bond: Unify hash functions in hash action and entry lookup.
Add dev mailing list. It got dropped by accident.
---------- Forwarded message ----------
From: Andy Zhou <azhou at ovn.org>
Date: Fri, Jul 21, 2017 at 2:14 PM
Subject: Re: [PATCH] bond: Unify hash functions in hash action and entry lookup.
To: Ilya Maximets <i.maximets at samsung.com>
As it turns out, we can go even further:
Notice that lookup_bond_entry() is only called with the code path of BM_SLB.
and bond_hash() is only called by lookup_bond_entry().
I think we can just absorb the logic of lookup_bond_entry() into
choose_output_slave()
and remove bond_hash() all together. What do you think?
On Fri, Jul 21, 2017 at 1:06 PM, Andy Zhou <azhou at ovn.org> wrote:
> On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maximets at samsung.com> wrote:
>> 'lookup_bond_entry' currently uses 'flow_hash_symmetric_l4' while
>> OVS_ACTION_ATTR_HASH uses 'flow_hash_5tuple'. This may lead to
>> inconsistency in slave choosing for the new flows. In general,
>> there is no point to unify hash functions, because it's not
>> required for correct work, but it's logically wrong to use
>> different hash functions there.
>>
>> Unfortunately we're not able to use RSS hash here, because we have
>> no packet at this point, but we may reduce inconsistency by using
>> 'flow_hash_5tuple' instead of 'flow_hash_symmetric_l4' because
>> symmetric quality is not needed.
>>
>> 'flow_hash_symmetric_l4' was used previously just because there
>> was no other implemented hash function at the moment. Now we
>> have 5tuple hash and may replace the old function.
[Darrell]
What other load balance option is available to do load balancing of L2 packets (non-IP)
‘at the same time’ as IPv4/6 packets for bonds ?
Unless there is another, I am not sure giving up the load balancing of L2 packets is desirable.
There would be a loss of feature functionality with this patch.
A bond at a gateway (one of the most common use cases) could handle many CFM
sessions, for example and dropping L2 fields from the hash sends all L2 packets to a
single interface of a bond (single point of failure).
The algorithm flow_hash_symmetric_l4 includes L2 fields (macs and vlans)
in addition to IPv4/6 and L4 fields, which means it can load balance L2 packets (eg CFM)
in addition to IPv4/6 packets.
We have documented that L2 load balancing is included in balance-tcp, which at the very
least would need to change, assuming we thought such a change had more advantages than disadvantages.
http://openvswitch.org/support/dist-docs/ovs-vswitchd.conf.db.5.pdf
“The following modes require the upstream switch to support 802.3ad with successful LACP negotiation. If
LACP negotiation fails and other-config:lacp-fallback-ab is true, then active−backup mode is used:
balance−tcp
Balances flows among slaves based on L2, L3, and L4 protocol information such as destination
MAC address, IP address, and TCP port.”
What is the overall time cost savings in the scope of the whole code pipeline for flow creation, not
just the hash function itself (as mentioned in the commit message) ?
This is not a per packet cost; it is per flow cost. Since this patch removes feature content in lieu of
some performance gain, I think it might be good to have some pipeline performance measurements to
make a decision whether it is worth it.
>>
>> 'flow_hash_5tuple' is preferable solution because it in 2 - 8 times
>> (depending on the flow) faster than symmetric function.
>> So, this change will also speed up handling of the new flows and
>> statistics accounting.
>>
>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>> ---
>> ofproto/bond.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/ofproto/bond.c b/ofproto/bond.c
>> index cb25a1d..72b373c 100644
>> --- a/ofproto/bond.c
>> +++ b/ofproto/bond.c
>> @@ -1746,12 +1746,10 @@ static unsigned int
>> bond_hash_tcp(const struct flow *flow, uint16_t vlan, uint32_t basis)
>> {
>> struct flow hash_flow = *flow;
>> +
>> hash_flow.vlans[0].tci = htons(vlan);
>>
>> - /* The symmetric quality of this hash function is not required, but
>> - * flow_hash_symmetric_l4 already exists, and is sufficient for our
>> - * purposes, so we use it out of convenience. */
>> - return flow_hash_symmetric_l4(&hash_flow, basis);
>> + return flow_hash_5tuple(&hash_flow, basis);
>> }
>>
>> static unsigned int
>> --
>> 2.7.4
>>
>
> llya, thanks for the patch. I agree with the patch comments, but I think
> it can further by remove the bond_hash_tcp() function.
> This should further speed up hashing by avoid copying flow.
>
> What do you think? Would you please consider and evaluate this approach?
>
> While at it, we can probably get rid of bond_hash_src() also. It
> can be a separate patch or fold into this one -- up to you.
>
> Thanks!
>
>
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 21370b5f9a47..eb965b04cd3a 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -177,8 +177,6 @@ static void bond_choose_active_slave(struct bond *)
> OVS_REQ_WRLOCK(rwlock);
> static unsigned int bond_hash_src(const struct eth_addr mac,
> uint16_t vlan, uint32_t basis);
> -static unsigned int bond_hash_tcp(const struct flow *, uint16_t vlan,
> - uint32_t basis);
> static struct bond_entry *lookup_bond_entry(const struct bond *,
> const struct flow *,
> uint16_t vlan)
> @@ -1742,24 +1740,12 @@ bond_hash_src(const struct eth_addr mac,
> uint16_t vlan, uint32_t basis)
> }
>
> static unsigned int
> -bond_hash_tcp(const struct flow *flow, uint16_t vlan, uint32_t basis)
> -{
> - struct flow hash_flow = *flow;
> - hash_flow.vlans[0].tci = htons(vlan);
> -
> - /* The symmetric quality of this hash function is not required, but
> - * flow_hash_symmetric_l4 already exists, and is sufficient for our
> - * purposes, so we use it out of convenience. */
> - return flow_hash_symmetric_l4(&hash_flow, basis);
> -}
> -
> -static unsigned int
> bond_hash(const struct bond *bond, const struct flow *flow, uint16_t vlan)
> {
> ovs_assert(bond->balance == BM_TCP || bond->balance == BM_SLB);
>
> return (bond->balance == BM_TCP
> - ? bond_hash_tcp(flow, vlan, bond->basis)
> + ? flow_hash_5tuple(flow, bond->basis)
> : bond_hash_src(flow->dl_src, vlan, bond->basis));
> }
_______________________________________________
dev mailing list
dev at openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=8SBQ9dIcqXDTjo3cocON-of1LicoVhkYv9z1Db6OxdA&s=wfY6zju7gQT347GKnjXBo4cvS5lS2Qhq9en9CnSGBSo&e=
More information about the dev
mailing list