2016-12-22 2:05 GMT-08:00 Zoltán Balogh <[email protected]>: > Hi Daniele, > > Thank you for the confirmation. I've used Ivy Bridge Xeon E5-2658 v2 running > at 3GHz. This is an older architecture than yours. Your results look better > for the new patch. I think we should take the new patch.
Great, I pushed the new patch to master. Thanks! Daniele > > Best regards, > Zoltan > > -----Original Message----- > From: Daniele Di Proietto [mailto:[email protected]] > Sent: Thursday, December 22, 2016 4:06 AM > To: Zoltán Balogh <[email protected]> > Cc: [email protected] > Subject: Re: [ovs-dev] [PATCH v2] odp-execute: Optimize IP header > modification in OVS datapath > > 2016-12-13 9:27 GMT-08:00 Zoltán Balogh <[email protected]>: >> >> Hi Daniele, >> >>> Have you tried avoiding also computing the new field if the mask is 0? >>> For example, if >>> mask->ipv4_src is 0, there's not reason to compute new_ip_src, or to >>> extract ip_src_nh. >> >> Yes, I have investigated this case in mid-October. My results was >> that execution of "output + dec_ttl" and "output + mod_nw_tos" became even >> faster, but execution of rest of the actions I investigated became slower >> compared to the patch I posted previously. >> So I rejected this modification. >> >> Now, I rebased the code to 9th Dec master and ran the tests with DPDK 16.11 >> again. Please find the new rebased patch below. >> The results of my original and the new patch: >> >> | T | T | I | I | >> | T | O | P | P | Vanilla OVS || + old patch | + new patch >> | L | S | s | d | (nsec/packet) || (nsec/packet) | (nsec/packet) >> -------+---+---+---+---+---------------++---------------+--------------- >> output | | | | | 67.19 || 67.19 | 67.19 >> | X | | | | 74.48 || 69.53 | 68.78 (+) >> | | X | | | 74.42 || 71.20 | 70.07 (+) >> | | | X | | 84.62 || 78.70 | 78.03 (+) >> | | | | X | 84.25 || 78.51 | 77.94 (+) >> | | | X | X | 97.46 || 91.98 | 91.86 (+) >> | X | | X | X | 100.42 || 95.00 | 96.00 (-) >> | X | X | X | X | 102.80 || 100.40 | 100.73 (-) >> >> I ran each test five times. The values are the mean of the readings obtained. >> >> As you can see, there is an improvement in the first 5 cases. But, in case of >> "output + dec_ttl + mod_nw_src + mod_nw_dst" and >> "output + dec_ttl + mod_nw_tos + mod_nw_src + mod_nw_dst", the execution of >> actions became slower compared to the old patch. However, this is still >> faster >> than vanilla OVS. >> >> Could you please confirm the results? >> Are these values acceptable? >> Is there anything still to improve in the new patch? > > Sorry for the delay and thanks for the details and the new version > > I've tried to reproduce your tests (64 bytes UDP phy-phy throughput) > on my system (2 Ghz haswell with ixgbe). > > Here are my results: > > | T | T | I | I | > | T | O | P | P | Vanilla OVS || + old patch | + new patch > | L | S | s | d | (nsec/packet) || (nsec/packet) | (nsec/packet) > -------+---+---+---+---+---------------++---------------+--------------- > output | | | | | 67.20 || 67.20 | 67.20 > | X | | | | 87.03 || 82.17 | 80.19 (+) > | | X | | | 87.03 || 81.37 | 81.50 (-) > | | | X | | 95.88 || 90.66 | 87.26 (+) > | | | | X | 95.51 || 90.42 | 87.18 (+) > | | | X | X | 107.53 || 103.41 | 100.20 (+) > | X | | X | X | 111.98 || 108.23 | 105.49 (+) > | X | X | X | X | 116.01 || 112.49 | 111.11 (+) > > There are no regressions compared to master in any case, which is good. > > Both versions look good to me. Since you did the original profiling, which > version do you prefer? > > Thanks, > > Daniele > >> >> The patch was applied to: 7971b36c3acc279f8e931d360f16c200752a3be2 >> >> Best regards, >> Zoltan >> >> Signed-off-by: Zoltán Balogh <[email protected]> >> >> --- >> >> diff --git a/lib/odp-execute.c b/lib/odp-execute.c >> index 65a6fcd..cc555b8 100644 >> --- a/lib/odp-execute.c >> +++ b/lib/odp-execute.c >> @@ -33,6 +33,7 @@ >> #include "flow.h" >> #include "unaligned.h" >> #include "util.h" >> +#include "csum.h" >> >> /* Masked copy of an ethernet address. 'src' is already properly masked. */ >> static void >> @@ -68,13 +69,50 @@ odp_set_ipv4(struct dp_packet *packet, const struct >> ovs_key_ipv4 *key, >> const struct ovs_key_ipv4 *mask) >> { >> struct ip_header *nh = dp_packet_l3(packet); >> + ovs_be32 ip_src_nh; >> + ovs_be32 ip_dst_nh; >> + ovs_be32 new_ip_src; >> + ovs_be32 new_ip_dst; >> + uint8_t new_tos; >> + uint8_t new_ttl; >> + >> + if (mask->ipv4_src) { >> + ip_src_nh = get_16aligned_be32(&nh->ip_src); >> + new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src); >> + >> + if (ip_src_nh != new_ip_src) { >> + packet_set_ipv4_addr(packet, &nh->ip_src, new_ip_src); >> + } >> + } >> >> - packet_set_ipv4( >> - packet, >> - key->ipv4_src | (get_16aligned_be32(&nh->ip_src) & ~mask->ipv4_src), >> - key->ipv4_dst | (get_16aligned_be32(&nh->ip_dst) & ~mask->ipv4_dst), >> - key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos), >> - key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl)); >> + if (mask->ipv4_dst) { >> + ip_dst_nh = get_16aligned_be32(&nh->ip_dst); >> + new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst); >> + >> + if (ip_dst_nh != new_ip_dst) { >> + packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst); >> + } >> + } >> + >> + if (mask->ipv4_tos) { >> + new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos); >> + >> + if (nh->ip_tos != new_tos) { >> + nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t) >> nh->ip_tos), >> + htons((uint16_t) new_tos)); >> + nh->ip_tos = new_tos; >> + } >> + } >> + >> + if (OVS_LIKELY(mask->ipv4_ttl)) { >> + new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl); >> + >> + if (OVS_LIKELY(nh->ip_ttl != new_ttl)) { >> + nh->ip_csum = recalc_csum16(nh->ip_csum, htons(nh->ip_ttl << 8), >> + htons(new_ttl << 8)); >> + nh->ip_ttl = new_ttl; >> + } >> + } >> } >> >> static const ovs_be32 * >> diff --git a/lib/packets.c b/lib/packets.c >> index ae81f46..4a5eb72 100644 >> --- a/lib/packets.c >> +++ b/lib/packets.c >> @@ -849,7 +849,7 @@ eth_compose(struct dp_packet *b, const struct eth_addr >> eth_dst, >> return data; >> } >> >> -static void >> +void >> packet_set_ipv4_addr(struct dp_packet *packet, >> ovs_16aligned_be32 *addr, ovs_be32 new_addr) >> { >> diff --git a/lib/packets.h b/lib/packets.h >> index 21bd35c..97d1b38 100644 >> --- a/lib/packets.h >> +++ b/lib/packets.h >> @@ -1093,6 +1093,8 @@ void *snap_compose(struct dp_packet *, const struct >> eth_addr eth_dst, >> unsigned int oui, uint16_t snap_type, size_t size); >> void packet_set_ipv4(struct dp_packet *, ovs_be32 src, ovs_be32 dst, >> uint8_t tos, >> uint8_t ttl); >> +void packet_set_ipv4_addr(struct dp_packet *packet, ovs_16aligned_be32 >> *addr, >> + ovs_be32 new_addr); >> void packet_set_ipv6(struct dp_packet *, const ovs_be32 src[4], >> const ovs_be32 dst[4], uint8_t tc, >> ovs_be32 fl, uint8_t hlmit); >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
