On 24.07.2017 22:53, Andy Zhou wrote:
> On Mon, Jul 24, 2017 at 9:23 AM, Ilya Maximets <i.maxim...@samsung.com> wrote:
>> On 23.07.2017 00:02, Darrell Ball wrote:
>>>
>>>
>>> -----Original Message-----
>>> From: <ovs-dev-boun...@openvswitch.org> on behalf of Andy Zhou
>>> <az...@ovn.org>
>>> Date: Friday, July 21, 2017 at 2:17 PM
>>> To: "<d...@openvswitch.org>" <d...@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 <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.
>>>
>>> [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.
>>
>>
>> Looks like L2 fields is not used for balance-tcp bonding since
>> 4f150744921f ("dpif-netdev: Use miniflow as a flow key.").
>> Maybe this was changed by mistake, but 5tuple hash is used in
>> recirculation for the last 3 years.
>>
>> bond_hash_tcp(), actually, doesn't have any valuable effect on
>> flows distribution if recirculation supported. I think, we can
>> avoid using any type of hash inside base bonding code.
>> I'll check this additionally later.
>>
>
> Agreed.
>
>> And I think we need to fix the documentation to remove misleading
>> L2 mentioning.
> Yes, do you mind include the change in your series.
I'll include this in v2.
>>
>>> >>
>>> >> '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
I'm not offended, but the first letter of my name is "I", not "L".
>>> > 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!
I like the below clean up. We don't need to have separate function,
if it's only purpose is to call another function with the same
parameters. I'll send v2 with that fix and I'll add you as a
co-author to that patch.
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://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=
>>>
>>>
>>>
>>>
>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
