On Sat, Jul 22, 2017 at 2:02 PM, Darrell Ball <db...@vmware.com> 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.
I agree with Llya's comment on this. When recirc is in use. this hashing value only affect the first packet. I would not consider this as loss of feature. > > 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. CFM is usually used for detect tunnel connectivity issues, thus it is usually sent within a tunneled packet. The most popular tunnels are UDP based, we should get a fair distruction with a 5 tuple hash. > > 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.” > I agree that documentation needs update. > 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.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://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