Add dev mailing list. It got dropped by accident.
---------- Forwarded message ----------
From: Andy Zhou <az...@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.maxim...@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 <az...@ovn.org> wrote:
> On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maxim...@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.
>>
>> '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.maxim...@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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
