[ovs-dev] Fwd: [PATCH] bond: Unify hash functions in hash action and entry lookup.
Andy Zhou
azhou at ovn.org
Mon Jul 24 19:50:15 UTC 2017
On Sat, Jul 22, 2017 at 2:02 PM, Darrell Ball <dball at vmware.com> wrote:
>
>
> -----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.
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.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