On Mon, Jul 24, 2017 at 9:23 AM, Ilya Maximets <[email protected]> wrote: > On 23.07.2017 00:02, Darrell Ball wrote: >> >> >> -----Original Message----- >> From: <[email protected]> on behalf of Andy Zhou >> <[email protected]> >> Date: Friday, July 21, 2017 at 2:17 PM >> To: "<[email protected]>" <[email protected]> >> 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 <[email protected]> >> 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 <[email protected]> >> >> >> 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 <[email protected]> wrote: >> > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets >> <[email protected]> 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. > >> >> >> >> '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 <[email protected]> >> >> --- >> >> 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 >> [email protected] >> >> 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
