| Message ID | 1500643702-5955-1-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
Add dev mailing list. It got dropped by accident. ---------- Forwarded message ---------- From: Andy Zhou <azhou@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@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@ovn.org> wrote: > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maximets@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.maximets@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)); > }
-----Original Message----- From: <ovs-dev-bounces@openvswitch.org> on behalf of Andy Zhou <azhou@ovn.org> Date: Friday, July 21, 2017 at 2:17 PM To: "<dev@openvswitch.org>" <dev@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@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@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@ovn.org> wrote: > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maximets@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. >> >> '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@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@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=
On 23.07.2017 00:02, Darrell Ball wrote: > > > -----Original Message----- > From: <ovs-dev-bounces@openvswitch.org> on behalf of Andy Zhou <azhou@ovn.org> > Date: Friday, July 21, 2017 at 2:17 PM > To: "<dev@openvswitch.org>" <dev@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@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@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@ovn.org> wrote: > > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maximets@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. And I think we need to fix the documentation to remove misleading L2 mentioning. > >> > >> '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@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@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= > > > >
On Sat, Jul 22, 2017 at 2:02 PM, Darrell Ball <dball@vmware.com> wrote: > > > -----Original Message----- > From: <ovs-dev-bounces@openvswitch.org> on behalf of Andy Zhou <azhou@ovn.org> > Date: Friday, July 21, 2017 at 2:17 PM > To: "<dev@openvswitch.org>" <dev@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@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@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@ovn.org> wrote: > > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maximets@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@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@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= > > > >
On Mon, Jul 24, 2017 at 9:23 AM, Ilya Maximets <i.maximets@samsung.com> wrote: > On 23.07.2017 00:02, Darrell Ball wrote: >> >> >> -----Original Message----- >> From: <ovs-dev-bounces@openvswitch.org> on behalf of Andy Zhou <azhou@ovn.org> >> Date: Friday, July 21, 2017 at 2:17 PM >> To: "<dev@openvswitch.org>" <dev@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@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@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@ovn.org> wrote: >> > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maximets@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. > >> >> >> >> '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@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@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= >> >> >> >>
-----Original Message----- From: Andy Zhou <azhou@ovn.org> Date: Monday, July 24, 2017 at 12:50 PM To: Darrell Ball <dball@vmware.com> Cc: Ilya Maximets <i.maximets@samsung.com>, "ovs-dev@openvswitch.org" <ovs-dev@openvswitch.org> Subject: Re: [ovs-dev] Fwd: [PATCH] bond: Unify hash functions in hash action and entry lookup. On Sat, Jul 22, 2017 at 2:02 PM, Darrell Ball <dball@vmware.com> wrote: > > > -----Original Message----- > From: <ovs-dev-bounces@openvswitch.org> on behalf of Andy Zhou <azhou@ovn.org> > Date: Friday, July 21, 2017 at 2:17 PM > To: "<dev@openvswitch.org>" <dev@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@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@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@ovn.org> wrote: > > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maximets@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. agreed, I traced it and see it is partially implemented at this point. Since the use cases that I am interested in for dpdk need more plumbing at this point. it is outside the scope of such a patch, as the patch is a cleanup. > > 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. After offline discussion, it seems we have different use cases in mind. I think you are thinking about a particular product with a specific usage of CFM. In general, CFM is end-to-end and may not be initiated by OVS. > > 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. > > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_support_dist-2Ddocs_ovs-2Dvswitchd.conf.db.5.pdf&d=DwIFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=_T4mZQM545qzV9mHcGw-26QCK6TH-oPE8mHB1vXoZVI&s=0AR1_W6ywJjt8tLyMyS_X9FVb1B1seCHwH6pFvWngus&e= > > “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. good > 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@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@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= > > > >
On 24.07.2017 22:53, Andy Zhou wrote: > On Mon, Jul 24, 2017 at 9:23 AM, Ilya Maximets <i.maximets@samsung.com> wrote: >> On 23.07.2017 00:02, Darrell Ball wrote: >>> >>> >>> -----Original Message----- >>> From: <ovs-dev-bounces@openvswitch.org> on behalf of Andy Zhou <azhou@ovn.org> >>> Date: Friday, July 21, 2017 at 2:17 PM >>> To: "<dev@openvswitch.org>" <dev@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@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@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@ovn.org> wrote: >>> > On Fri, Jul 21, 2017 at 6:28 AM, Ilya Maximets <i.maximets@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.maximets@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 >>> dev@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= >>> >>> >>> >>> > > >
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
'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.maximets@samsung.com> --- ofproto/bond.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)